Skip to content

Bound setMediaStream search to the trak atom size #9286

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.

Sign up for GitHub

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

Open
MarkLee131 wants to merge 2 commits into
base: main
Choose a base branch
from
+11 −4
Open

Bound setMediaStream search to the trak atom size #9286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5cb228
Bound setMediaStream search to the trak atom size
MarkLee131 Mar 29, 2026
fd3f4fb
Update include/exiv2/quicktimevideo.hpp
MarkLee131 Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion include/exiv2/quicktimevideo.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,10 @@ class EXIV2API QuickTimeVideo : public Image {
/*!
@brief Recognizes which stream is currently under processing,
and save its information in currentStream_ .
@param atom_size Full size of the atom currently being processed, in bytes,
including both the atom header and its payload.
*/
void setMediaStream();
void setMediaStream(size_t atom_size);
/*!
@brief Used to discard a tag along with its data. The Tag will
be skipped and not decoded.
Expand Down
11 changes: 8 additions & 3 deletions src/quicktimevideo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ void QuickTimeVideo::tagDecoder(Exiv2::DataBuf& buf, size_t size, size_t recursi
fileTypeDecoder(size);

else if (equalsQTimeTag(buf, "trak"))
setMediaStream();
setMediaStream(size);

else if (equalsQTimeTag(buf, "mvhd"))
movieHeaderDecoder(size);
Expand Down Expand Up @@ -1126,13 +1126,18 @@ void QuickTimeVideo::NikonTagsDecoder(size_t size) {
io_->seek(cur_pos + size, BasicIo::beg);
} // QuickTimeVideo::NikonTagsDecoder

void QuickTimeVideo::setMediaStream() {
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();
Comment on lines +1129 to +1133
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
DataBuf buf(4 + 1);

while (!io_->eof()) {
while (!io_->eof() && io_->tell() + 4 <= search_end) {
io_->readOrThrow(buf.data(), 4);
Comment on lines +1129 to 1137
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
if (equalsQTimeTag(buf, "hdlr")) {
if (io_->tell() + 12 > search_end)
break;
Comment on lines +1136 to +1140
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

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()).

Suggested change
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;

Copilot uses AI. Check for mistakes.
io_->readOrThrow(buf.data(), 4);
io_->readOrThrow(buf.data(), 4);
io_->readOrThrow(buf.data(), 4);
Expand Down