diff --git a/src/mux/codec/h264.rs b/src/mux/codec/h264.rs index ebcf7bb..6da1625 100644 --- a/src/mux/codec/h264.rs +++ b/src/mux/codec/h264.rs @@ -112,6 +112,26 @@ impl H264Parser { } } +// Bytes reserved at the front of every assembled access unit so the keyframe +// parameter-set re-assert can be spliced in without reallocating. An SPS + PPS +// re-assert is a couple hundred bytes (each a 4-byte length prefix plus a NAL +// that is tens to low hundreds of bytes on real BD/UHD streams); 1 KiB covers it +// with margin, and costs 1 KiB of slack per in-flight frame. If a stream's +// parameter sets ever exceed this the splice still produces correct output — it +// just reallocates once, exactly as it always did. Mirrors the HEVC parser. +const PARAM_REASSERT_HEADROOM: usize = 1024; + +// Per-thread count of keyframe re-asserts that had to reallocate the frame +// buffer. Test-only instrumentation: the whole point of +// `PARAM_REASSERT_HEADROOM` is that the splice is in-place, so that is MEASURED +// rather than reasoned about. See +// `keyframe_param_reassert_does_not_reallocate_the_frame`. Mirrors the HEVC +// parser. +#[cfg(test)] +thread_local! { + static PARAM_REASSERT_REALLOCS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + /// Append `nal` to `out` as a 4-byte big-endian length prefix + body. A NAL /// longer than `u32::MAX` can't be length-prefixed in the 4-byte field, so it /// is skipped rather than mis-framed. Unreachable in practice (no AU > 4 GiB). @@ -207,7 +227,11 @@ impl CodecParser for H264Parser { // Pre-size: output is ~input bytes plus a few 4-byte NAL length prefixes. // The unsized Vec growth chain otherwise reallocs several times per // frame in the mux hot path (mirrors the HEVC parser). - let mut frame_data = Vec::with_capacity(pes.data.len() + 64); + // + // Plus `PARAM_REASSERT_HEADROOM` so the keyframe parameter-set re-assert + // below can be spliced in FRONT of the frame without reallocating. See + // that site. + let mut frame_data = Vec::with_capacity(pes.data.len() + 64 + PARAM_REASSERT_HEADROOM); // MVC dependent-view passthrough: keep ALL param sets in-band (the frame // is a self-contained BlockAdditional access unit), never strip/re-assert. @@ -274,12 +298,36 @@ impl CodecParser for H264Parser { // that dropped the set at a reset recovers, and a stale avcC re-apply // can't revert it. Skipped per-type only when this AU already carried it. if keyframe && !mvc { - let mut prefix = Vec::new(); + let mut prefix = Vec::with_capacity(PARAM_REASSERT_HEADROOM); reassert_active(&mut prefix, &self.cur_sps, emitted_sps); reassert_active(&mut prefix, &self.cur_pps, emitted_pps); if !prefix.is_empty() { - prefix.extend_from_slice(&frame_data); - frame_data = prefix; + // SPLICE the couple hundred prefix bytes into the front of the + // already-assembled frame, in place. + // + // This used to be `prefix.extend_from_slice(&frame_data)` followed + // by `frame_data = prefix`: that grew `prefix` from a couple hundred + // bytes to the FULL access-unit size (a fresh multi-hundred-KB + // allocation), memcpy'd the whole frame into it, and dropped the + // presized `frame_data` buffer — one extra whole-frame allocation + // plus one extra whole-frame copy per keyframe. A UHD title is + // ~200,000 frames of 150-400 KB with a keyframe every 1-2 s, so + // that is thousands of avoidable multi-hundred-KB copies per title, + // each large enough to go through mmap. + // + // `frame_data` was reserved with `PARAM_REASSERT_HEADROOM` to spare + // precisely so this splice fits without reallocating; what remains + // is one in-place memmove inside the existing buffer. Byte-identical + // output either way — pinned by + // `keyframe_param_reassert_emits_exact_bytes`. Mirrors the HEVC + // parser's fix so the two stay consistent. + #[cfg(test)] + let cap_before = frame_data.capacity(); + frame_data.splice(0..0, prefix); + #[cfg(test)] + if frame_data.capacity() != cap_before { + PARAM_REASSERT_REALLOCS.with(|c| c.set(c.get() + 1)); + } } } @@ -599,6 +647,123 @@ mod tests { } } + // --- keyframe parameter-set re-assert: exact bytes + no whole-frame copy --- + + /// The keyframe SPS/PPS re-assert must produce EXACTLY these bytes: the + /// active SPS, then the active PPS, then the access unit's own NALs, each as + /// a 4-byte big-endian length prefix followed by the NAL body (ISO/IEC + /// 14496-15 length-prefixed form, lengthSizeMinusOne = 3). + /// + /// This pins the full byte string, not just its length, so the in-place + /// `splice` that replaced the build-a-new-buffer-and-copy approach is proven + /// byte-for-byte equivalent — including the ORDER (parameter sets ahead of + /// the slices, which is what makes the keyframe self-contained). The literals + /// below were captured from the pre-splice implementation's output. + #[test] + fn keyframe_param_reassert_emits_exact_bytes() { + fn annexb(nal: &[u8]) -> Vec { + let mut v = vec![0x00, 0x00, 0x01]; + v.extend_from_slice(nal); + v + } + const SPS: [u8; 5] = [0x67, 0x42, 0x00, 0x1E, 0xAB]; + const PPS: [u8; 3] = [0x68, 0xCE, 0x01]; + const IDR1: [u8; 4] = [0x65, 0x88, 0x84, 0x21]; + const IDR2: [u8; 4] = [0x65, 0x88, 0x11, 0x22]; + + let mut parser = H264Parser::new(); + + // AU1: SPS + PPS + IDR. Both parameter sets are first-of-type, so they + // seed avcC and are stripped from the in-band scan output; the keyframe + // re-assert then splices them back in ahead of the slice. + let au1 = [annexb(&SPS), annexb(&PPS), annexb(&IDR1)].concat(); + let f1 = parser.parse(&make_pes(au1, Some(0))); + assert_eq!( + f1[0].data, + vec![ + 0x00, 0x00, 0x00, 0x05, 0x67, 0x42, 0x00, 0x1E, 0xAB, // SPS + 0x00, 0x00, 0x00, 0x03, 0x68, 0xCE, 0x01, // PPS + 0x00, 0x00, 0x00, 0x04, 0x65, 0x88, 0x84, 0x21, // IDR slice + ], + "keyframe must emit length-prefixed SPS, PPS, then the slice" + ); + + // AU2: BARE keyframe — the source omits the parameter sets. The active + // set must be re-asserted ahead of the slice, byte-identically. + let f2 = parser.parse(&make_pes(annexb(&IDR2), Some(3600))); + assert_eq!( + f2[0].data, + vec![ + 0x00, 0x00, 0x00, 0x05, 0x67, 0x42, 0x00, 0x1E, 0xAB, // SPS + 0x00, 0x00, 0x00, 0x03, 0x68, 0xCE, 0x01, // PPS + 0x00, 0x00, 0x00, 0x04, 0x65, 0x88, 0x11, 0x22, // IDR slice + ], + "bare keyframe must re-assert the active SPS/PPS ahead of the slice" + ); + } + + /// MEASURED: the keyframe parameter-set re-assert must be spliced into the + /// front of the already-assembled access unit IN PLACE, not built as a fresh + /// full-size buffer. + /// + /// It used to `prefix.extend_from_slice(&frame_data)` and then replace + /// `frame_data` with `prefix`, which grew a few-hundred-byte `prefix` to the + /// FULL access-unit size — a fresh multi-hundred-KB allocation — memcpy'd the + /// whole frame into it, and dropped the presized buffer. One extra + /// whole-frame allocation plus one extra whole-frame copy per keyframe: a UHD + /// title is ~200,000 frames of 150-400 KB with a keyframe every 1-2 s, i.e. + /// thousands of avoidable multi-hundred-KB copies per title, each large + /// enough to go through mmap. `PARAM_REASSERT_HEADROOM` exists so the splice + /// never reallocates; this counts the reallocations that happen, which must + /// be zero. Mirrors the HEVC parser's test of the same name. + #[test] + fn keyframe_param_reassert_does_not_reallocate_the_frame() { + fn annexb(nal_header: u8, body: &[u8]) -> Vec { + let mut v = vec![0x00, 0x00, 0x01, nal_header]; + v.extend_from_slice(body); + v + } + let mut parser = H264Parser::new(); + // AU1 seeds the active SPS/PPS. The SPS is deliberately ~200 bytes rather + // than a handful: `frame_data` is presized to `pes.data.len() + 64 + + // PARAM_REASSERT_HEADROOM` and a bare keyframe uses slightly less than + // `pes.data.len()`, so a tiny parameter set fits in the incidental `+64` + // slack and the test would pass with `PARAM_REASSERT_HEADROOM` set to + // zero — proving only that this fixture does not realloc, not that the + // headroom is what prevents it. At ~200 bytes the prefix exceeds the + // incidental slack, so the constant is load-bearing and zeroing it makes + // this test fail. + let mut big_sps = vec![0x42, 0x00, 0x1E]; + big_sps.extend(std::iter::repeat_n(0xAB, 200)); + let au1 = [ + annexb(0x67, &big_sps), + annexb(0x68, &[0xCE, 0x01]), + annexb(0x65, &[0x88; 4096]), + ] + .concat(); + parser.parse(&make_pes(au1, Some(0))); + + // A run of BARE keyframes (source omits the parameter sets), each of + // which takes the re-assert path. Payload sized like a real coded + // picture so a reallocation would be the expensive one. + PARAM_REASSERT_REALLOCS.with(|c| c.set(0)); + for i in 0..30i64 { + let au = annexb(0x65, &vec![0x88u8; 300_000]); + let f = parser.parse(&make_pes(au, Some(3600 * (i + 1)))); + // The re-assert really happened (otherwise the count is vacuously 0). + assert!( + f[0].data.len() > 300_000, + "keyframe {i} must carry the re-asserted parameter sets" + ); + } + let reallocs = PARAM_REASSERT_REALLOCS.with(|c| c.get()); + assert_eq!( + reallocs, 0, + "the parameter-set splice must fit in the reserved headroom; \ + {reallocs} of 30 keyframes reallocated the whole frame" + ); + } + // --- parse SPS+PPS → codec_private --- #[test] diff --git a/src/scsi/linux.rs b/src/scsi/linux.rs index 7a3c871..bfecbf8 100644 --- a/src/scsi/linux.rs +++ b/src/scsi/linux.rs @@ -28,6 +28,11 @@ const SG_DXFER_TO_DEV: i32 = -2; const SG_DXFER_FROM_DEV: i32 = -3; const SG_FLAG_Q_AT_HEAD: u32 = 0x10; +/// Width of `sg_io_hdr.cmdp` as far as SG_IO is concerned: `cmd_len` is a +/// single byte and every SPC-4/MMC command this crate issues is 6, 10, 12 or +/// 16 bytes. Matches `K_MAX_CDB_SIZE` in the macOS and Windows backends. +const K_MAX_CDB_SIZE: usize = 16; + #[repr(C)] #[allow(non_camel_case_types)] struct sg_io_hdr { @@ -289,7 +294,13 @@ impl ScsiTransport for SgIoTransport { DataDirection::FromDevice => SG_DXFER_FROM_DEV, DataDirection::ToDevice => SG_DXFER_TO_DEV, }; - let cmd_len = cdb.len().min(16) as u8; + // Reject an over-length CDB rather than truncating it. This used to be + // `cdb.len().min(16) as u8`, which silently dropped the tail: SPC-4 + // fixes a command's length by its opcode group code, so the shortened + // CDB is a DIFFERENT command, which the drive executes and answers + // with GOOD status and data for a request nobody made. Matches the + // macOS and Windows backends (all three call the same helper). + let cmd_len = super::checked_cdb_len(cdb, K_MAX_CDB_SIZE)?; let mut sense = [0u8; 32]; let mut hdr: sg_io_hdr = unsafe { std::mem::zeroed() }; diff --git a/src/scsi/macos.rs b/src/scsi/macos.rs index b416285..d776ec7 100644 --- a/src/scsi/macos.rs +++ b/src/scsi/macos.rs @@ -161,13 +161,11 @@ impl ScsiTransport for MacScsiTransport { let mut task_status: u8 = 0xFF; let mut transfer_count: u64 = 0; - if cdb.len() > K_MAX_CDB_SIZE { - return Err(Error::InvalidCdbLength { - len: cdb.len(), - max: K_MAX_CDB_SIZE, - }); - } - let cdb_len = cdb.len() as u8; + // Reject an over-length CDB rather than truncating it. The guard now + // lives in `scsi::checked_cdb_len`, shared with the Linux and Windows + // backends (which used to truncate here instead of erroring) so it + // cannot drift per platform again. + let cdb_len = super::checked_cdb_len(cdb, K_MAX_CDB_SIZE)?; let kr = unsafe { shim_execute( cdb.as_ptr(), @@ -266,39 +264,30 @@ mod tests { use crate::error::Error; /// A CDB longer than K_MAX_CDB_SIZE must be rejected with - /// `Error::InvalidCdbLength` before the shim is ever called. - /// This test exercises the length guard portably — it calls the - /// guard logic directly without opening an IOKit handle. + /// `Error::InvalidCdbLength` before the shim is ever called. Exercises the + /// real guard `MacScsiTransport::execute` uses, without opening an IOKit + /// handle. #[test] fn oversized_cdb_returns_invalid_cdb_length() { // Build a CDB one byte over the limit. let long_cdb = [0u8; K_MAX_CDB_SIZE + 1]; - // Replicate the guard logic from MacScsiTransport::execute so - // this test runs on Linux CI as well (no IOKit present there). - let result: Result<(), Error> = if long_cdb.len() > K_MAX_CDB_SIZE { - Err(Error::InvalidCdbLength { - len: long_cdb.len(), - max: K_MAX_CDB_SIZE, - }) - } else { - Ok(()) - }; - match result { + match crate::scsi::checked_cdb_len(&long_cdb, K_MAX_CDB_SIZE) { Err(Error::InvalidCdbLength { len, max }) => { assert_eq!(len, K_MAX_CDB_SIZE + 1); assert_eq!(max, K_MAX_CDB_SIZE); } - other => panic!("expected InvalidCdbLength, got {:?}", other), + other => panic!("expected InvalidCdbLength, got {other:?}"), } } - /// A CDB exactly at the limit must not trigger the guard. + /// A CDB exactly at the limit must not trigger the guard, and its length + /// reaches the shim verbatim. #[test] fn max_length_cdb_does_not_trigger_guard() { let cdb = [0u8; K_MAX_CDB_SIZE]; - let triggered = cdb.len() > K_MAX_CDB_SIZE; - assert!( - !triggered, + assert_eq!( + crate::scsi::checked_cdb_len(&cdb, K_MAX_CDB_SIZE).ok(), + Some(K_MAX_CDB_SIZE as u8), "CDB of exactly K_MAX_CDB_SIZE should not trigger guard" ); } diff --git a/src/scsi/mod.rs b/src/scsi/mod.rs index c0d05a3..d6a6cea 100644 --- a/src/scsi/mod.rs +++ b/src/scsi/mod.rs @@ -101,6 +101,85 @@ pub const SCSI_STATUS_CHECK_CONDITION: u8 = 0x02; /// [`Error::ScsiError`] with `sense = None`. pub const SCSI_STATUS_TRANSPORT_FAILURE: u8 = 0xFF; +// ── CDB length validation ────────────────────────────────────────────────── + +/// Validate a CDB against a transport's CDB field width, returning the length +/// as the `u8` every backend's pass-through descriptor wants. +/// +/// A CDB longer than the field must be REJECTED, never truncated. Under SPC-4 +/// the opcode's group code (bits 7-5 of byte 0) fixes the CDB length, so +/// dropping the tail bytes does not produce a shorter form of the same +/// command — it produces a DIFFERENT command with a different meaning for the +/// bytes that remain. The drive will usually execute it and return GOOD +/// status with data for a request the caller never made: a silently wrong +/// result, on the transport layer everything else in the crate sits on. +/// +/// Lives here, shared by all three platform backends, so the guard cannot +/// drift per platform (it previously truncated on Linux and Windows while +/// erroring on macOS — the "works on my platform, not theirs" class). +pub(crate) fn checked_cdb_len(cdb: &[u8], max: usize) -> Result { + if cdb.len() > max { + return Err(Error::InvalidCdbLength { + len: cdb.len(), + max, + }); + } + // `max` is 16 on every backend, so the cast cannot truncate. + Ok(cdb.len() as u8) +} + +#[cfg(test)] +mod cdb_len_tests { + use super::*; + + /// The maximum every backend declares (`K_MAX_CDB_SIZE`). + const MAX: usize = 16; + + /// A CDB one byte over the transport's field width must be REJECTED with + /// [`Error::InvalidCdbLength`], never silently shortened to `max`. This is + /// the cross-platform test for the guard that `linux.rs`, `macos.rs` and + /// `windows.rs` all route through — none of those three modules is + /// compiled on more than one host, so the shared helper is the only place + /// the behaviour can be tested on every platform's CI. + #[test] + fn oversized_cdb_is_rejected_not_truncated() { + let cdb = [0u8; MAX + 1]; + match checked_cdb_len(&cdb, MAX) { + Err(Error::InvalidCdbLength { len, max }) => { + assert_eq!(len, MAX + 1); + assert_eq!(max, MAX); + } + Err(other) => panic!("expected InvalidCdbLength, got {other:?}"), + Ok(n) => panic!( + "over-length CDB was accepted and truncated to {n} bytes — the drive \ + would execute a DIFFERENT command than the caller asked for" + ), + } + } + + /// A CDB exactly at the field width is legal and passes through untouched. + #[test] + fn max_length_cdb_is_accepted() { + let cdb = [0u8; MAX]; + assert_eq!(checked_cdb_len(&cdb, MAX).ok(), Some(MAX as u8)); + } + + /// Every real CDB length (SPC-4 groups 0-5: 6, 10, 12, 16 bytes) is + /// accepted and reported verbatim, and an empty CDB reports 0 — the + /// backends' own empty-CDB guards handle that case. + #[test] + fn in_range_cdb_lengths_pass_through_verbatim() { + for len in [0usize, 6, 10, 12, 16] { + let cdb = vec![0u8; len]; + assert_eq!( + checked_cdb_len(&cdb, MAX).ok(), + Some(len as u8), + "CDB of {len} bytes must be accepted verbatim" + ); + } + } +} + // ── SPC-4 sense keys (§4.5.6 Table 28) ───────────────────────────────────── // // Broad failure category returned in a CHECK CONDITION reply's sense data. diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs index 8b66428..b910874 100644 --- a/src/scsi/windows.rs +++ b/src/scsi/windows.rs @@ -436,7 +436,14 @@ impl ScsiTransport for SptiTransport { let mut sptwb: SptwbDirect = unsafe { std::mem::zeroed() }; - let cdb_len = cdb.len().min(K_MAX_CDB_SIZE); + // Reject an over-length CDB rather than truncating it. This used to be + // `cdb.len().min(K_MAX_CDB_SIZE)`, and the copy into `sptwb.spt.Cdb` + // below then took only the first 16 bytes: SPC-4 fixes a command's + // length by its opcode group code, so the shortened CDB is a DIFFERENT + // command, which the drive executes and answers with GOOD status and + // data for a request nobody made. Matches the Linux and macOS backends + // (all three call the same helper). + let cdb_len = usize::from(super::checked_cdb_len(cdb, K_MAX_CDB_SIZE)?); sptwb.spt.Length = std::mem::size_of::() as u16; sptwb.spt.CdbLength = cdb_len as u8; sptwb.spt.SenseInfoLength = K_SENSE_SIZE as u8;