Fuzzing fixes#95
Open
bobsayshilol wants to merge 8 commits into
Open
Conversation
See comment for why this change is necessary.
MP4File::GetTrackH264SeqPictHeaders() can return early if it fails to parse or allocate, which means that MP4GetTrackH264SeqPictHeaders() can return success and look like the pointers need freeing even though some may still be nullptrs. To fix this, guard the derefs.
We need an extra byte for the terminating NUL, otherwise the strncat() can write past the end or the caller can read past the end of the string.
ASAN flags up that these read out of bounds on bad data.
MP4ConvertTime() can throw on bad data which ends up leaking |chapters| since it's not owned by anything. Add some RAII to plug that leak.
If the reallocation or overflow check fails then an exception will be thrown, leaving the container thinking that it has the new size when it's still using the old data. This lead to cases where we'd try and free memory that we didn't allocate.
This is created as a 2 bit MP4BitfieldProperty, so read it as that.
Author
|
Didn't think to search the closed issues but it looks like #74 (comment) was closed incorrectly. The trace shown there matches that of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is basically a copy of enzo1982/mp4v2#63 brought over because this fork seems more active. I checked each commit and only the ones that actually fixed issues in this fork were kept. There's also 2 new fixes that were found when running against the old corpus (the last 2 commits).
See individual commits for more details. Sanitizer output in summaries below:
edaad04 - Avoid leaking memory if |new MP4BytesProperty()| throws
c5d62c9 - Fix an OOM and stack overflow
1accd5d - Fix crash on nullptr
9aacbe7 - Fix read/write out of bounds due to off-by-one error
e93427b - Bounds check tag sizes before attempting to read them
^ this fixes #90
7f5d4ed - Plug a leak if an exception is thrown
ccc1d7e - Assign array size after reallocation
ab04a28 - Cast to the correct property type