Use readOrThrow in MatroskaVideo::decodeBlock for EOF detection#9282
Use readOrThrow in MatroskaVideo::decodeBlock for EOF detection#9282MarkLee131 wants to merge 1 commit intoExiv2:mainfrom
Conversation
Four io_->read() calls in decodeBlock() do not check the return value. On a truncated file, the buffer may contain stale data from the previous iteration. Replace them with readOrThrow() to throw on short reads. This is consistent with the ASF parser fix in PR Exiv2#3005.
There was a problem hiding this comment.
Pull request overview
This PR updates the Matroska (MKV) metadata parser to avoid using partially-read buffers when parsing truncated files, aligning behavior with prior EOF-handling hardening done in other parsers.
Changes:
- Replace four unchecked
io_->read()calls inMatroskaVideo::decodeBlock()withio_->readOrThrow(...). - Ensure short reads during tag-id, size, and payload reads are treated as corrupted metadata (exception) instead of silently continuing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| io_->read(buf, 1); | ||
| io_->readOrThrow(buf, 1, ErrorCode::kerCorruptedMetadata); |
There was a problem hiding this comment.
decodeBlock() still uses io_->read(buf, 1) for the first byte and only checks eof(). If the read fails due to an I/O error (e.g., ferror) without setting EOF, buf[0] can remain uninitialized and the subsequent tag parsing will use garbage. Consider checking the returned byte count and io_->error(), or switching to readOrThrow() with a special-case for clean EOF (0 bytes + eof()) to stop traversal.
|
|
||
| DataBuf buf2(bufMaxSize + 1); | ||
| io_->read(buf2.data(), size); | ||
| io_->readOrThrow(buf2.data(), size, ErrorCode::kerCorruptedMetadata); |
There was a problem hiding this comment.
These new readOrThrow() paths change truncated-file behavior from “silently continue with stale/partial data” to “throw kerCorruptedMetadata”. Since there are existing gtest unit tests for MatroskaVideo, it would be good to add a regression test that feeds a minimal valid MKV header followed by a truncated element and asserts readMetadata() throws (and does not populate metadata).
Fix #9281:
Four io_->read() calls in decodeBlock() do not check the return value. On a truncated file, the buffer may contain stale data from the previous iteration.
Replace them with readOrThrow() to throw on short reads. This is consistent with the ASF parser fix in PR #3005.