diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a63591e..7c3f503 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,4 +39,9 @@ jobs: steps: - uses: actions/checkout@v5 - uses: dtolnay/rust-toolchain@1.86.0 - - run: cargo check + # Build the tests (not just `cargo check`): catches errors in test + # code and forces full codegen of the Windows-only SPTI transport + # (src/scsi/windows.rs), which never compiles on the Linux/macOS dev + # hosts. We don't `cargo test` here — the suite needs no drive but the + # extra build is the value; running tests is covered by the Linux job. + - run: cargo build --tests diff --git a/Cargo.toml b/Cargo.toml index 911f0b9..a0a9103 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "libfreemkv" -version = "1.0.0-rc.3" +version = "1.0.0-rc.3.1" edition = "2024" rust-version = "1.86" license = "AGPL-3.0-only" diff --git a/src/css/mod.rs b/src/css/mod.rs index 19e3fa8..78aaf80 100644 --- a/src/css/mod.rs +++ b/src/css/mod.rs @@ -56,6 +56,58 @@ pub fn crack_key( crack_key_halt(reader, extents, batch_sectors, None) } +/// Outcome of a CSS crack scan that distinguishes the THREE cases the bare +/// `Option` conflated (and which caused a silent-failure bug: +/// scrambled-but-uncracked content was treated as "unencrypted" and muxed as +/// plaintext garbage at exit 0): +/// +/// - [`CrackOutcome::Cracked`] — a scrambled sector yielded a title key. +/// - [`CrackOutcome::Unencrypted`] — NO scrambled sector was seen across the +/// scanned extents (`is_scrambled` never true): the content is genuinely +/// plaintext, so proceeding without a key is correct. +/// - [`CrackOutcome::ScrambledUncracked`] — scrambled sectors WERE seen but no +/// key could be recovered (the Stevenson attack found no crackable crib, or +/// the scrambled region was unreadable). The content is encrypted; muxing it +/// as plaintext would emit garbage, so callers MUST surface a hard error +/// ([`crate::error::Error::CssKeyMissing`]) instead of falling through to +/// "unencrypted". +#[derive(Debug, Clone)] +pub enum CrackOutcome { + Cracked(CssState), + Unencrypted, + ScrambledUncracked, +} + +impl CrackOutcome { + /// The cracked `CssState`, if any. `None` for `Unencrypted` / + /// `ScrambledUncracked`. Lets the `Option`-returning wrappers stay thin. + pub fn into_state(self) -> Option { + match self { + CrackOutcome::Cracked(s) => Some(s), + _ => None, + } + } + + /// True when scrambled sectors were seen but no key was recovered — the + /// case callers must surface as a hard error instead of "unencrypted". + pub fn is_scrambled_uncracked(&self) -> bool { + matches!(self, CrackOutcome::ScrambledUncracked) + } +} + +/// [`crack_key`] returning the full [`CrackOutcome`] (Cracked / Unencrypted / +/// ScrambledUncracked) so callers can distinguish "genuinely unencrypted" from +/// "encrypted but uncrackable" — the latter must become a hard error, never a +/// silent fall-through to plaintext. +pub fn crack_key_outcome( + reader: &mut dyn SectorSource, + extents: &[Extent], + batch_sectors: u16, + halt: Option<&crate::halt::Halt>, +) -> CrackOutcome { + crack_key_scan(reader, extents, batch_sectors, halt) +} + /// [`crack_key`] with an optional cooperative-cancellation token. /// /// "No silent hangs": the crack scans up to 50_000 sectors, which on a live @@ -70,6 +122,19 @@ pub fn crack_key_halt( batch_sectors: u16, halt: Option<&crate::halt::Halt>, ) -> Option { + crack_key_scan(reader, extents, batch_sectors, halt).into_state() +} + +/// The crack scan, returning the full [`CrackOutcome`]. Tracks a +/// `saw_scrambled` flag so a scrambled-but-uncracked disc is distinguished +/// from a genuinely-unencrypted one (the [`crack_key`] / [`crack_key_halt`] +/// `Option` wrappers collapse both to `None`). +fn crack_key_scan( + reader: &mut dyn SectorSource, + extents: &[Extent], + batch_sectors: u16, + halt: Option<&crate::halt::Halt>, +) -> CrackOutcome { // Batch the reads: a live optical drive at 1 sector/read is glacial, and the // crack only needs to FIND one scrambled sector whose 0x80 plaintext matches // a known PES header. `batch_sectors` MUST be sized to the source — a drive @@ -90,6 +155,12 @@ pub fn crack_key_halt( let max_tries = 50_000u32; let mut buf = vec![0u8; batch as usize * 2048]; let mut hb = crate::progress::Heartbeat::new("css_crack"); + // Track whether ANY scrambled sector was observed. If we exhaust the scan + // budget having seen scrambled data but never recovered a key, the content + // is encrypted-but-uncrackable — a HARD failure the caller must surface, + // NOT silently treat as unencrypted (which would mux scrambled MPEG as + // plaintext → garbage at exit 0). See `CrackOutcome::ScrambledUncracked`. + let mut saw_scrambled = false; 'outer: for (extent_idx, ext) in extents.iter().enumerate() { let mut i = 0u32; @@ -122,8 +193,9 @@ pub fn crack_key_halt( tried += 1; let sect = &buf[s * 2048..(s + 1) * 2048]; if is_scrambled(sect) { + saw_scrambled = true; if let Some(key) = stevenson::crack_title_key(sect) { - return Some(CssState { + return CrackOutcome::Cracked(CssState { title_key: key, crack_span, }); @@ -142,7 +214,16 @@ pub fn crack_key_halt( } } - None + // Budget exhausted / extents walked with no key recovered. Distinguish the + // two indistinguishable-in-`Option` cases: if scrambled sectors were seen + // (case b: crack failed; case c: scrambled but the crackable region was + // unreadable), this is encrypted-but-uncracked — a hard failure. Only a + // scan that NEVER saw a scrambled sector is genuinely unencrypted (case a). + if saw_scrambled { + CrackOutcome::ScrambledUncracked + } else { + CrackOutcome::Unencrypted + } } /// Descramble a single CSS-encrypted sector in place. @@ -290,6 +371,68 @@ mod tests { ); } + // ── CrackOutcome: scrambled-but-uncracked vs genuinely unencrypted (Fix 6) ─ + + /// A scan over CLEAR sectors (scramble flag never set) returns + /// `Unencrypted` — the content is genuinely plaintext, so proceeding + /// without a key is correct. + #[test] + fn crack_outcome_clear_sectors_is_unencrypted() { + let mut src = MockSource::new(0x00); // never scrambled + let extents = [Extent { + start_lba: 0, + sector_count: 100, + }]; + let outcome = crack_key_outcome(&mut src, &extents, 1, None); + assert!( + matches!(outcome, CrackOutcome::Unencrypted), + "no scrambled sector seen → Unencrypted, got {outcome:?}" + ); + // The Option wrapper collapses Unencrypted → None. + assert!(crack_key(&mut MockSource::new(0x00), &extents, 1).is_none()); + } + + /// THE Fix 6 regression: a scan that SEES scrambled sectors (flag set) but + /// recovers no key (the mock's zeroed data has no Stevenson crib) must + /// return `ScrambledUncracked` — a HARD failure — NOT `Unencrypted`. The + /// old code conflated this with "unencrypted" and muxed scrambled MPEG as + /// plaintext (garbage at exit 0). + #[test] + fn crack_outcome_scrambled_uncracked_is_hard_failure() { + let mut src = MockSource::new(0x30); // scrambled flag set, no crackable crib + let extents = [Extent { + start_lba: 0, + sector_count: 100, + }]; + let outcome = crack_key_outcome(&mut src, &extents, 1, None); + assert!( + outcome.is_scrambled_uncracked(), + "scrambled sectors seen but no key → ScrambledUncracked, got {outcome:?}" + ); + // The legacy Option wrapper still collapses this to None (the callers + // that need the distinction now use crack_key_outcome instead). + assert!(crack_key(&mut MockSource::new(0x30), &extents, 1).is_none()); + } + + /// Even when every read FAILS, a scan that never managed to observe a + /// scrambled sector reports `Unencrypted` (we cannot prove encryption from + /// unreadable data alone — the AACS/keydb paths and the disc-level + /// `css_error` plumbing cover genuinely unreadable encrypted discs). + #[test] + fn crack_outcome_all_reads_fail_is_unencrypted() { + let mut src = MockSource::new(0x30); + src.fail_all = true; // no sector is ever inspected + let extents = [Extent { + start_lba: 0, + sector_count: 10, + }]; + let outcome = crack_key_outcome(&mut src, &extents, 1, None); + assert!( + matches!(outcome, CrackOutcome::Unencrypted), + "no readable scrambled sector → Unencrypted, got {outcome:?}" + ); + } + /// The budget spans ALL extents, not per-extent: two extents summing past /// the cap must still stop at 50_000 total reads. /// diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 2c0c477..9570a9b 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -60,6 +60,16 @@ pub struct Disc { /// "disc hash not in KEYDB", etc. None when AACS resolution wasn't /// attempted (unencrypted disc) or succeeded. pub aacs_error: Option, + /// CSS crack failure: `Some(Error::CssKeyMissing)` when the scan SAW + /// scrambled sectors but could NOT recover a title key (the + /// known-plaintext attack found no crackable crib, or the scrambled + /// region was unreadable). `css` is `None` in that case — but the disc is + /// genuinely encrypted, so callers MUST surface this hard error rather + /// than treat `css.is_none()` as "unencrypted" and mux scrambled MPEG as + /// plaintext garbage. `None` when no scrambled sector was seen (genuinely + /// unencrypted) or a key was recovered (`css.is_some()`). The CSS analogue + /// of [`Self::aacs_error`]. + pub css_error: Option, /// Content format (BD transport stream vs DVD program stream) pub content_format: ContentFormat, } @@ -1343,7 +1353,7 @@ impl Disc { let crack_batch = detect_max_batch_sectors(session.device_path()); tracing::info!(target: "freemkv::scan", crack_batch, "phase: CSS — known-plaintext crack"); let crack_t0 = std::time::Instant::now(); - let crack_result = crate::css::crack_key_halt( + let crack_result = crate::css::crack_key_outcome( session, &main_extents, crack_batch, @@ -1352,15 +1362,27 @@ impl Disc { tracing::info!( target: "freemkv::scan", elapsed_ms = crack_t0.elapsed().as_millis() as u64, - found = crack_result.is_some(), + outcome = ?crack_result, "phase: CSS — crack done" ); - if let Some(state) = crack_result { - tracing::debug!(target: "freemkv::disc", "dvd css: title key recovered via known-plaintext crack"); - disc.css = Some(state); - disc.encrypted = true; - } else { - tracing::warn!(target: "freemkv::disc", "dvd css: no crackable scrambled sector (unencrypted or atypical layout)"); + match crack_result { + crate::css::CrackOutcome::Cracked(state) => { + tracing::debug!(target: "freemkv::disc", "dvd css: title key recovered via known-plaintext crack"); + disc.css = Some(state); + disc.encrypted = true; + } + crate::css::CrackOutcome::ScrambledUncracked => { + // Scrambled sectors WERE seen but no key could be + // recovered — the content is encrypted-but-uncrackable. + // Record a hard error so callers fail loudly instead of + // muxing scrambled MPEG as plaintext garbage at exit 0. + tracing::warn!(target: "freemkv::disc", "dvd css: scrambled sectors seen but no title key cracked"); + disc.encrypted = true; + disc.css_error = Some(crate::error::Error::CssKeyMissing); + } + crate::css::CrackOutcome::Unencrypted => { + tracing::debug!(target: "freemkv::disc", "dvd css: no scrambled sector seen (genuinely unencrypted)"); + } } } } @@ -1405,10 +1427,21 @@ impl Disc { }; if !main_extents.is_empty() { // Image reads aren't drive-batch-limited; use a generous batch. - if let Some(state) = crate::css::crack_key(reader, &main_extents, 32) { - tracing::info!(target: "freemkv::scan", "image css: title key recovered via known-plaintext crack"); - disc.css = Some(state); - disc.encrypted = true; + match crate::css::crack_key_outcome(reader, &main_extents, 32, None) { + crate::css::CrackOutcome::Cracked(state) => { + tracing::info!(target: "freemkv::scan", "image css: title key recovered via known-plaintext crack"); + disc.css = Some(state); + disc.encrypted = true; + } + crate::css::CrackOutcome::ScrambledUncracked => { + // Scrambled image data with no recoverable key — a hard + // failure, surfaced so the mux path doesn't pass scrambled + // MPEG through as plaintext (garbage at exit 0). + tracing::warn!(target: "freemkv::scan", "image css: scrambled sectors seen but no title key cracked"); + disc.encrypted = true; + disc.css_error = Some(crate::error::Error::CssKeyMissing); + } + crate::css::CrackOutcome::Unencrypted => {} } } } @@ -1599,6 +1632,9 @@ impl Disc { css, encrypted, aacs_error, + // CSS crack runs AFTER scan_with returns (in `scan` / `scan_image`), + // which set this when they observe scrambled-but-uncracked content. + css_error: None, content_format, }) } @@ -3513,6 +3549,7 @@ mod tests { css: None, encrypted: false, aacs_error: None, + css_error: None, content_format: ContentFormat::BdTs, }; let gb = disc.capacity_gb(); @@ -3599,6 +3636,7 @@ mod tests { css: None, encrypted: false, aacs_error: None, + css_error: None, content_format: ContentFormat::BdTs, } } diff --git a/src/error.rs b/src/error.rs index 6684dd3..9904f57 100644 --- a/src/error.rs +++ b/src/error.rs @@ -124,6 +124,11 @@ pub const E_M2TS_PACKET_MALFORMED: u16 = 9021; /// connect to (every resolved IP was loopback / private / link-local / /// multicast / unspecified). Closes the DNS-rebinding SSRF window. pub const E_NETWORK_ADDR_BLOCKED: u16 = 9022; +/// A muxer's `finish()` was called after zero frames were emitted — the +/// output would be a header-only container with no media. Surfaced so a +/// zero-frame mux (undecryptable input, fully-unreadable title, every +/// frame dropped before the first keyframe) cannot report success. +pub const E_MUX_EMPTY: u16 = 9023; pub const E_EXTENT_NOT_UNIT_ALIGNED: u16 = 9030; /// READ CAPACITY returned a short or overflowing transfer. pub const E_DISC_CAPACITY_MALFORMED: u16 = 9047; @@ -368,6 +373,13 @@ pub enum Error { NetworkAddrBlocked { addr: String, }, + /// A muxer's `finish()` was reached after zero frames were written, so + /// the output would be a header-only container with no media. Surfaced + /// (instead of writing a valid-but-empty file and reporting success) so + /// a zero-frame mux — undecryptable input, a fully-unreadable title, or + /// every frame dropped before the first keyframe — fails loudly. The + /// `m2ts://` analogue of [`Error::MkvInvalid`]'s zero-frame guard. + MuxEmpty, PesFrameTooLarge { size: usize, }, @@ -515,6 +527,7 @@ impl Error { Error::StreamUrlMissingPath { .. } => E_STREAM_URL_MISSING_PATH, Error::StreamUrlMissingPort { .. } => E_STREAM_URL_MISSING_PORT, Error::NetworkAddrBlocked { .. } => E_NETWORK_ADDR_BLOCKED, + Error::MuxEmpty => E_MUX_EMPTY, Error::PesFrameTooLarge { .. } => E_PES_FRAME_TOO_LARGE, Error::PesInvalidMagic => E_PES_INVALID_MAGIC, Error::PesTrackTooLarge { .. } => E_PES_TRACK_TOO_LARGE, @@ -739,6 +752,9 @@ impl From for std::io::Error { // 9022 NetworkAddrBlocked: the output host resolved only to // blocked (loopback/private/link-local) addresses — refuse. E_NETWORK_ADDR_BLOCKED => std::io::ErrorKind::PermissionDenied, + // 9023 MuxEmpty: finish() reached with zero frames — the output + // would be a header-only container. Treat as invalid output. + E_MUX_EMPTY => std::io::ErrorKind::InvalidData, // 9030 ExtentNotUnitAligned: a malformed/non-AACS-aligned // extent was handed to the prefetch producer. 9030 => std::io::ErrorKind::InvalidInput, @@ -1109,6 +1125,7 @@ mod tests { E_STREAM_URL_MISSING_PATH, E_STREAM_URL_MISSING_PORT, E_NETWORK_ADDR_BLOCKED, + E_MUX_EMPTY, E_PES_FRAME_TOO_LARGE, E_PES_INVALID_MAGIC, E_PES_TRACK_TOO_LARGE, @@ -1195,6 +1212,7 @@ mod tests { (Error::SweepConsumerGone, E_SWEEP_CONSUMER_GONE), (Error::PipelineConsumerGone, E_PIPELINE_CONSUMER_GONE), (Error::DiscCapacityOverflow, E_DISC_CAPACITY_OVERFLOW), + (Error::MuxEmpty, E_MUX_EMPTY), (Error::M2tsPacketMalformed, E_M2TS_PACKET_MALFORMED), (Error::ExtentNotUnitAligned, E_EXTENT_NOT_UNIT_ALIGNED), (Error::DiscCapacityMalformed, E_DISC_CAPACITY_MALFORMED), diff --git a/src/keydb.rs b/src/keydb.rs index 93ed43f..8d90b03 100644 --- a/src/keydb.rs +++ b/src/keydb.rs @@ -43,15 +43,41 @@ fn read_capped_to_string(reader: R) -> Result { String::from_utf8(buf).map_err(|_| Error::KeydbParse) } -/// Standard keydb storage path. +/// Standard keydb storage path — the canonical location to write the keydb to. +/// +/// On Windows this is the idiomatic per-user roaming dir +/// `%APPDATA%\freemkv\keydb.cfg`, falling back to the legacy +/// `%USERPROFILE%\.config\freemkv\keydb.cfg` only if `APPDATA` is unset. On +/// Linux/macOS it stays the long-standing `$HOME/.config/freemkv/keydb.cfg`. +/// +/// The CLI's read-side search (first existing of several locations) lives in +/// `freemkv-keysources::keydb_search_paths`; this function is the single +/// *write* default used by `save`/`update`, kept in lock-step with that crate's +/// `default_keydb_path` for the same OS. pub fn default_path() -> Result { - let home = std::env::var("HOME") - .or_else(|_| std::env::var("USERPROFILE")) - .map_err(|_| Error::KeydbParse)?; - Ok(PathBuf::from(home) - .join(".config") - .join("freemkv") - .join("keydb.cfg")) + #[cfg(windows)] + { + if let Ok(appdata) = std::env::var("APPDATA") { + if !appdata.is_empty() { + return Ok(PathBuf::from(appdata).join("freemkv").join("keydb.cfg")); + } + } + let profile = std::env::var("USERPROFILE").map_err(|_| Error::KeydbParse)?; + Ok(PathBuf::from(profile) + .join(".config") + .join("freemkv") + .join("keydb.cfg")) + } + #[cfg(not(windows))] + { + let home = std::env::var("HOME") + .or_else(|_| std::env::var("USERPROFILE")) + .map_err(|_| Error::KeydbParse)?; + Ok(PathBuf::from(home) + .join(".config") + .join("freemkv") + .join("keydb.cfg")) + } } /// Download a KEYDB from a URL, verify, save to the standard path. diff --git a/src/mux/resolve.rs b/src/mux/resolve.rs index 579f4f2..34635dc 100644 --- a/src/mux/resolve.rs +++ b/src/mux/resolve.rs @@ -284,6 +284,15 @@ pub fn input(url: &str, opts: &InputOptions) -> io::Result io::Error { e.into() })?; } + // CSS scrambled-but-uncracked guard (Fix 6): the scan saw scrambled + // sectors but recovered no title key, so `disc.css` is None yet the + // content IS encrypted. Without this, `css.is_none()` would be read + // as "unencrypted" and the scrambled MPEG would mux as plaintext + // garbage at exit 0. Surface the recorded hard error instead. + // `--raw` skips decryption, so it is exempt. + if !opts.raw && disc.css_error.is_some() { + return Err(crate::error::Error::CssKeyMissing.into()); + } // No-key guard: if decryption is requested (not --raw) and the disc // is AACS-encrypted but key resolution yielded no usable key, FAIL // here — muxing an undecryptable stream produces ~100 MB of garbage diff --git a/src/mux/tsmux.rs b/src/mux/tsmux.rs index 04a8b9e..e532bde 100644 --- a/src/mux/tsmux.rs +++ b/src/mux/tsmux.rs @@ -46,6 +46,12 @@ pub struct TsMuxer { /// the audio/video offset is preserved. Frames that arrive before it /// is set saturate to 0. base_pts_ns: Option, + /// Count of PES frames actually emitted (a frame dropped as non-key + /// before the first keyframe does NOT count). `finish()` returns + /// [`Error::MuxEmpty`](crate::error::Error::MuxEmpty) when this is zero, + /// so a header-only `m2ts://` output can't be reported as success — + /// mirroring `MkvMuxer.frame_count`. + frame_count: u64, } impl TsMuxer { @@ -58,6 +64,7 @@ impl TsMuxer { codec_privates: vec![None; n], params_written: vec![false; n], base_pts_ns: None, + frame_count: 0, } } @@ -164,6 +171,10 @@ impl TsMuxer { first_pes = false; } } + // A frame that survived the pre-keyframe drop guard above and reached + // the writer counts as emitted. `finish()` checks this so a zero-frame + // mux fails loudly instead of producing a header-only "success". + self.frame_count += 1; Ok(()) } @@ -284,7 +295,17 @@ impl TsMuxer { /// Flush the underlying writer. BD-TS needs no stream trailer, so this /// only drains buffering; the muxer remains usable afterwards. + /// + /// Returns [`Error::MuxEmpty`](crate::error::Error::MuxEmpty) when not a + /// single frame was emitted: an `m2ts://` sink that wrote only the FMKV + /// header (e.g. undecryptable ciphertext yielded no demuxable frames, or + /// every frame was dropped before the first keyframe) would otherwise be a + /// header-only file reported as a successful rip. Mirrors the zero-frame + /// guard in `MkvMuxer::finish`. pub fn finish(&mut self) -> io::Result<()> { + if self.frame_count == 0 { + return Err(crate::error::Error::MuxEmpty.into()); + } self.writer.flush() } } @@ -562,7 +583,12 @@ mod tests { let mut mux = TsMuxer::new(&mut sink, &[VIDEO_PID]); let p = fake_hevc_nal(1, 80); mux.write_frame(0, 0, false, &p).unwrap(); - mux.finish().unwrap(); + // The single non-key frame was dropped (no keyframe to anchor), + // so finish() now reports MuxEmpty rather than producing a + // header-only "success". The drop behaviour itself is still + // verified by the empty packet list below. + let err = mux.finish().unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); } // Nothing should be emitted for that PID. let packets = parse_bd_ts(&sink); @@ -572,6 +598,43 @@ mod tests { ); } + #[test] + fn finish_with_zero_frames_errors_mux_empty() { + // Fix 4: a TsMuxer that never emitted a frame must NOT report a + // clean finish — an m2ts:// sink that wrote only the FMKV header + // (undecryptable ciphertext → no demuxable frames) would otherwise + // be a header-only file published as a successful rip. + let mut sink: Vec = Vec::new(); + let mut mux = TsMuxer::new(&mut sink, &[VIDEO_PID]); + let err = mux.finish().unwrap_err(); + assert_eq!( + err.kind(), + std::io::ErrorKind::InvalidData, + "zero-frame finish must surface MuxEmpty (E9023 → InvalidData)" + ); + // The MuxEmpty variant carries the E9023 code, and its io::Error + // mapping is InvalidData (matching the kind above). Asserting both + // pins the variant ⇄ code ⇄ kind wiring without a lossy round-trip + // (From for io::Error → from-io goes back to IoError/E5000). + assert_eq!( + crate::error::Error::MuxEmpty.code(), + crate::error::E_MUX_EMPTY + ); + let mapped: std::io::Error = crate::error::Error::MuxEmpty.into(); + assert_eq!(mapped.kind(), std::io::ErrorKind::InvalidData); + } + + #[test] + fn finish_after_real_frame_succeeds() { + // The counterpart: once a genuine keyframe is emitted, finish() is Ok. + let mut sink: Vec = Vec::new(); + let mut mux = TsMuxer::new(&mut sink, &[VIDEO_PID]); + let idr = fake_hevc_nal(19, 50); + mux.write_frame(0, 0, true, &idr).unwrap(); + mux.finish() + .expect("a written keyframe makes finish succeed"); + } + const AUDIO_PID: u16 = 0x1100; /// Decode the 33-bit PTS from the first PUSI packet on `pid`. Assumes diff --git a/src/scsi/mod.rs b/src/scsi/mod.rs index 8fc32e6..a7fb5af 100644 --- a/src/scsi/mod.rs +++ b/src/scsi/mod.rs @@ -600,6 +600,82 @@ pub fn build_read10_fua(lba: u32, count: u16) -> [u8; 10] { ] } +/// Round a pointer/address `p` up to the next boundary that satisfies an +/// SPTI-style `AlignmentMask` (`STORAGE_ADAPTER_DESCRIPTOR::AlignmentMask`, +/// ntddscsi.h / winioctl.h). +/// +/// `mask` is a *mask*, not a power-of-two alignment value: `0` means "no +/// alignment requirement" (any address is fine), `1` means 2-byte, `3` +/// means DWORD (4-byte), `7` means 8-byte, etc. — always one less than the +/// required alignment. An address is acceptable iff `(addr & mask) == 0`. +/// +/// Returns the smallest `addr >= p` with `(addr & mask) == 0`. The +/// standard branch-free idiom `(p + mask) & !mask` works for any valid +/// (`2^n - 1`) mask, including `mask == 0` (where it is the identity). +/// +/// Lives here, compiled on every platform, so the Windows SPTI bounce +/// buffer in `windows.rs` can share it and so the arithmetic gets unit +/// coverage on macOS/Linux CI even though the SPTI path only builds on +/// Windows. +#[allow(dead_code)] +pub(crate) fn align_up(p: usize, mask: usize) -> usize { + (p + mask) & !mask +} + +#[cfg(test)] +mod align_tests { + use super::align_up; + + #[test] + fn mask_zero_is_identity() { + // AlignmentMask 0 (USB optical bridges) — no alignment required. + for p in [0usize, 1, 2, 3, 7, 8, 13, 4096, 0x7fff_ffff] { + assert_eq!(align_up(p, 0), p); + } + } + + #[test] + fn already_aligned_is_unchanged() { + // DWORD (mask 3): multiples of 4 are already aligned. + assert_eq!(align_up(0, 3), 0); + assert_eq!(align_up(4, 3), 4); + assert_eq!(align_up(8, 3), 8); + // 8-byte (mask 7): multiples of 8. + assert_eq!(align_up(0, 7), 0); + assert_eq!(align_up(16, 7), 16); + } + + #[test] + fn rounds_up_to_next_boundary() { + // mask 1 (2-byte): odd → next even. + assert_eq!(align_up(1, 1), 2); + assert_eq!(align_up(3, 1), 4); + // mask 3 (DWORD): 1,2,3 → 4; 5,6,7 → 8. + assert_eq!(align_up(1, 3), 4); + assert_eq!(align_up(2, 3), 4); + assert_eq!(align_up(3, 3), 4); + assert_eq!(align_up(5, 3), 8); + // mask 7 (8-byte): 1..=7 → 8; 9 → 16. + assert_eq!(align_up(1, 7), 8); + assert_eq!(align_up(7, 7), 8); + assert_eq!(align_up(9, 7), 16); + } + + #[test] + fn result_always_satisfies_mask() { + for &mask in &[0usize, 1, 3, 7, 15, 31, 63] { + for p in 0usize..256 { + let a = align_up(p, mask); + assert!(a >= p, "align_up({p},{mask})={a} went backwards"); + assert_eq!(a & mask, 0, "align_up({p},{mask})={a} not aligned"); + // Smallest such value: anything in (p-1-mask, a) would be < p + // or unaligned; check a - p never exceeds mask. + assert!(a - p <= mask, "align_up({p},{mask})={a} overshot"); + } + } + } +} + #[cfg(test)] mod parse_sense_tests { //! Unit tests for [`parse_sense`]. Covers both SPC-4 sense data diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs index 9d8db53..04054e1 100644 --- a/src/scsi/windows.rs +++ b/src/scsi/windows.rs @@ -140,6 +140,17 @@ pub struct SptiTransport { /// [`WINDOWS_MIN_TRANSFER_BYTES`]. A single READ larger than this fails /// `DeviceIoControl` outright, so [`crate::Drive::read`] chunks to it. max_transfer: usize, + /// Adapter `AlignmentMask` (STORAGE_ADAPTER_DESCRIPTOR, ntddscsi.h / + /// winioctl.h), queried alongside `max_transfer`. It is a *mask*: `0` + /// (the common case on USB optical bridges) means the DataBuffer may + /// sit at any address; `3` means DWORD-aligned, `7` 8-byte, etc. — + /// always one less than the required alignment. SCSI/SAS HBAs report + /// nonzero masks, and IOCTL_SCSI_PASS_THROUGH_DIRECT rejects a + /// misaligned `DataBuffer` (DeviceIoControl fails → all reads return + /// transport failure / status 0xFF). When set and the caller's buffer + /// is misaligned, `execute()` bounces through an aligned scratch + /// buffer (see there). + alignment_mask: u32, } // SptiTransport's only field is an isize HANDLE, so the compiler @@ -202,11 +213,12 @@ impl SptiTransport { }); } - let max_transfer = query_max_transfer_bytes(handle); + let (max_transfer, alignment_mask) = query_adapter_descriptor(handle); Ok(SptiTransport { handle, max_transfer, + alignment_mask, }) } @@ -311,14 +323,24 @@ impl Drop for SptiTransport { } } -/// Query the storage adapter's `MaximumTransferLength` (bytes) via -/// IOCTL_STORAGE_QUERY_PROPERTY / StorageAdapterProperty. On any failure -/// (IOCTL failed, short reply, or a nonsensical zero) returns the -/// conservative [`WINDOWS_MIN_TRANSFER_BYTES`]; otherwise clamps the -/// reported value up to that floor. Never returns 0. -fn query_max_transfer_bytes(handle: isize) -> usize { +/// Query the storage adapter descriptor via IOCTL_STORAGE_QUERY_PROPERTY / +/// StorageAdapterProperty and return `(max_transfer_bytes, alignment_mask)`. +/// +/// `max_transfer_bytes`: the adapter's `MaximumTransferLength`. On any +/// failure (IOCTL failed, short reply, or a nonsensical zero) falls back to +/// the conservative [`WINDOWS_MIN_TRANSFER_BYTES`]; otherwise clamped up to +/// that floor. Never 0. +/// +/// `alignment_mask`: the adapter's `AlignmentMask` (offset 16 in +/// STORAGE_ADAPTER_DESCRIPTOR). `0` means no alignment requirement (the +/// common case for USB optical bridges). A nonzero mask (SCSI/SAS HBAs) +/// forces `execute()` to bounce the DataBuffer through an aligned scratch +/// buffer. If the reply is too short to include `AlignmentMask`, returns +/// `0` (no requirement) — the safe default, since any address satisfies a +/// zero mask and the descriptor's leading fields are read first regardless. +fn query_adapter_descriptor(handle: isize) -> (usize, u32) { if handle == INVALID_HANDLE_VALUE { - return WINDOWS_MIN_TRANSFER_BYTES; + return (WINDOWS_MIN_TRANSFER_BYTES, 0); } let query = StoragePropertyQuery { PropertyId: STORAGE_ADAPTER_PROPERTY, @@ -341,14 +363,25 @@ fn query_max_transfer_bytes(handle: isize) -> usize { }; // MaximumTransferLength sits at offset 8; need at least that many bytes // written for the field to be valid. - let valid = ok != 0 + let max_valid = ok != 0 && bytes_returned as usize >= std::mem::offset_of!(StorageAdapterDescriptor, MaximumTransferLength) + std::mem::size_of::(); - if !valid || desc.MaximumTransferLength == 0 { - return WINDOWS_MIN_TRANSFER_BYTES; - } - (desc.MaximumTransferLength as usize).max(WINDOWS_MIN_TRANSFER_BYTES) + let max_transfer = if !max_valid || desc.MaximumTransferLength == 0 { + WINDOWS_MIN_TRANSFER_BYTES + } else { + (desc.MaximumTransferLength as usize).max(WINDOWS_MIN_TRANSFER_BYTES) + }; + + // AlignmentMask sits at offset 16; only trust it if the reply is long + // enough. Otherwise assume 0 (no alignment requirement). + let align_valid = ok != 0 + && bytes_returned as usize + >= std::mem::offset_of!(StorageAdapterDescriptor, AlignmentMask) + + std::mem::size_of::(); + let alignment_mask = if align_valid { desc.AlignmentMask } else { 0 }; + + (max_transfer, alignment_mask) } impl ScsiTransport for SptiTransport { @@ -396,11 +429,49 @@ impl ScsiTransport for SptiTransport { // sub-second resolution; biasing toward "more time" is safer than // truncating (truncation broke 1500ms fast-reads on Drive::read). sptwb.spt.TimeOutValue = ((timeout_ms + 999) / 1000).max(1); - sptwb.spt.DataBuffer = if data.is_empty() { + + // AlignmentMask bounce buffer. + // + // IOCTL_SCSI_PASS_THROUGH_DIRECT requires `DataBuffer` to satisfy + // the adapter's `AlignmentMask` (`(ptr & mask) == 0`). On USB + // optical bridges the mask is 0, so the caller's buffer is always + // acceptable and we point straight at it (zero-copy fast path). + // On SCSI/SAS HBAs the mask can be 3/7/… ; if the caller's buffer + // happens to be misaligned the IOCTL fails outright (status 0xFF / + // all reads fail). In that case we transfer through an aligned + // scratch buffer: over-allocate by `mask` extra bytes so an aligned + // base is guaranteed to exist inside it, align the base with + // [`crate::scsi::align_up`], and use that as `DataBuffer`. For a + // FROM-device transfer the result is copied back into `data` after + // the IOCTL; for a TO-device transfer `data` is copied in before. + // + // `bounce` is kept alive for the whole `execute()` body so the + // aligned pointer we hand the driver stays valid across the IOCTL. + let mask = self.alignment_mask as usize; + let needs_bounce = + !data.is_empty() && mask != 0 && (data.as_mut_ptr() as usize) & mask != 0; + let mut bounce: Vec = Vec::new(); + let data_ptr: *mut u8 = if data.is_empty() { std::ptr::null_mut() + } else if needs_bounce { + // Over-allocate by `mask` so an aligned start exists within. + bounce = vec![0u8; data.len() + mask]; + let base = bounce.as_mut_ptr() as usize; + let aligned = crate::scsi::align_up(base, mask); + let aligned_ptr = aligned as *mut u8; + // For writes (ToDevice) prime the aligned region with the + // caller's payload before the IOCTL. (FromDevice copies back + // after.) + if direction == DataDirection::ToDevice { + unsafe { + std::ptr::copy_nonoverlapping(data.as_ptr(), aligned_ptr, data.len()); + } + } + aligned_ptr } else { data.as_mut_ptr() }; + sptwb.spt.DataBuffer = data_ptr; sptwb.spt.SenseInfoOffset = std::mem::offset_of!(SptwbDirect, sense) as u32; sptwb.spt.Cdb[..cdb_len].copy_from_slice(&cdb[..cdb_len]); @@ -452,15 +523,28 @@ impl ScsiTransport for SptiTransport { }); } + // Clamp to the caller's buffer length, matching Linux/macOS: a + // driver that reports DataTransferLength > data.len() must never let + // callers read past the buffer they handed in. + let transferred = (sptwb.spt.DataTransferLength as usize).min(data.len()); + + // If we bounced a FROM-device read, copy the aligned scratch back + // into the caller's buffer (only the bytes actually transferred). + if needs_bounce && direction == DataDirection::FromDevice { + let aligned_ptr = data_ptr; // points inside `bounce` + unsafe { + std::ptr::copy_nonoverlapping(aligned_ptr, data.as_mut_ptr(), transferred); + } + } + // `bounce` is dropped here, after the last use of `data_ptr`. + drop(bounce); + let mut sense = [0u8; 32]; sense.copy_from_slice(&sptwb.sense); Ok(ScsiResult { status: sptwb.spt.ScsiStatus, - // Clamp to the caller's buffer length, matching Linux/macOS: a - // driver that reports DataTransferLength > data.len() must never - // let callers read past the buffer they handed in. - bytes_transferred: (sptwb.spt.DataTransferLength as usize).min(data.len()), + bytes_transferred: transferred, sense, }) } diff --git a/tests/integration_progress_and_halt.rs b/tests/integration_progress_and_halt.rs index 40739a6..8bcb838 100644 --- a/tests/integration_progress_and_halt.rs +++ b/tests/integration_progress_and_halt.rs @@ -102,6 +102,7 @@ fn synthetic_disc(capacity_sectors: u32) -> Disc { css: None, encrypted: false, aacs_error: None, + css_error: None, content_format: ContentFormat::BdTs, } } diff --git a/tests/pass_n_size_aware_skip.rs b/tests/pass_n_size_aware_skip.rs index de83bae..5ee021f 100644 --- a/tests/pass_n_size_aware_skip.rs +++ b/tests/pass_n_size_aware_skip.rs @@ -102,6 +102,7 @@ fn synthetic_disc(capacity_sectors: u32) -> Disc { css: None, encrypted: false, aacs_error: None, + css_error: None, content_format: ContentFormat::BdTs, } } diff --git a/tests/passn_handler_ab.rs b/tests/passn_handler_ab.rs index c4e082c..6518fa4 100644 --- a/tests/passn_handler_ab.rs +++ b/tests/passn_handler_ab.rs @@ -186,6 +186,7 @@ fn synthetic_disc(capacity_sectors: u32) -> Disc { css: None, encrypted: false, aacs_error: None, + css_error: None, content_format: ContentFormat::BdTs, } }