Fix verified correctness issues from #197 - #214
Conversation
Addresses the low-severity, verified correctness items tracked in #197. Prefers spec-correctness over back-compat (some previously-"successful" but malformed encodes now return Err instead of silently truncating). Core / types: - u24/u48 TryFrom now reject out-of-range values (new Error::OutOfRange) instead of silently truncating; adds u48::MAX. - FixedPoint Debug prints the true value (int + dec/2^bits), not int/dec. - Header::encode uses checked arithmetic and a 32-bit-safe largesize branch; decode paths (sync/ReadFrom/tokio) use usize::try_from so a 64-bit largesize no longer truncates on 32-bit targets. - Atoms >4 GiB now re-encode via the 64-bit largesize form instead of Error::TooLarge (new BufMut::insert_slice + shared write_box_size helper). - Mdhd gets a manual Default of language "und" so it round-trips (empty string isn't representable in the packed form). Codecs: - avcC: extension block gated on profile_idc (High-profile set incl. 244) instead of leftover bytes; Avcc::new emits the ext for High profiles; SPS(<=31)/PPS/NAL count & length overflow checks. - hvcC: array/NALU count & length overflow checks. - av01: fix initial_presentation_delay underflow and add range checks. - colr: full_range_flag decoded as (byte & 0x80 != 0). - FLAC: opaque metadata blocks retain their payload and round-trip, and the last-metadata-block flag is always emitted (variant shapes changed). - eac3: reject >13-bit data_rate and empty/oversized substream lists. - uncv: return Err instead of panicking on a missing component_type_uri. - subs: size version derived from all subsamples (errors on mixed widths); codec_specific_parameters length enforced to 4. moov / moof: - prft: nonconformant real_time without consistent_offset now errors. - mfra: Mfro.parent_size recomputed on encode; tfra drops an .expect(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughThe changes add shared 32-bit and 64-bit box-size backfilling, buffer insertion support, and checked largesize decoding. They introduce explicit range errors for fixed-width values and strengthen validation across AV1, AVC, HEVC, E-AC-3, subsample, and component encoders. FLAC metadata payloads are preserved for round-tripping. Additional updates recalculate fragment sizes, define 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@CodeRabbit: review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs (1)
137-155: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate bit-packed substream fields to prevent adjacent field corruption.
While the outer list and
data_rateare properly validated, the bit-packed fields within each substream are left unchecked. If an caller provides an out-of-range value (e.g.,acmod > 0b111orbsid > 0b11111), the bitwise left-shifts (<<) will overflow into neighboring fields within the encoded byte. This silently corrupts the container format, defeating the PR's objective of rejecting invalid codec fields.Enforce boundary checks on the bit-packed properties before packing them.
🛡️ Proposed validation
header.encode(buf)?; for substream in &self.substreams { + if substream.fscod > 0b11 + || substream.bsid > 0b11111 + || substream.bsmod > 0b111 + || substream.acmod > 0b111 + || substream.num_dep_sub > 0b1111 + || substream.chan_loc.unwrap_or(0) > 0x1FF + { + return Err(Error::OutOfRange); + } // low bit is reserved = 0 let b = (substream.fscod << 6) | (substream.bsid << 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moov/trak/mdia/minf/stbl/stsd/eac3.rs` around lines 137 - 155, Validate each substream’s bit-packed fields before encoding in the substream loop: ensure fscod, bsid, bsmod, acmod, and num_dep_sub fit their allocated bit widths, and ensure chan_loc fits the 9-bit dependent-channel field when applicable. Return the established validation error for any out-of-range value before performing the shifts, while preserving the existing packing for valid substreams.
🧹 Nitpick comments (1)
src/buf.rs (1)
145-149: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid double-reallocation when extending the buffer.
Because
split_offshrinks the capacity ofselfto exactlypos, the subsequentextend_from_slice(val)call will trigger a reallocation. Furthermore, becauseselfwas reallocated, it is no longer contiguous withtail, causingunsplit(tail)to degenerate intoextend_from_slice(tail.as_ref())which triggers a second reallocation.You can explicitly reserve enough capacity upfront to fit both
valandtail, reducing this to a single reallocation and preventing the double-copy of the existing bytes.⚡ Proposed optimization
fn insert_slice(&mut self, pos: usize, val: &[u8]) { let tail = self.split_off(pos); + self.reserve(val.len() + tail.len()); self.extend_from_slice(val); self.unsplit(tail); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/buf.rs` around lines 145 - 149, Update Buf::insert_slice to reserve capacity for the final buffer size before splitting or extending, accounting for both val.len() and the existing tail. Preserve the current insertion order and unsplit behavior while ensuring extend_from_slice and unsplit do not cause separate reallocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mfra/tfra.rs`:
- Around line 86-91: Update the integer-width encoding match so the 0 and 1
branches use fallible conversions from value to u8 and u16, respectively,
matching the existing try_into behavior in the u24 branch and propagating
conversion errors instead of silently truncating.
In `@src/moov/trak/mdia/minf/stbl/stsd/flac.rs`:
- Around line 87-89: Replace the `as u32` cast in the payload-length calculation
with a checked `u32::try_from(data.len())`, mapping conversion failure to
`Error::TooLarge(Dfla::KIND)` before the existing `u24` conversion. Preserve the
subsequent `u24` bounds validation so lengths exceeding either limit fail
explicitly.
---
Outside diff comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/eac3.rs`:
- Around line 137-155: Validate each substream’s bit-packed fields before
encoding in the substream loop: ensure fscod, bsid, bsmod, acmod, and
num_dep_sub fit their allocated bit widths, and ensure chan_loc fits the 9-bit
dependent-channel field when applicable. Return the established validation error
for any out-of-range value before performing the shifts, while preserving the
existing packing for valid substreams.
---
Nitpick comments:
In `@src/buf.rs`:
- Around line 145-149: Update Buf::insert_slice to reserve capacity for the
final buffer size before splitting or extending, accounting for both val.len()
and the existing tail. Preserve the current insertion order and unsplit behavior
while ensuring extend_from_slice and unsplit do not cause separate
reallocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97fc2c5f-88f4-4529-a5e2-f98836fa95a2
📒 Files selected for processing (20)
src/any.rssrc/atom.rssrc/buf.rssrc/error.rssrc/header.rssrc/meta/properties/irot.rssrc/mfra/mod.rssrc/mfra/tfra.rssrc/moov/trak/mdia/mdhd.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.rssrc/moov/trak/mdia/minf/stbl/stsd/colr.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avcc.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/subs.rssrc/prft.rssrc/tokio/header.rssrc/types.rs
| 0 => (value as u8).encode(buf), | ||
| 1 => (value as u16).encode(buf), | ||
| 2 => { | ||
| let v: u24 = value.try_into().expect("should have already been checked"); | ||
| let v: u24 = value.try_into()?; | ||
| v.encode(buf) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use fallible conversions for all integer widths.
The as u8 and as u16 casts silently truncate value if it exceeds their respective maximums, which goes against the PR's objective of preventing silent truncation. Since you already converted the u24 case to use a fallible conversion, apply the same protection to the u8 and u16 cases.
🛡️ Proposed protection
match num_bits_minus_one {
- 0 => (value as u8).encode(buf),
- 1 => (value as u16).encode(buf),
+ 0 => u8::try_from(value).map_err(|_| Error::OutOfRange)?.encode(buf),
+ 1 => u16::try_from(value).map_err(|_| Error::OutOfRange)?.encode(buf),
2 => {
let v: u24 = value.try_into()?;
v.encode(buf)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 0 => (value as u8).encode(buf), | |
| 1 => (value as u16).encode(buf), | |
| 2 => { | |
| let v: u24 = value.try_into().expect("should have already been checked"); | |
| let v: u24 = value.try_into()?; | |
| v.encode(buf) | |
| } | |
| 0 => u8::try_from(value).map_err(|_| Error::OutOfRange)?.encode(buf), | |
| 1 => u16::try_from(value).map_err(|_| Error::OutOfRange)?.encode(buf), | |
| 2 => { | |
| let v: u24 = value.try_into()?; | |
| v.encode(buf) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mfra/tfra.rs` around lines 86 - 91, Update the integer-width encoding
match so the 0 and 1 branches use fallible conversions from value to u8 and u16,
respectively, matching the existing try_into behavior in the u24 branch and
propagating conversion errors instead of silently truncating.
| let length: u24 = (data.len() as u32) | ||
| .try_into() | ||
| .map_err(|_| Error::TooLarge(Dfla::KIND))?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Prevent silent truncation when determining payload length.
Casting data.len() from usize to u32 via as u32 silently truncates lengths that exceed u32::MAX (e.g., on 64-bit platforms). For example, a slice of 4GB+1 byte would truncate to 1, which successfully passes the subsequent .try_into() bounds check for u24. This results in writing a length of 1 but emitting 4GB of payload, corrupting the bitstream.
To align with the PR's objective of preventing silent truncation, use u32::try_from() to strictly bounds-check the usize length before checking it against u24. Based on learnings, operations evaluating payload lengths should fail explicitly with an error instead of silently truncating.
🐛 Proposed fix
- let length: u24 = (data.len() as u32)
+ let length: u24 = u32::try_from(data.len())
+ .map_err(|_| Error::TooLarge(Dfla::KIND))?
.try_into()
.map_err(|_| Error::TooLarge(Dfla::KIND))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let length: u24 = (data.len() as u32) | |
| .try_into() | |
| .map_err(|_| Error::TooLarge(Dfla::KIND))?; | |
| let length: u24 = u32::try_from(data.len()) | |
| .map_err(|_| Error::TooLarge(Dfla::KIND))? | |
| .try_into() | |
| .map_err(|_| Error::TooLarge(Dfla::KIND))?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/moov/trak/mdia/minf/stbl/stsd/flac.rs` around lines 87 - 89, Replace the
`as u32` cast in the payload-length calculation with a checked
`u32::try_from(data.len())`, mapping conversion failure to
`Error::TooLarge(Dfla::KIND)` before the existing `u24` conversion. Preserve the
subsequent `u24` bounds validation so lengths exceeding either limit fail
explicitly.
Source: Learnings
|
@bradh lemme know if you want this split up into a bunch of PRs... |
|
Would probably be easier (aka sooner) to review, but only if your agent can do it without much fuss. |
2 participants
Addresses the low-severity, verified correctness items tracked in #197 (overflow/underflow edges, flag-fidelity round-trips, encode truncations).
Per the issue author's guidance, this prefers spec-correctness over back-compat: some previously-"successful" but malformed encodes now return
Errinstead of silently truncating. This is a0.xcrate.Core / types
u24/u48TryFromnow reject out-of-range values (newError::OutOfRange) instead of silently truncating viato_be_bytes()[1..]; addsu48::MAX.FixedPoint'sDebugprints the true fixed-point value (int + dec/2^bits) rather thanint / dec.Header::encodeuses checked arithmetic (no overflow panic/wrap) and a 32-bit-safe largesize branch; decode paths (sync,ReadFrom, tokio) useusize::try_fromso a 64-bit largesize no longer truncates on 32-bit targets.largesizeform instead ofError::TooLarge(the existing TODO). This adds a newBufMut::insert_slicemethod (implemented forVec,&mut T,BytesMut) plus a sharedwrite_box_sizehelper — a breaking change for externalBufMutimplementors.Mdhdgets a manualDefaultwithlanguage: "und"sodecode(encode(default)) == default(an empty string isn't representable in the packed 15-bit form).Codecs
profile_idc(the ISO 14496-15 High-profile set, incl.244) instead of leftover bytes, so trailing padding on a Baseline record no longer misparses as the chroma/bit-depth extension;Avcc::newemits the ext for High profiles; SPS (≤31) / PPS / NAL count & length overflow checks.initial_presentation_delay - 1underflow and adds range checks (seq_profile,seq_level_idx_0,chroma_sample_position).full_range_flagdecoded asbyte & 0x80 != 0(was== 0x80).Vec<u8>).data_rateand empty/oversized substream lists.Errinstead of panicking on a missingcomponent_type_uri.codec_specific_parameterslength enforced to 4.moov / moof
real_timewithoutconsistent_offsetnow errors instead of misparsing as an unrelatedReferenceTime.angle & 0x03on encode to match decode.Mfro.parent_sizerecomputed on encode;tfradrops the.expect()in favor of?.Not included / deviations
144; the real value is244(144isn't a profile). Both are included in the gate set for safety;244is what theavcc_ext_2fixture actually uses.Hmhd(hint media header) — already present onmain, so no change here.String::decodemissing-NUL — left lenient: many end-of-box string fields (hdlrname,urllocation,auxc,cprt) legitimately omit the trailing NUL in real files; strict decode would reject valid input, and re-encode already appends the NUL (box sizes recomputed, so benign).FullBoxfor reserved bits that are ~always 0).minf"exactly one media header" — not enforced (risks rejecting real files; left as a separate decision).Testing
cargo test --all-features(240 pass, incl. new tests for the largesize header format, FLAC padding round-trip, andMdhd::defaultround-trip),cargo clippy --all-targets --all-features -- -D warnings, andcargo fmt --checkall clean.Closes #197
🤖 Generated with Claude Code