[jeffqjiangNew]

Motivation

This PR addresses 5 review findings in roc_video_parser, the base shared by the AVC and HEVC parsers.
Three are out-of-bounds reads reachable from a malformed or truncated packet. The other two
both corrupt the size that every downstream parse function is handed, which makes the length
checks inside those functions inert: an error sentinel that no caller checked, and an RBSP
length that was reported larger than the buffer holding it.

Technical Details

Invalid emulation prevention sequence — EbspToRbsp()

EbspToRbsp() returned (size_t)-1 when 0x000003 was followed by a byte above 0x03, and
none of its nine callers checked for it. They assigned it into rbsp_size_ and passed that
on. rbsp_size_ was an int, so the sentinel landed as −1, and every parse function it
feeds — ParseSps, ParsePps, ParseVps, ParseSliceHeader, ParseSeiMessage — takes
size_t, so it arrived as SIZE_MAX. Any length check inside those functions is then inert:
the SEI bounds below are four offset < size tests that all pass unconditionally when size
is SIZE_MAX.

It now returns a ParserResult with the size handed back through an out parameter, so a
failure cannot be read as a length. All nine call sites log and skip the NAL unit, which is
what they already did for a failed parameter set or slice header parse. rbsp_size_ becomes
size_t to match what it is passed to.

Reported RBSP length — EbspToRbsp()

The same function reported end_bytepos - begin_bytepos + reduce_count. Every byte counted
in reduce_count has just been removed from the buffer, so adding it gives a length larger
than the EBSP ever held — 00 00 03 01 is 3 bytes of RBSP but was reported as 5. Callers
pass that on as the size of rbsp_buf_, a fixed 1024 bytes that ebsp_size already fills in
the worst case, so the parse functions were told the buffer was larger than it is. Same class
of problem as the sentinel above.

The subtraction is now the right way round. The cabac_zero_word path breaks with the
trailing 0x03 still in the buffer and the spec discards it, so it is counted in
reduce_count before the break; that keeps reduce_count meaning the number of discarded
bytes and makes one subtraction correct at both exits.

This predates the branch, and conformance does not exercise it: the parse functions stop at
the end of the syntax they need rather than the end of the buffer, so an over-reported size
changes nothing for a well formed stream.

SEI message parsing — ParseSeiMessage()

The function took the NAL unit size but used it only in the loop condition at the bottom, so
a malformed SEI read past the end four ways: the two ff_byte scans ran unbounded; the
terminating byte after each was read without checking the offset; payload_size came
straight from the bitstream and was never compared against what was left, so the memcpy
appending the payload read an attacker-chosen length; and both accumulators were int taking
+= 255 in an unbounded loop, overflowing a signed type before anything caught it.

Every read is now bounded by size, payload_size must fit the bytes left after the
headers, and the accumulation is size_t so the running total cannot wrap before it is
checked. SEI is supplemental, so the parse stops at the first bad message and keeps the ones
already read; the result is returned for the record and the callers carry on with the
picture rather than failing it. That contract is written on the declaration.

Separately, sei_payload_buf_size_ was not updated when the payload buffer grew. Writes
stayed in range because the buffer is never smaller than the recorded size, so this was
repeated reallocation rather than an overflow, but the bookkeeping was wrong.

Start code scan — GetNalUnit()

The scan ran while curr_byte_offset_ < pic_data_size_ - 2. Both are uint32_t, so a
one-byte packet made the bound 0xFFFFFFFF and the loop read three bytes at a time far past
the end. AVC and HEVC both guard payload && payload_size, so a zero length packet cannot
arrive, but a one byte packet can; two bytes was already safe by accident. The size is now
tested separately.

NAL unit offsets and sizes — signedness

curr_start_code_offset_, next_start_code_offset_ and nal_unit_size_ were int while
pic_data_size_ and curr_byte_offset_ are uint32_t, so the arithmetic between them
converted signedness. All three are now uint32_t.

The int was load bearing, so it is replaced rather than dropped. nal_unit_size_ was
assigned from pic_data_size_ - curr_start_code_offset_, which already evaluates unsigned,
so an offset past the end gave a huge value that became negative entering the int and was
caught by the nal_unit_size_ >= 4 / >= 5 floors in the callers. Widening the member alone
would have let that through the floor and into a memcpy of RBSP_BUF_SIZE bytes from past
the end of the packet. GetNalUnit() now range checks the two offsets before deriving the
size, replacing the accidental backstop with a deliberate one.

ebsp_size and sei_ebsp_size were int for the same reason: with nal_unit_size_
unsigned, a NAL unit larger than INT_MAX would have made them negative, and sei_ebsp_size
reaches new uint8_t[] and memcpy() directly. Both are uint32_t now.

Issue Tracking

JIRA ID: ROCM-31875, AICV-310

Test Plan

Run /validate.

Test Result

validate 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.

[therock-pr-bot]

✅ 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_parser.cpp, projects/rocdecode/src/parser/hevc_parser.cpp, projects/rocdecode/src/parser/roc_video_parser.cpp, projects/rocdecode/src/parser/roc_video_parser.h; 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]

Copilot review overview

🟡 Changes recommended

The converted RBSP length incorrectly adds removed bytes, allowing parsing beyond the valid buffer.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Subtract removed emulation-prevention bytes from reported RBSP length · New
• Update API documentation for output size and ParserResult return · New

What changed in this PR

Bounds AVC/HEVC NAL and SEI parsing against malformed or truncated packets.

Changes:

• Adds NAL offset and SEI payload bounds checks.
• Replaces the unchecked RBSP size sentinel with ParserResult.
• Corrects parser size types and SEI buffer bookkeeping.

File	Description
roc_video_parser.h	Updates parser types and method contracts.
roc_video_parser.cpp	Adds shared NAL, RBSP, and SEI validation.
hevc_parser.cpp	Handles RBSP conversion failures for HEVC.
avc_parser.cpp	Handles RBSP conversion failures for AVC.

---

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

[Copilot]

Copilot review overview

🟡 Changes recommended

Exact-fit SEI buffer growth retains quadratic reallocation behavior and enables resource exhaustion.

Review effort: Balanced
Findings: 1

Open (1)

• Use geometric capacity growth to prevent quadratic SEI payload copying · New

Resolved since last review (2)

• Subtract removed emulation-prevention bytes from reported RBSP length
• Update API documentation for output size and ParserResult return

[Copilot]

Copilot review overview

🔵 Needs a closer look

Security-sensitive parsing changes for attacker-controlled input warrant final human review despite no additional defects found.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Use geometric capacity growth to prevent quadratic SEI payload copying

[Copilot]

Copilot review overview

🟢 Approval recommended

The updated bounds, size calculations, and caller error handling are consistent and address the reported safety issues.

Review effort: Balanced
Findings: None

[rrawther]

added a comment

[rrawther]

I see some revew comments are not resolved. After that it can be merged

[rrawther]

Also please take a look at this
3. Issues found
None blocking. I re-audited the delta for new problems and found none:

All 9 EbspToRbsp call sites check the result and skip the NAL unit; I enumerated them and confirmed there is no 10th.
reduce_count cannot exceed end − begin — each increment is paired with one distinct discarded byte in the span (the memmove branch also decrements streamBuffer_end) — so the new size_t subtraction at roc_video_parser.cpp:234 cannot underflow.
The nal_payload_size hoist is arithmetically identical to what it replaced, just evaluated where the floor is visible.
No public header or dispatch-table change → no ABI version bump required, correctly omitted.
4. Suggestions
CHANGELOG.md:17 — "HVEC" is a typo for "HEVC". This is user-facing release-note text; worth fixing before merge. The same line also credits AV1 and VP9, which this PR does not touch (VP9 landed via #12624) — defensible as a consolidated entry, but if it's meant to describe this PR it overstates its scope.
roc_video_parser.h:231-232 — the doc comment names only the malformed-emulation-sequence cause of PARSER_INVALID_ARG; end_bytepos < begin_bytepos now returns it too. One clause would make the contract complete.
Unit tests — I agree with @AryanSalmanpour's request. EbspToRbsp is a pure function over a byte buffer and the corrected end − (begin + reduce_count) formula is exactly the kind of thing that silently regresses. My ad-hoc harness covers these cases today but lives outside the repo.
Follow-up, out of scope: es_reader.cpp:840 still carries a near-duplicate EbspToRbsp with the same return -1 sentinel that neither caller checks, and its callers at lines 611 and 713 memcpy a fixed 256 bytes out of the stream buffer unconditionally. Same bug class, separate PR.
Minor: roc_video_parser.cpp:188 is missing a space after the comma in the signature, and the header's parameter name is stream_buffer where the definition uses streamBuffer.

[Copilot]

🟡 Changes recommended

Extended SEI types are misreported, and valid large payload accumulation can be rejected.

2 open findings

• Preserve extended SEI payload types without 8-bit truncation · New
• Prevent capacity doubling from dropping representable SEI payloads · New

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Security-sensitive bitstream parsing and malformed-input handling warrant final human review.

0 open findings

2 resolved since last review

• Prevent capacity doubling from dropping representable SEI payloads
• Preserve extended SEI payload types without 8-bit truncation

🧠 Review effort: Balanced

[AryanSalmanpour]

Re-review at fe19a15

Re-reviewed against 3d0a553, the commit of my earlier review. Every point raised there has been addressed — seven in code, one in comments — and I found no new defects in the changes.

Worth calling out as better than what I suggested:

• The SEI buffer growth is now correct rather than merely guarded. Capping the doubling at 0xFFFFFFFF/2 and falling back to the exact requirement means the capacity check can no longer reject a growth whose requirement would itself have fit, and the >4 GB case fails loudly instead of narrowing. I ran the loop through its edges, including needed just below and just above 0xFFFFFFFF; it never produces a capacity that overflows the uint32_t field.
• Retyping sei_message_count_ and num_slices_ to uint32_t, rather than casting at the comparison, removes the sign mismatch at the source and carried through to the four dependent for (int slice_index …) loops in both parsers. num_slices_ - 1 at hevc_parser.cpp:475 is safe because it only evaluates inside the slice_index < num_slices_ loop, so the zero case cannot wrap.
• Skipping an SEI message whose payload_type exceeds 0xFF is a sharper answer than the truncation note I left. Delivering type 261 as type 5 would have had the consumers read it as SEI_TYPE_USER_DATA_UNREGISTERED, so dropping it with a log is the right call, and stepping over an already bounds-checked payload keeps the surrounding messages intact.

On the early return from the PARSER_INVALID_FORMAT branch: the new comment argues that neither reporting condition can fire with slices already accumulated. I checked that rather than take it on trust — pic_data_size_ is assigned exactly once per call, at avc_parser.cpp:107 and hevc_parser.cpp:572, before the NAL loop, so the short-packet test can only fire on the first pass while num_slices_ is still 0. The claim holds, and flagging the constraint for whoever adds a third condition is the useful part.

The one thing I raised that was not taken is the observation that the pic_data_size_ < 3 check is loop-invariant but re-evaluated per NAL unit. That was a nit and I do not think it is worth pursuing.

Resolving my threads accordingly. The standing gap remains test coverage: EbspToRbsp is a pure function over a byte buffer, and a handful of table-driven cases (00 00 03 01, a trailing 00 00 03, consecutive escapes, a malformed 00 00 03 FF, end < begin) would pin this behaviour cheaply against regression.

[systems-assistant]

TheRock Submodule Bump Activity

Newest first

• 2026-10-12 — Included in TheRock via ROCm/TheRock#8932.