Skip to content

saiz/saio/hvcc: bound Vec::with_capacity on counts#162

Merged
bradh merged 2 commits into
kixelated:mainfrom
rhoopr:fix/bounded-vec-capacity-saiz-saio-hvcc
May 1, 2026
Merged

saiz/saio/hvcc: bound Vec::with_capacity on counts#162
bradh merged 2 commits into
kixelated:mainfrom
rhoopr:fix/bounded-vec-capacity-saiz-saio-hvcc

Conversation

@rhoopr

@rhoopr rhoopr commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Closes #156. Follow-up to #157.

Three more Vec::with_capacity sites pass a count read straight from input as the capacity argument, with no upper bound:

  • saiz sample_count (Vec<u8>, u32): worst case ~4 GiB upfront.
  • saio entry_count (Vec<u64>, u32): worst case ~34 GiB upfront for v1 (8 bytes/entry).
  • hvcc num_nalus (Vec<Vec<u8>>, u16): worst case ~1.5 MiB upfront (24 bytes per Vec<u8> slot).

Fix

Same idiom as #157: pre-flight reject counts that can't fit in the remaining buffer (count > buf.remaining() / N, where N is the per-element minimum), then cap the upfront reservation.

  • saiz/saio cap at 4096 to match trun.rs. Covers typical CMAF segments (hundreds to low thousands of samples) without reallocating.
  • hvcc caps at 64. Real HEVC arrays hold a handful of VPS/SPS/PPS/SEI entries, so 4096 would be well past anything realistic.

Tests

Three regression tests pinned to Error::OverDecode (the wrapper Atom::decode_maybe produces from Error::OutOfBounds). All 210 existing tests still pass.

Test plan

  • cargo test --all-features
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo fmt -- --check

Three more `Vec::with_capacity` sites pass a count read straight from
input as the capacity argument, with no upper bound:

- `saiz` `sample_count` (`Vec<u8>`, `u32`): worst case ~4 GiB upfront.
- `saio` `entry_count` (`Vec<u64>`, `u32`): worst case ~34 GiB upfront
  for v1 (8 bytes/entry).
- `hvcc` `num_nalus` (`Vec<Vec<u8>>`, `u16`): worst case ~1.5 MiB
  upfront (24 bytes per `Vec<u8>` slot).

Same defensive idiom as kixelated#157: pre-flight reject counts that cannot
possibly fit in the remaining buffer (`count > buf.remaining() / N`,
where N is the per-element minimum), then cap the upfront reservation.

- saiz/saio: cap at 4096 to match `trun.rs` — covers typical CMAF
  segments (hundreds to low thousands of samples) without reallocating.
- hvcc: cap at 64 — real HEVC arrays hold a handful of VPS/SPS/PPS/SEI
  entries, so 4096 would be well past anything realistic.

Adds regression tests pinned to `Error::OverDecode` (the wrapper that
`Atom::decode_maybe` produces from `Error::OutOfBounds`).

Closes kixelated#156.
@coderabbitai

coderabbitai Bot commented Apr 30, 2026

Copy link
Copy Markdown

Walkthrough

Two decoder modules add validation to prevent oversized vector allocations. The SAIZ and SAIO decoders pre-validate sample_count and entry_count fields against remaining buffer length, capping allocations to 4096 elements. The HVCC decoder validates num_nalus before NALU vector allocation, limiting initial reservation to 64 entries. Both changes reject impossible counts with Error::OverDecode. New regression tests confirm that maximum unsigned integer values are handled cleanly without memory issues.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description is directly related to the changeset, explaining the issue, the fix applied, and the test coverage added.
Linked Issues check ✅ Passed The pull request fully addresses the requirements from issue #156: pre-flight validation of counts against buffer length, capping Vec::with_capacity at 4096 (saiz/saio) and 64 (hvcc), and adding regression tests with Error::OverDecode assertions.
Out of Scope Changes check ✅ Passed All changes are within scope—modifications to saiz, saio, and hvcc decoders plus their regression tests directly address the unbounded allocation issue described in issue #156.
Title check ✅ Passed The title 'saiz/saio/hvcc: bound Vec::with_capacity on counts' accurately summarizes the main change: bounding unbounded Vec allocations by capping the capacity on attacker-controlled counts in three decoder modules.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

These public items in the touched files lacked docstrings, dragging
the diff's docstring coverage below the 80% threshold the review bot
checks against. Add minimal spec references matching the existing
docstring style on `Saiz` / `Saio`.
@rhoopr rhoopr changed the title saiz/saio/hvcc: bound Vec::with_capacity on attacker-controlled counts saiz/saio/hvcc: bound Vec::with_capacity on counts Apr 30, 2026
@bradh bradh merged commit a6025ae into kixelated:main May 1, 2026
1 check passed
@github-actions github-actions Bot mentioned this pull request Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unbounded Vec::with_capacity in saiz/saio and hvcC (follow-up to #154)

2 participants