Stop reading past the end of the file in BinaryFileReader - #87
Open
arpitjain099 wants to merge 1 commit into
Open
arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This branch has not been deployed
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.
BinaryFileReader.readBytesallocatesnew byte[length]and then loopsbytes[bp] = (byte) inputStream.read()without ever checking the return value, so the -1 thatread()returns at end of file is stored as 0xFF. The length usually comes out of the header being parsed, which means a file can ask for more bytes than it contains and get them.What that looks like through the CLI: take a DPX written by ffmpeg, set USER_DEFINED_HEADER_LENGTH at offset 32 to 132, and
-jsonreportsNone of those bytes are in the file. With the field set to 0x7FFFFFFF the same file asks for a 2 GiB array instead.
The guard for this is already written and never called:
canRead(int)andhasMoreData()are defined and, across the whole repository, those two lines are their only occurrences. This wirescanReadintoreadBytes, fixes its off-by-one so that reading exactly up to the end of the file is allowed, and makesskipcount the bytes it moves past so the position it compares against is right.read()returning -1 mid-array is now anIOExceptionrather than 0xFF.createDPXFileInformationalready catches the failure and prints "Failed to parse file metadata", but leaves the file in the list with null metadata, andJsonWriterDpxthen dereferenced it. That was unreachable in practice before, since parsing rarely failed; it is reachable now, so the writer skips those files with a message instead.Verified by building the CLI the way
embARC_Checks.ymldoes and running it on the five ffmpeg frames from that workflow:-applyChangesFromJSONthen-jsonstill finds PLACEHOLDERTEXT, and a complete DPX produces the same output as before. The four malformed files now print "No metadata was read from , skipping it" instead of returning fabricated content.