Skip to content

Stop reading past the end of the file in BinaryFileReader - #87

Open
arpitjain099 wants to merge 1 commit into
LibraryOfCongress:mainfrom
arpitjain099:fix/reader-bounds
Open

arpitjain099 wants to merge 1 commit into
LibraryOfCongress:mainfrom
arpitjain099:fix/reader-bounds

Conversation

@arpitjain099

Copy link
Copy Markdown

BinaryFileReader.readBytes allocates new byte[length] and then loops bytes[bp] = (byte) inputStream.read() without ever checking the return value, so the -1 that read() 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 -json reports

"User Identification": "HEX: FFFFFFFFFFFFFFFF..."   (100 bytes)
"User Defined Data":   "HEX: FFFFFFFFFFFFFFFF..."   (32 bytes)

None 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) and hasMoreData() are defined and, across the whole repository, those two lines are their only occurrences. This wires canRead into readBytes, fixes its off-by-one so that reading exactly up to the end of the file is allowed, and makes skip count the bytes it moves past so the position it compares against is right. read() returning -1 mid-array is now an IOException rather than 0xFF.

createDPXFileInformation already catches the failure and prints "Failed to parse file metadata", but leaves the file in the list with null metadata, and JsonWriterDpx then 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.yml does and running it on the five ffmpeg frames from that workflow: -applyChangesFromJSON then -json still 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.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

This branch has not been deployed

No deployments
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.

1 participant