Reject an over-length CDB on every transport, splice H.264 param sets in place
Two fixes from round 5. The Linux and Windows backends truncated a CDB longer than 16 bytes (`cdb.len().min(16)`) where macOS returned InvalidCdbLength. Under SPC-4 a command's length is fixed by its opcode group code, so a shortened CDB is not a shorter form of the same command — it is a DIFFERENT command, and the drive executes it and answers GOOD with data for a request nobody made. A silently wrong result on the layer everything else sits on. Rather than mirror the guard a third time it now lives in scsi::mod as checked_cdb_len, with all three backends routed through it, so it cannot drift per platform again. That also makes it testable everywhere: each platform module is cfg-gated to its own host, so a guard inlined into linux.rs and windows.rs would have had no test coverage on any single machine. The shared helper is the only place the behaviour can be asserted on every platform's CI. The two existing macOS tests were tautological — they replicated the guard's logic inline instead of calling it, so they would have passed with the guard deleted. They now call the real helper. Separately, the H.264 keyframe parameter-set re-assert grew a few-hundred-byte prefix buffer to the full access-unit size, copied the whole frame into it, and dropped the presized buffer: one extra whole-frame allocation and copy per keyframe. A UHD title is ~200,000 frames of 150-400 KB with a keyframe every second or two, so that is thousands of avoidable multi-hundred-KB copies per title, each large enough to go through mmap. It now splices into the reserved headroom in place. This mirrors the identical fix already made in hevc.rs, which the H.264 path had drifted from. Byte-for-byte equivalence is pinned by a test whose expected literals were captured from the pre-change implementation, and which I confirmed still passes when the old build-and-copy code is restored. The no-reallocation claim is measured rather than argued: a counter over 30 bare keyframes, which reports 30 of 30 against the old path and 0 with the splice. The reallocation test initially passed even with PARAM_REASSERT_HEADROOM set to zero, because a small parameter set fits in the presize's incidental slack — it proved the fixture did not reallocate, not that the headroom prevented it. Its SPS is now large enough that the constant is load-bearing, so zeroing it fails the test. Not verified: no runtime behaviour on Linux or Windows: no drive, no ioctl. Both files were confirmed to compile for their own targets.
This commit is contained in:
@@ -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<u8> {
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user