Repository navigation
Conversation
Owner
|
The commit needs to be split, it does too much and the message is not clear at all. |
DoviRpu::parse read and parsed the whole payload first and only then compared the CRC32. Corrupt data therefore reached the parser, where the panics and over-allocations fixed in the following commits could be triggered by a stream that would have been rejected anyway. Read the stored CRC32 from the end of the data and compare it before read_rpu_data. The error message is unchanged. Add a test module that crafts RPUs bit by bit, used by the tests of this series.
A polynomial curve with poly_order_minus1 == 0 and linear_interp_flag set hit unimplemented!() in both parsing and writing. A crafted RPU could therefore panic the process, which aborts when built as the C library with panic=abort. Replace both with bail!(). The message is the same as before.
num_pivots_minus2 was read as an unbounded ue(v) and used as the size of the pivots vector. A large value overflowed the addition or tried to allocate that many entries, which panics or aborts the process. The spec allows at most 8 pieces per component, that is 9 pivots, so reject values above 7 before allocating. FFmpeg enforces the same limit (AV_DOVI_MAX_PIECES).
A level 8, 9 or 10 block with a length other than the defined ones (8: 10, 12, 13, 19 or 25 bytes, 9: 1 or 17, 10: 5 or 21) was read without complaint, and required_bits() then hit unreachable!() while parsing, in validate_and_read_remaining. No RPU that parsed before is rejected by this check. Check the length at the start of parse() and validate(), the same way level253 already does.
WithExtMetadataBlocks::write computed the padding with required_bits() before the block was written and validated. For a level 8, 9 or 10 block with an undefined length, which can be set through the C API or the JSON generation path, that hit unreachable!(). Compute the padding after the block has been written, so the validation error is returned first.
num_ext_blocks was read as an unbounded ue(v) and passed to with_blocks_allocation(), which reserves that many blocks up front. A large count overflowed the allocation size or exhausted memory, which panics or aborts the process before any block is read. Every block takes at least 9 bits (ext_block_length and ext_block_level), so reject counts above available_bits / 9. A valid stream is never rejected by this.
convert_av1_rpu_payload_to_regular allocated emdf_payload_size + 1 bytes, with the size read from the stream, before checking that the data holds that many bytes. A small T.35 payload declaring a size of several megabytes allocated that much and then failed on the read, and a larger size could abort the process. Return an error if the size exceeds the remaining data.
parse_variable_bits accumulated the value in a u32 with plain +, << and +. Enough groups with the read_more flag set overflowed it: a panic in debug builds, and a silently wrong size in release builds. Use checked_add and checked_mul and return an error on overflow.
pkajaba
force-pushed
the
harden-rpu-parsing-v2
branch
from
October 16, 2026 19:06
97ee971 to
2442e3a
Compare
Author
|
Thanks for the review, and sorry for the oversized commit. I split it into eight small commits, one per check, each with its own test. Each builds and passes
Why this matters: the C API is built with |
Owner
|
Most of this seems AI assisted so it might take a while to go through and verify everything. |
Author
yes, they were, I will put a disclaimer about it into a PR description. Is there anything I can do to help with verification? |
Owner
|
Not really, it just feels mostly like fuzzing and I'm not really sure yet if all of it is necessary. |
Owner
Owner
|
Thanks for reporting, all the issues are fixed in the main branch. |
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.
A malformed RPU currently panics or over-allocates in several places. In a player that uses the C API (built with
panic = abort), that takes down the whole process instead of failing one frame. This PR turns each case into anErr.I ran into these while adding Dolby Vision Profile 7 FEL support to AetherEngine (superuser404notfound/AetherEngine#702), which passes RPUs from arbitrary files to the C API.
What went wrong and how it is handled now:
DoviRpu::parsecompared the CRC only after the payload had been parsed, so corrupt data went through the whole parser first. The CRC is now compared beforeread_rpu_data. Same error message, same set of accepted RPUs (the stored CRC is read from the same last five bytes the parser used); only the order of errors changes.unimplemented!on parse and write is nowbail!.ext_block_lengthreachedunreachable!inrequired_bits. A smallensure_valid_lengthhelper (same pattern aslevel253.rs) now runs inparseandvalidate. Allowed lengths: L8 10/12/13/19/25, L9 1/17, L10 5/21.num_pivots_minus2.num_pivots_minus2is now limited to 7 (at most 9 pivots, the range in the spec and in ffmpeg's AV_DOVI_MAX_PIECES). Previously larger values were accepted when the data was present, and very large values overflowed or over-allocated.dovi_generate_from_json).num_ext_blocksis checked against the bits left (available_bits / 9, the smallest possible block) before theVecis allocated.emdf_payload_sizemust fit in the remaining data before allocating, andparse_variable_bitsuses checked arithmetic.Behaviour change for callers: inputs that used to panic or abort now return
Err. Valid RPUs are parsed as before. Streams with undefined L8/L9/L10 lengths always panicked before and now return an error. Streams with more than 9 pivots were previously accepted and are now rejected, since the spec allows at most 9.Tests: new
dolby_vision/src/rpu/tests.rsbuilds malformed RPUs bit by bit (7 tests), plus 3 tests inav1/emdf.rs. Each new negative test fails on currentmain, 7 of 8 by a panic.Testing:
cargo fmt --check;cargo clippy -- -D warningsindolby_visionwith--all-targets,--all-targets --features capiand--all-features --all-targets --tests;cargo testat the root (37 + 73, includes the real sample RPUs inassets/tests) andcargo test --all-features --all-targetsindolby_vision(all pass). Root clippy in the--all-features --all-targets --testsform fails only on two pre-existing unused imports insrc/tests/rpu.rs, which this patch does not touch.AI assistance: this patch, its tests and this description were written with the help of Claude (Anthropic's AI assistant). Each commit was built and tested as listed above before it was pushed.