Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(unreal): Allow to parse with limit [INGEST-565] #447

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Nov 9, 2021

To avoid unbounded allocation on bad input data, this PR introduces a new API
Unreal4Crash::parse_with_limit that allows specifying a maximum allocation
size.

@jan-auer jan-auer requested a review from a team November 9, 2021 19:34
return Err(Unreal4ErrorKind::Empty.into());
}

let mut decompressed = Vec::new();
std::io::copy(&mut ZlibDecoder::new(bytes), &mut decompressed)
ZlibDecoder::new(slice)
.take(limit as u64 + 1)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alertnative to the +1 hack, I can offer this:

        let mut decompressed = Vec::new();
        let decoder = &mut ZlibDecoder::new(slice);

        decoder
            .take(limit as u64)
            .read_to_end(&mut decompressed)
            .map_err(|e| Unreal4Error::new(Unreal4ErrorKind::BadCompression, e))?;

        if !matches!(decoder.read(&mut [0; 1]), Ok(0)) {
            return Err(Unreal4ErrorKind::TooLarge.into());
        }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that might be a good alternative, yes, as it makes the limit really explicit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the current code prevent you from using u64::MAX as the limit? Not that one byte more or less is likely to make a difference, but still, I think it's kind of surprising behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, that's also why the default limit is set to usize::MAX - 1 in the parse method above.

Going to update this with code from the comment and add a test, then.

return Err(Unreal4ErrorKind::Empty.into());
}

let mut decompressed = Vec::new();
std::io::copy(&mut ZlibDecoder::new(bytes), &mut decompressed)
ZlibDecoder::new(slice)
.take(limit as u64 + 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that might be a good alternative, yes, as it makes the limit really explicit.

@jan-auer jan-auer changed the title fix(unreal): Allow to parse with limit fix(unreal): Allow to parse with limit [INGEST-565] Nov 11, 2021
@jan-auer jan-auer enabled auto-merge (squash) November 11, 2021 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants