Skip to content

fix(rocDecode): Bound AVC ref pic list modification and MMCO array indices - #12477

Merged
jeffqjiangNew merged 2 commits into
developfrom
users/jeffqjiangNew/rocDecode_avc_parser_error_handle
Oct 1, 2026
Merged

jeffqjiangNew merged 2 commits into
developfrom
users/jeffqjiangNew/rocDecode_avc_parser_error_handle

Conversation

@jeffqjiangNew

Copy link
Copy Markdown
Contributor

Motivation

A review pass over avc_parser.cpp turned up three out-of-bounds accesses in the slice
header's reference picture handling. All three share the same shape: the loop index is not
derived from a syntax element at all, it is bounded only by a terminator value appearing
in the bitstream
. A stream that simply never emits the terminator runs the index without
limit.

The slice header is parsed into a stack local (AvcVideoParser::ParseNalUnit()), so an
unterminated list walks off the end of a stack object.

Two of the three arrays are also sized smaller than the specification allows, so the last
legal entry of a conforming stream already wrote one element past the end. That means a
bare capacity check would have rejected valid streams; the arrays are resized as well.

The third finding is a read-side overflow with a correctness bug attached, described below.

Technical Details

Ref pic list modification — ParseSliceHeader()

modification_l0 and modification_l1 were filled by do { ... } while (modification_of_pic_nums_idc != 3). The index had no bound other than that terminator,
against arrays of AVC_MAX_REF_PICTURE_NUM (32) entries.

7.4.3.1 bounds the list: the number of entries with modification_of_pic_nums_idc not equal
to 3 shall not exceed num_ref_idx_lX_active_minus1 + 1. Both loops are now capped at
num_ref_idx_lX_active_minus1 + 1, allowing for the terminating entry.
num_ref_idx_lX_active_minus1 is already range checked to 31 or less on every path that
reaches these loops — the PPS defaults at ParsePps() and the slice override at
ParseSliceHeader(), whose check is inside the same slice_type guard that gates the L0
loop, with the L1 loop reachable only for B slices — so the largest index is 32.

That is one past the end of a 32-entry array, and it is reachable from a conforming stream:
a field-coded slice may carry 32 active references and 32 modification entries, plus the
terminator. Both arrays are therefore sized with a new AVC_MAX_REF_LIST_MOD_NUM, 32
entries plus the terminator.

Decoded reference picture marking — ParseSliceHeader()

The MMCO loop has the same structure, terminating on
memory_management_control_operation == 0, writing into mmco[AVC_MAX_REF_PICTURE_NUM].

Unlike the modification lists, the specification places no single limit on the number of
MMCO operations in a slice, so the previous size of 32 was an assumption rather than a
bound. A limit is derived instead: each of the up to 32 reference fields can be marked
unused for reference by operation 1 or 2 and be assigned a long term frame index by
operation 3 or 6, plus one operation 4, one operation 5, and the terminating operation 0.
That becomes AVC_MAX_MMCO_NUM (67), the array is resized to it, and the loop is capped
by it.

Ref pic list modification process — ModifiyRefList()

ModifiyRefList() is called once per list, receiving modification_l0 or modification_l1
as a parameter, but the advance at the bottom of its while loop was hardcoded to
p_slice_header->ref_pic_list.modification_l0 regardless. Only the first entry came from
the array the caller passed.

This is an out-of-bounds read as well as a correctness bug. The loop's only exit condition
is finding modification_of_pic_nums_idc == 3 in the array it is walking. On the L1 call it
looked for that terminator in L0's array, and when ref_pic_list_modification_flag_l0 is 0
nothing was ever parsed into modification_l0 for this slice — the slice header is an
uninitialized stack local, copied wholesale into slice_info_list_, so the walk searched
indeterminate memory for a terminator that need not be there.

The fix saves the base pointer on entry and advances within the list the caller passed.
Indexing by ref_idx_lx is unchanged and remains correct: each iteration consumes exactly
one entry and increments ref_idx_lx exactly once, for modification_of_pic_nums_idc 0, 1
and 2 alike, so the entry index and ref_idx_lx stay in step.

This also corrects list 1 construction, which previously drew on list 0's operations
whenever an L1 modification carried more than one entry.

Debug dump — PrintSliceHeader()

The three dump loops iterated a hardcoded 32 entries, which after the resizes would print
neither the full modification lists nor a meaningful fraction of the MMCO array. They now
follow the new sizes and stop at the terminator, and at mmco_count for the MMCO dump. This
runs only at ROCDEC_LOG_LEVEL=4.

Issue Tracking

JIRA ID: ROCM-31875, AICV-310

Test Plan

Run /validate

Test Result

Full validation passed.

╔══════════════════════════════╗
║ 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

…rays.

 - ParseSliceHeader() filled ref_pic_list.modification_l0 / modification_l1 and
   dec_ref_pic_marking.mmco with do/while loops whose index was bounded only by
   an in-bitstream terminator, modification_of_pic_nums_idc equal to 3 and
   memory_management_control_operation equal to 0. All three arrays were sized
   AVC_MAX_REF_PICTURE_NUM, so a stream that simply never emits the terminator
   walked the index past 32 and wrote out of bounds.
 - Cap the modification loops at num_ref_idx_lX_active_minus1 + 1, the bound
   7.4.3.1 places on the entries with modification_of_pic_nums_idc not equal to
   3, allowing for the terminating entry. num_ref_idx_lX_active_minus1 is
   already range checked to 31 or less on every path that reaches these loops,
   so the largest index is 32.
 - AVC_MAX_REF_PICTURE_NUM was also one entry short of what those bounds permit,
   so a bare capacity check would have rejected conforming streams. Size the two
   modification arrays with AVC_MAX_REF_LIST_MOD_NUM, 32 entries plus the
   terminator.
 - The spec places no single limit on the MMCO count, so derive one: each of the
   up to 32 reference fields can be marked unused for reference by operation 1
   or 2 and be assigned a long term frame index by operation 3 or 6, plus one
   operation 4, one operation 5, and the terminating operation 0. Apply that as
   AVC_MAX_MMCO_NUM and cap the loop with it.
 - ModifiyRefList() advanced through ref_pic_list.modification_l0 regardless of
   which list it was called for, so on the list 1 call it searched for the
   terminator in list 0's array. When ref_pic_list_modification_flag_l0 is 0
   that array holds no terminator for the current slice and the walk ran off the
   end. Advance through the list the caller passed in. This also fixes list 1
   being built from list 0's operations whenever an L1 modification has more
   than one entry.
 - PrintSliceHeader() dumped a hardcoded 32 entries of each array. Follow the
   new sizes and stop at the terminator, and at mmco_count for the MMCO dump.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rocdecode/src/parser/avc_defines.h, projects/rocdecode/src/parser/avc_parser.cpp; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The changes consistently prevent the identified overflows while preserving valid AVC limits and correcting L1 processing.

Review effort: Balanced
Findings: None

What changed in this PR

Bounds AVC reference-list and MMCO parsing to prevent out-of-bounds access while correcting list-1 modification handling.

Changes:

  • Adds correctly sized limits for modification and MMCO arrays.
  • Enforces parser bounds on bitstream-driven loops.
  • Corrects list selection and debug output iteration.
File Description
projects/​rocdecode/​src/​parser/​avc_parser.cpp Adds bounds checks, fixes list traversal, and updates debug dumps.
projects/​rocdecode/​src/​parser/​avc_defines.h Defines expanded capacities for modification and MMCO entries.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jeffqjiangNew
jeffqjiangNew merged commit 5b1daf2 into develop Oct 1, 2026
84 of 85 checks passed
@jeffqjiangNew
jeffqjiangNew deleted the users/jeffqjiangNew/rocDecode_avc_parser_error_handle branch October 1, 2026 01:16
systems-assistant Bot pushed a commit to ROCm/rocDecode that referenced this pull request Oct 1, 2026
 array indices (#12477)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Motivation

A review pass over `avc_parser.cpp` turned up three out-of-bounds
accesses in the slice
header's reference picture handling. All three share the same shape: the
loop index is not
derived from a syntax element at all, it is bounded only by a
**terminator value appearing
in the bitstream**. A stream that simply never emits the terminator runs
the index without
limit.

The slice header is parsed into a stack local
(`AvcVideoParser::ParseNalUnit()`), so an
unterminated list walks off the end of a stack object.

Two of the three arrays are also sized smaller than the specification
allows, so the last
legal entry of a *conforming* stream already wrote one element past the
end. That means a
bare capacity check would have rejected valid streams; the arrays are
resized as well.

The third finding is a read-side overflow with a correctness bug
attached, described below.

## Technical Details

**Ref pic list modification — `ParseSliceHeader()`**

`modification_l0` and `modification_l1` were filled by `do { ... } while
(modification_of_pic_nums_idc != 3)`. The index had no bound other than
that terminator,
against arrays of `AVC_MAX_REF_PICTURE_NUM` (32) entries.

7.4.3.1 bounds the list: the number of entries with
`modification_of_pic_nums_idc` not equal
to 3 shall not exceed `num_ref_idx_lX_active_minus1 + 1`. Both loops are
now capped at
`num_ref_idx_lX_active_minus1 + 1`, allowing for the terminating entry.
`num_ref_idx_lX_active_minus1` is already range checked to 31 or less on
every path that
reaches these loops — the PPS defaults at `ParsePps()` and the slice
override at
`ParseSliceHeader()`, whose check is inside the same `slice_type` guard
that gates the L0
loop, with the L1 loop reachable only for B slices — so the largest
index is 32.

That is one past the end of a 32-entry array, and it is reachable from a
conforming stream:
a field-coded slice may carry 32 active references and 32 modification
entries, plus the
terminator. Both arrays are therefore sized with a new
`AVC_MAX_REF_LIST_MOD_NUM`, 32
entries plus the terminator.

**Decoded reference picture marking — `ParseSliceHeader()`**

The MMCO loop has the same structure, terminating on
`memory_management_control_operation == 0`, writing into
`mmco[AVC_MAX_REF_PICTURE_NUM]`.

Unlike the modification lists, the specification places no single limit
on the number of
MMCO operations in a slice, so the previous size of 32 was an assumption
rather than a
bound. A limit is derived instead: each of the up to 32 reference fields
can be marked
unused for reference by operation 1 or 2 *and* be assigned a long term
frame index by
operation 3 or 6, plus one operation 4, one operation 5, and the
terminating operation 0.
That becomes `AVC_MAX_MMCO_NUM` (67), the array is resized to it, and
the loop is capped
by it.

**Ref pic list modification process — `ModifiyRefList()`**

`ModifiyRefList()` is called once per list, receiving `modification_l0`
or `modification_l1`
as a parameter, but the advance at the bottom of its `while` loop was
hardcoded to
`p_slice_header->ref_pic_list.modification_l0` regardless. Only the
first entry came from
the array the caller passed.

This is an out-of-bounds read as well as a correctness bug. The loop's
only exit condition
is finding `modification_of_pic_nums_idc == 3` in the array it is
walking. On the L1 call it
looked for that terminator in L0's array, and when
`ref_pic_list_modification_flag_l0` is 0
nothing was ever parsed into `modification_l0` for this slice — the
slice header is an
uninitialized stack local, copied wholesale into `slice_info_list_`, so
the walk searched
indeterminate memory for a terminator that need not be there.

The fix saves the base pointer on entry and advances within the list the
caller passed.
Indexing by `ref_idx_lx` is unchanged and remains correct: each
iteration consumes exactly
one entry and increments `ref_idx_lx` exactly once, for
`modification_of_pic_nums_idc` 0, 1
and 2 alike, so the entry index and `ref_idx_lx` stay in step.

This also corrects list 1 construction, which previously drew on list
0's operations
whenever an L1 modification carried more than one entry.

**Debug dump — `PrintSliceHeader()`**

The three dump loops iterated a hardcoded 32 entries, which after the
resizes would print
neither the full modification lists nor a meaningful fraction of the
MMCO array. They now
follow the new sizes and stop at the terminator, and at `mmco_count` for
the MMCO dump. This
runs only at `ROCDEC_LOG_LEVEL=4`.

## Issue Tracking

JIRA ID: ROCM-31875, AICV-310

## Test Plan

Run /validate

## Test Result

Full validation passed.

  ╔══════════════════════════════╗
  ║  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

- [ ] Look over the contributing guidelines at
https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
[rocm-systems] ROCm/rocm-systems#12477 (commit 5b1daf2)
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants