Bound setMediaStream search to the trak atom size#9286
Bound setMediaStream search to the trak atom size#9286MarkLee131 wants to merge 2 commits intoExiv2:mainfrom
Conversation
setMediaStream() scans for "hdlr" in a while(!eof) loop with no size bound. When a file has no "hdlr" atom in the expected position, the loop reads through the entire rest of the file. Pass the trak atom size and stop the search at that boundary.
There was a problem hiding this comment.
Pull request overview
This PR addresses Exiv2 issue #9285 by bounding QuickTimeVideo::setMediaStream()’s search for the hdlr atom to the enclosing trak atom’s data size, preventing an unbounded scan across the rest of the file when hdlr is missing.
Changes:
- Pass the
trakatom data size intosetMediaStream()fromtagDecoder. - Add an explicit end position (
search_end) and stop scanning once thetrakboundary is reached. - Update the public header declaration to match the new
setMediaStream(size_t)signature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/quicktimevideo.cpp |
Bounds the hdlr scan within the trak payload and updates the call site to pass the size. |
include/exiv2/quicktimevideo.hpp |
Updates setMediaStream declaration to accept the size parameter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void QuickTimeVideo::setMediaStream(size_t atom_size) { | ||
| size_t current_position = io_->tell(); | ||
| size_t search_end = current_position + atom_size; | ||
| if (search_end > io_->size()) | ||
| search_end = io_->size(); |
There was a problem hiding this comment.
The new atom_size parameter is actually the box payload size (the size passed from tagDecoder is boxSize - headerSize). Consider renaming this parameter (and related locals like search_end) to reflect that it’s a payload/data-size bound, or update call sites to pass the full box size, to avoid future misuse/off-by-header confusion.
| while (!io_->eof() && io_->tell() + 4 <= search_end) { | ||
| io_->readOrThrow(buf.data(), 4); | ||
| if (equalsQTimeTag(buf, "hdlr")) { | ||
| if (io_->tell() + 12 > search_end) | ||
| break; |
There was a problem hiding this comment.
The bounds checks use io_->tell() + N arithmetic (+4 in the loop condition and +12 when handling hdlr). Using addition on size_t can overflow; it would be safer to express these as io_->tell() <= search_end - N (after checking search_end >= N) or by computing remaining = search_end - io_->tell() as done elsewhere in this file (e.g., io_->size() - io_->tell()).
| while (!io_->eof() && io_->tell() + 4 <= search_end) { | |
| io_->readOrThrow(buf.data(), 4); | |
| if (equalsQTimeTag(buf, "hdlr")) { | |
| if (io_->tell() + 12 > search_end) | |
| break; | |
| while (!io_->eof()) { | |
| // Ensure there are at least 4 bytes remaining before reading | |
| size_t pos = io_->tell(); | |
| if (pos >= search_end) | |
| break; | |
| size_t remaining = search_end - pos; | |
| if (remaining < 4) | |
| break; | |
| io_->readOrThrow(buf.data(), 4); | |
| if (equalsQTimeTag(buf, "hdlr")) { | |
| // After reading "hdlr", ensure there are at least 12 bytes remaining | |
| pos = io_->tell(); | |
| if (pos >= search_end) | |
| break; | |
| remaining = search_end - pos; | |
| if (remaining < 12) | |
| break; |
| void QuickTimeVideo::setMediaStream(size_t atom_size) { | ||
| size_t current_position = io_->tell(); | ||
| size_t search_end = current_position + atom_size; | ||
| if (search_end > io_->size()) | ||
| search_end = io_->size(); | ||
| DataBuf buf(4 + 1); | ||
|
|
||
| while (!io_->eof()) { | ||
| while (!io_->eof() && io_->tell() + 4 <= search_end) { | ||
| io_->readOrThrow(buf.data(), 4); |
There was a problem hiding this comment.
setMediaStream now relies on the trak size bound to avoid scanning the rest of the file when hdlr is missing. There are existing QuickTime regression tests for long-running parsers under tests/bugfixes/github/; please add a new regression test (with a small PoC .mov/.mp4 fixture) that exercises a trak without an hdlr atom and ensures the command completes quickly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix #9285:
setMediaStream() scans for "hdlr" in a while(!eof) loop with no size bound. When a file has no "hdlr" atom in the expected position, the loop reads through the entire rest of the file.
Pass the trak atom size and stop the search at that boundary.