Repository navigation
fix(rocDecode): Bound the VP9 parser reads and fix the quantizer and loop filter arithmetic - #12624
Merged
jeffqjiangNew merged 7 commits intoOct 2, 2026
Conversation
- CheckSuperframe() reads num_frames sizes of up to 4 bytes each from the superframe index and stored them in frame_sizes_ without comparing them to the chunk they describe. ParsePictureData() then passes each one to ParseUncompressedHeader() as the available size and advances pic_data_ptr by it, so both the length handed to the header parser and the pointer step came straight from the bitstream. A single frame size can reach about 4 GB, and the effect compounds across the frames of the superframe. - Sum the sizes and require them to fit the chunk ahead of the index. One aggregate check covers the whole walk: the prefix sums are bounded by the total, so each frame is guaranteed to fit in the bytes that remain. The comparison is <= rather than an exact match so that a chunk carrying trailing padding still decodes. - The size accumulation shifted p_stream[offset], which promotes to int, by up to 24 bits. With size_bytes equal to 4 the last byte overflows the signed 32 bit type, which is undefined. Cast to uint32_t before the shift. - CheckSuperframe() returned void and had nowhere to report any of this. It now returns a ParserResult and ParsePictureData() propagates the failure. - The index parsing loop itself is already in bounds: offset starts at chunk_data_size - size_index + 1 and reads exactly size_index - 2 bytes, ending at chunk_data_size - 2. It is left as is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- DcQ() and AcQ() index dc_qlookup[3][256] and ac_qlookup[3][256] with the row (bit_depth - 8) >> 1. The column was clamped, the row was not. - The color config is coded only in key frames and intra only frames; every other frame carries the previous one over. uncompressed_header_ is zeroed in the constructor and the per frame memset in ParseUncompressedHeader() is commented out on purpose, so bit_depth is still 0 if an inter frame reaches the parser before any frame that codes a color config. The row index is then (0 - 8) >> 1, which is -4, reading 2048 bytes ahead of each table. - This does not need a malformed stream. Feeding the parser a packet from the middle of a stream, as a seek to a non key frame would, is enough. - Check bit_depth for 8, 10 or 12 in ParseUncompressedHeader() before SetupSegDequant() uses it, and return PARSER_WRONG_STATE otherwise. Clamping alone would have kept decoding with the 8 bit tables for a frame that cannot be decoded at all. - Clamp the row in DcQ() and AcQ() as well. They return int and have no way to report an error, so the bound belongs there too, alongside the clamp already applied to the column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rameInit(). - The loop filter ref and mode deltas are int8_t read as signed values, and the defaults alone include -1. Shifting them left by n_shift is undefined for a negative value in C++17. Spec 4.4 defines << as a zero filling two's complement shift, which for these magnitudes is multiplication by 2^n_shift, so scale by that multiplier instead. No output changes. - Spec 8.8.1 applies Clip3(0, MAX_LOOP_FILTER, x) to signed intermediates, but lvlSeg, intraLvl and interLvl were accumulated into uint8_t and only cast back to int inside the clamp. A negative result wrapped before the clamp could act on it: with lvl_seg 0 and a delta of -1 the level became (uint8_t)(-1), so clamp(255) selected 63, the maximum filter strength, where the spec asks for 0. - segmentation_params.feature_data is int16_t, so the same wrap applied one level up when a negative SEG_LVL_ALT_L delta drove lvl_seg below zero. Hold all three intermediates in int. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeffqjiangNew
requested review from
LakshmiKumar23
and
a balanced review from Copilot
October 1, 2026 14:16
jeffqjiangNew
requested review from
AryanSalmanpour,
kiritigowda and
rrawther
as code owners
October 1, 2026 14:16
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Zero-length frames and signed index truncation leave two out-of-bounds access paths unresolved.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Hardens VP9 parsing against malformed superframe sizes, invalid quantizer indexing, and loop-filter arithmetic errors.
Changes:
- Validates aggregate superframe sizes and propagates failures.
- Guards quantizer table indexing by bit depth.
- Uses signed, defined loop-filter arithmetic.
| File | Description |
|---|---|
projects/rocdecode/src/parser/vp9_parser.h |
Updates the superframe parser return contract. |
projects/rocdecode/src/parser/vp9_parser.cpp |
Implements bounds checks and arithmetic fixes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- chunk_data_size is uint32_t and RocdecSourceDataPacket::payload_size is too, so a caller can pass a chunk larger than INT_MAX. chunk_data_size - size_index + 1 evaluates in uint32_t, and narrowing that into an int offset is implementation defined above INT_MAX. Clang wraps it to a negative value, and p_stream[offset++] then reads ahead of the buffer. - Hold the index in size_t instead. - The exposure here is an oversized payload_size from the application rather than anything in the bitstream. The other two reads in this function, p_stream[chunk_data_size - 1] and p_stream[chunk_data_size - size_index], index with uint32_t expressions that widen to size_t without truncation, so they are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The uncompressed header syntax is driven by the bitstream, and Parser::GetBit() and Parser::ReadBits() take no length, so a frame whose advertised size is shorter than the header it codes read past the end of the picture data. ParseUncompressedHeader() only compares uncomp_header_size_ with size once the whole header has been read, by which point the reads have already happened. - This does not need a superframe. The non superframe path sets frame_sizes_[0] to the chunk size with no minimum, so a one byte packet reaches ParseUncompressedHeader() with size 1 and the frame_sync_code alone reads past it. The aggregate superframe check added earlier bounds the walk between frames, not the reads inside one. - Add ReadBitsChecked() and GetBitChecked(), which refuse a read that would pass the end of the current frame, and route the 59 read sites and ReadSigned() through them. A refused read leaves bit_offset alone, records bitstream_overrun_ and yields 0, so no call site needs an error path and the five helpers that return void keep their signatures. - Reject the frame on bitstream_overrun_ at the show_existing_frame early return and once the header syntax is complete, before anything acts on values a refused read left at 0. The reads are already safe at that point; the check is what turns a truncated header into an error rather than a decode of zeros. - The limit is held in frame_data_size_in_bits_ and set once per frame, so the shared Parser:: helpers are untouched and the AVC, HEVC and AV1 parsers are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeffqjiangNew
changed the title
…parse. - The uncompressed header carries state forward between frames, and SetupPastIndependence() resets part of it part way through the parse: all of segmentation_params.feature_data and feature_enabled, the loop filter deltas and ref_frame_sign_bias. It writes straight into uncompressed_header_. - Every failure exit in ParseUncompressedHeader() comes after that point, the new overrun check and the pre-existing uncomp_header_size_ > size check included. ParsePictureData() logs the failure and moves on to the next frame in the chunk, so a truncated key or intra frame wiped the carried state and a following inter frame that omits those updates was then decoded against the wiped state. - Snapshot uncompressed_header_ on entry and restore it when the parse fails. Doing it in a wrapper covers all of the failure exits from one place instead of restoring at each, and it covers the syntax error paths that had the same problem before this branch. - y_dequant_, uv_dequant_ and lvl_lookup_ are left alone: they are derived and recomputed by every successful frame before anything reads them. pic_width_, new_seq_activated_ and reconfig_option_ are left alone as well, because the size change block that sets them already runs after the overrun check. - ParsePictureData() still skips a failed frame and continues with the rest of the chunk. With the state rolled back the following frames see the carried state they should. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ell. - The previous commit rolled back uncompressed_header_ only, on the reasoning that the size change block runs after the overrun check so a rejected frame never reaches it. That was wrong: the overrun check is not the last failure exit. The block assigns pic_width_ and pic_height_ and then returns PARSER_WRONG_STATE for an unsupported size up on a non key frame, and uncomp_header_size_ is assigned after the block with two more failure exits below it. - A failed frame therefore left pic_width_ and pic_height_ holding the size it had advertised. A following frame of that same size then compared equal, skipped the size change block entirely, and could be submitted against the older and smaller surfaces. - Extend the transaction to uncomp_header_size_, pic_width_, pic_height_, curr_surface_width_, curr_surface_height_, reconfig_option_ and new_seq_activated_, plus last_frame_type_ and frame_is_intra_, which the body also assigns. last_frame_type_ has no reader today, so it is inert rather than broken, but it carries the spec's LastFrameType and should not survive a frame that was rejected. - frame_data_size_in_bits_ and bitstream_overrun_ stay out of the transaction because the body sets both before its first read. y_dequant_, uv_dequant_ and lvl_lookup_ stay out because they are derived and recomputed by every successful frame before anything reads them. The comment on the wrapper records both exclusions so the rule is clear when the body gains state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rrawther
approved these changes
Oct 2, 2026
jeffqjiangNew
deleted the
users/jeffqjiangNew/rocDecode_vp9_parser_error_handle
branch
October 2, 2026 23:07
Contributor
TheRock Submodule Bump ActivityNewest first
|
jeffqjiangNew
added a commit
that referenced
this pull request
Oct 6, 2026
…idy EbspToRbsp(). - None of the four merged parser bounds PRs updated CHANGELOG.md: HEVC #12407, AVC #12477, AV1 #12493 and VP9 #12624 all landed without an entry. Comparable work in this project is recorded, so add one line per codec under the unreleased 1.10.0 Resolved issues, plus one for the shared NAL unit and SEI parsing this branch changes. - EbspToRbsp() reported end_bytepos as the size and PARSER_OK when the end came before the start, which is not a length under any reading. Return PARSER_INVALID_ARG with a size of 0 instead. Unreachable today, since all nine callers pass a begin of 0, but letting a failure be read as a length is the thing this branch set out to stop. - Mark the start code offset ordering check as deliberate defence rather than leaving it to be read as dead code. It is not reachable with the current callers, which reset both offsets per picture and only assign them values the scan bound already constrains. - Note that sei_message_type is uint8_t in the public RocdecSeiMessage, so an SEI type above 255 truncates on store. That is pre-existing and not what the wide accumulation is for, which is to stop the running total wrapping before the range checks see it. - reduce_count becomes size_t, since it is a byte count subtracted from a size_t span. count stays int: it is a zero run state of 0 to 2, not a size. - Restore the trailing newline on roc_video_parser.cpp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 task
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.
Motivation
This PR addresses 6 review findings in
vp9_parser: three out-of-bounds accesses driven by values thebitstream controls, one signed narrowing of a chunk index, one piece of undefined behaviour
sitting next to a spec deviation in the same expressions, and one case of a rejected frame
leaving parser state half updated for the next one.
Technical Details
Uncompressed header reads —
ParseUncompressedHeader()and its helpersThe header syntax is driven by the bitstream and
Parser::GetBit/ReadBitstake no length,so a frame whose advertised size is shorter than the header it codes read past the end of
the picture data; the existing
uncomp_header_size_ > sizecheck only runs once the wholeheader has been read. Not superframe-specific — the non-superframe path sets
frame_sizes_[0]to the chunk size with no minimum, so a 1-byte packet overruns on theframe_sync_codealone.New
ReadBitsChecked()/GetBitChecked()refuse a read past the end of the current frame,and the 59 read sites plus
ReadSigned()route through them. A refused read leavesbit_offsetalone, records an overrun and yields 0, so no call site needed an error pathand the five
voidhelpers kept their signatures. The frame is rejected on that flag oncethe syntax is complete. The limit is a member set once per frame, so the shared
Parser::helpers — and the AVC, HEVC and AV1 parsers — are untouched.
Carried state on a failed parse —
ParseUncompressedHeader()The header carries state forward between frames, and the body mutates it before its later
failure exits:
SetupPastIndependence()resets the segmentation features and loop filterdeltas mid-parse, and the size change block assigns
pic_width_/pic_height_before thecheck that rejects an unsupported size up on a non-key frame. Since
ParsePictureData()skips a failed frame and continues with the chunk, a truncated key frame wiped the carried
state for the next one, and a rejected size change left a following frame of that size
comparing equal — skipping the size change check and being submitted against the older,
smaller surfaces.
ParseUncompressedHeader()is now a wrapper that snapshots what the body can touch andrestores it on failure, with the body in
ParseUncompressedHeaderBody(). A wrapper ratherthan a restore at each exit, because it also covers the pre-existing syntax-error exits that
had the same problem.
frame_data_size_in_bits_/bitstream_overrun_are excluded (the bodysets both before its first read), as are
y_dequant_/uv_dequant_/lvl_lookup_(derived,recomputed by every successful frame before use); both exclusions are documented on the
wrapper so the rule is clear when the body gains state.
Superframe index frame sizes —
CheckSuperframe()The index supplies
num_framessizes of up to 4 bytes each, whichParsePictureData()usedboth as the length passed to the header parser and as the pointer step, unchecked against
the chunk. Now summed and required to fit the chunk ahead of the index — one aggregate check
suffices, since the prefix sums are bounded by the total. The comparison is
<=rather thanan exact match so a chunk with trailing padding still decodes.
Two smaller fixes in the same function: the size accumulation shifted an
int-promoted byteby up to 24 bits, now cast to
uint32_tfirst; and the chunk index was anintnarrowedfrom a
uint32_texpression, nowsize_t. That second one is reachable from an oversizedpayload_sizefrom the application rather than from the bitstream.CheckSuperframe()returned
voidand so had nowhere to report any of this; it now returns aParserResult.Quantizer table row index —
DcQ(),AcQ()Both index
[3][256]tables with the row(bit_depth - 8) >> 1; the column was clamped, therow was not. The color config is coded only in key and intra-only frames, and
uncompressed_header_is zeroed once in the constructor, sobit_depthis still 0 if aninter frame arrives first — a row index of −4, reading 2048 bytes ahead of each table. A seek
to a non-key frame is enough to reach it.
bit_depthis now checked for 8, 10 or 12 before use and the frame rejected otherwise;clamping alone would have decoded with the 8-bit tables for a frame that cannot be decoded at
all. The row is clamped too, since
DcQ()/AcQ()returnintand cannot report.Loop filter levels —
LoopFilterFrameInit()The ref and mode deltas are
int8_tand the defaults include −1, so the left shift byn_shiftwas undefined. Spec 4.4 defines<<as a zero-filling two's complement shift,which at these magnitudes is multiplication by
2^n_shift; the code scales by that instead.No output change.
Spec 8.8.1 clips signed intermediates, but
lvlSeg,intraLvlandinterLvlwereaccumulated into
uint8_tand only cast tointinside the clamp, so a negative resultwrapped first —
lvl_seg0 with a delta of −1 clamped to 63, maximum filter strength wherethe spec asks for 0.
feature_dataisint16_t, so the same wrap applied one level up. Allthree are now
int.n_shiftis unchanged: 8.8.1 setsnShiftfromloop_filter_levelbefore the per-segmentsteps begin, not from the per-segment
lvlSeg.Issue Tracking
JIRA ID: ROCM-31875, AICV-310
Test Plan
Run /validate.
Test Result
╔══════════════════════════════╗
║ rocDecode Validation Summary ║
╠══════════════════════════════╣
║ CTest (16 tests) ✓ PASSED ║
║ Conformance AVC ✓ PASSED ║
║ Conformance AV1 ✓ PASSED ║
║ Conformance HEVC ✓ PASSED ║
║ Conformance VP9 ✓ PASSED ║
╠══════════════════════════════╣
║ Overall ✓ PASSED ║
╚══════════════════════════════╝
Conformance breakdown
┌───────┬─────────┬────────┬────────┬────────────┐
│ Codec │ Streams │ Passed │ Failed │ Unfinished │
├───────┼─────────┼────────┼────────┼────────────┤
│ AVC │ 127 │ 127 │ 0 │ 0 │
├───────┼─────────┼────────┼────────┼────────────┤
│ AV1 │ 162 │ 162 │ 0 │ 0 │
├───────┼─────────┼────────┼────────┼────────────┤
│ HEVC │ 135 │ 135 │ 0 │ 0 │
├───────┼─────────┼────────┼────────┼────────────┤
│ VP9 │ 108 │ 108 │ 0 │ 0 │
├───────┼─────────┼────────┼────────┼────────────┤
│ Total │ 532 │ 532 │ 0 │ 0 │
└───────┴─────────┴────────┴────────┴────────────┘
Submission Checklist