From 337e77951cbed6c7943e93bea0cc0068a6d9e115 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Mon, 22 Jun 2026 08:47:52 -0700 Subject: [PATCH] rc2: macOS cross-compile fix + security/recovery hardening - build.rs: pass target -arch to cc so macos_shim cross-compiles (x86_64-apple-darwin) - AACS/CSS: unit-aligned decrypting sweep; per-VTS CSS title keys (hard-fail on wrong VTS); reject truncated Unit_Key_RO; AACS 2.0 sig-verify skip; CSS bus-auth random nonce - recovery: gap-filling mapfile load; sweep/copy resume reconciliation; stale-mapfile abort; patch wedge/damage-window range reset - mux: TS continuity + PSI CC desync guards; HEVC numTemporalLayers clamp; MPEG-2 pending byte-cap; PS parse_pts marker-bit validation; HdrFormat strict parse; Unknown-variant metadata - net/keydb: network:// SSRF parity (IPv4-mapped, CGNAT, 0.0.0.0/8, Class-E); bounded keydb header read + size cap + error context - io: durable mapfile fsync; NFS writeback degrade; sync_file_range error capture; Windows SCSI u32 transfer guard --- build.rs | 14 + src/aacs/handshake.rs | 39 +- src/aacs/keydb.rs | 22 ++ src/aacs/keys.rs | 62 ++- src/css/auth.rs | 26 +- src/css/mod.rs | 21 +- src/disc/mapfile.rs | 208 ++++++++++- src/disc/mod.rs | 767 +++++++++++++++++++++++++++++++++++--- src/disc/patch.rs | 65 +++- src/error.rs | 8 + src/io/writeback/linux.rs | 21 +- src/keydb.rs | 92 +++-- src/labels/mod.rs | 11 +- src/mux/codec/hevc.rs | 5 +- src/mux/codec/mod.rs | 2 +- src/mux/codec/mpeg2.rs | 27 +- src/mux/mkv.rs | 34 +- src/mux/mkvstream.rs | 9 +- src/mux/network.rs | 34 ++ src/mux/ps.rs | 36 +- src/mux/resolve.rs | 93 ++++- src/mux/ts.rs | 195 +++++++++- src/scsi/windows.rs | 15 +- src/sector/decrypting.rs | 68 ++++ tests/crypto_tests.rs | 1 + 25 files changed, 1722 insertions(+), 153 deletions(-) diff --git a/build.rs b/build.rs index 6c2d4cf..6feda5e 100644 --- a/build.rs +++ b/build.rs @@ -8,8 +8,22 @@ fn main() { let obj = format!("{out_dir}/macos_shim.o"); let lib = format!("{out_dir}/libmacos_scsi.a"); + // Build the shim for the TARGET arch, not the host's. A bare `cc` on an + // Apple-Silicon CI runner defaults to arm64, so cross-building to + // x86_64-apple-darwin would link a host-arch object against x86_64 Rust + // code → "Undefined symbols for architecture x86_64". (Still raw `cc`, + // not the `cc` crate, which breaks IOKit exclusive access.) + let target_arch = std::env::var("CARGO_CFG_TARGET_ARCH").unwrap_or_default(); + let clang_arch: &str = if target_arch == "aarch64" { + "arm64" + } else { + &target_arch // x86_64 → x86_64 + }; + std::process::Command::new("cc") .args([ + "-arch", + clang_arch, "-c", "src/scsi/macos_shim.c", "-o", diff --git a/src/aacs/handshake.rs b/src/aacs/handshake.rs index 7ae2943..3138bcd 100644 --- a/src/aacs/handshake.rs +++ b/src/aacs/handshake.rs @@ -905,20 +905,23 @@ pub fn aacs_authenticate( drive_nonce.copy_from_slice(&response[4..24]); drive_cert.copy_from_slice(&response[24..116]); - // Verify drive certificate + // Verify drive certificate. `is_aacs20` tracks the 2.0 cert type so the + // step-6 key-signature verify below is skipped too (see there). + let is_aacs20 = drive_cert[0] == 0x11; if drive_cert[0] == 0x01 { // AACS 1.0 certificate if !verify_cert(&drive_cert) { return Err(Error::AacsCertVerify); } - } else if drive_cert[0] == 0x11 { + } else if is_aacs20 { // AACS 2.0 certificate — verification intentionally skipped here. // Reason: backward compatibility. AACS 2.0 drives accept AACS 1.0 host // certs, so we proceed with the AACS 1.0 flow regardless. The P-256 // LA public key needed to verify 2.0 certs is not always available, and // failing here would break handshakes with drives that work fine otherwise. - // The drive's identity is still authenticated through the ECDH key - // exchange and signature verification in step 6 below. + // The 2.0 cert lays out its public key and signature at different byte + // offsets than the 1.0 cert, so the step-6 verify below (which reads + // 1.0 offsets) cannot validate a 2.0 cert and is skipped for it. } // Step 6: Read drive key point + signature (REPORT KEY format 0x02) @@ -930,19 +933,25 @@ pub fn aacs_authenticate( drive_key_point.copy_from_slice(&response[4..44]); drive_key_sig.copy_from_slice(&response[44..84]); - // Verify drive key signature: sign(drive_nonce=host_nonce || drive_key_point) - let (drive_pub_x, drive_pub_y) = cert_pub_key(&drive_cert); - let mut verify_data = [0u8; 60]; - verify_data[..20].copy_from_slice(&host_nonce); - verify_data[20..60].copy_from_slice(&drive_key_point); + // Verify drive key signature: sign(drive_nonce=host_nonce || drive_key_point). + // Skipped for an AACS 2.0 (type 0x11) cert: `cert_pub_key` reads the public + // key at AACS-1.0 byte offsets, which don't apply to a 2.0 cert, so the + // verify would be meaningless (it would reject every 2.0 drive). Mirrors the + // cert-verify skip above; the ECDH key exchange still proceeds. + if !is_aacs20 { + let (drive_pub_x, drive_pub_y) = cert_pub_key(&drive_cert); + let mut verify_data = [0u8; 60]; + verify_data[..20].copy_from_slice(&host_nonce); + verify_data[20..60].copy_from_slice(&drive_key_point); - let mut sig_r = [0u8; 20]; - let mut sig_s = [0u8; 20]; - sig_r.copy_from_slice(&drive_key_sig[..20]); - sig_s.copy_from_slice(&drive_key_sig[20..40]); + let mut sig_r = [0u8; 20]; + let mut sig_s = [0u8; 20]; + sig_r.copy_from_slice(&drive_key_sig[..20]); + sig_s.copy_from_slice(&drive_key_sig[20..40]); - if !ecdsa_verify(&drive_pub_x, &drive_pub_y, &sig_r, &sig_s, &verify_data) { - return Err(Error::AacsKeyVerify); + if !ecdsa_verify(&drive_pub_x, &drive_pub_y, &sig_r, &sig_s, &verify_data) { + return Err(Error::AacsKeyVerify); + } } // Step 7: Sign host key point (ECDSA over drive_nonce || host_key_point) diff --git a/src/aacs/keydb.rs b/src/aacs/keydb.rs index 61f8207..b21874d 100644 --- a/src/aacs/keydb.rs +++ b/src/aacs/keydb.rs @@ -2,6 +2,16 @@ use std::collections::HashMap; +/// Upper bound on the on-disk keydb.cfg size accepted by [`KeyDb::load`]. +/// The real public UHD keydb is a few MiB; 64 MiB is generous headroom while +/// still bounding the worst-case allocation from a hostile/corrupt file. +const MAX_KEYDB_BYTES: u64 = 64 * 1024 * 1024; + +/// Upper bound on parsed disc entries. The real public keydb carries +/// ~170k+ entries, so the cap sits well above that while still bounding +/// memory against a pathological input. Surplus lines are ignored. +const MAX_DISC_ENTRIES: usize = 500_000; + /// Parsed AACS key database. #[derive(Debug)] pub struct KeyDb { @@ -185,6 +195,9 @@ impl KeyDb { // Disc entry: starts with 0x if line.starts_with("0x") && line.contains(" = ") { + if db.disc_entries.len() >= MAX_DISC_ENTRIES { + continue; + } if let Some(entry) = Self::parse_disc_entry(line) { db.disc_entries.insert(entry.disc_hash.clone(), entry); } @@ -204,6 +217,15 @@ impl KeyDb { /// [`KeyDb`] rather than an error — callers needing a non-empty db must /// check the parsed contents. pub fn load(path: &std::path::Path) -> crate::error::Result { + // Stat-and-cap before reading so a hostile/corrupt file can't force an + // unbounded allocation. A file at or over the cap is rejected outright. + if let Ok(meta) = std::fs::metadata(path) { + if meta.len() > MAX_KEYDB_BYTES { + return Err(crate::error::Error::KeydbLoad { + path: path.display().to_string(), + }); + } + } let data = std::fs::read_to_string(path).map_err(|_| crate::error::Error::KeydbLoad { path: path.display().to_string(), })?; diff --git a/src/aacs/keys.rs b/src/aacs/keys.rs index ab99be9..8b1a089 100644 --- a/src/aacs/keys.rs +++ b/src/aacs/keys.rs @@ -163,6 +163,14 @@ pub fn parse_unit_key_ro(data: &[u8], version: AacsVersion) -> Option= 26 { @@ -1370,6 +1378,41 @@ mod tests { if path.exists() { Some(path) } else { None } } + /// Finding #5 regression: parse_unit_key_ro must REJECT a Unit_Key_RO.inf + /// whose declared `num_unit_keys` exceeds the keys actually present in the + /// buffer, instead of silently returning a short list. A truncated list + /// would later map title CPS units to nonexistent keys. + #[test] + fn parse_unit_key_ro_rejects_truncated_key_list() { + // V10 layout: stride 48, keys start at uk_pos + 48. + // uk_pos = 32; num_uk = 2; keys at 80 and 128. + let uk_pos = 32usize; + let build = |total_len: usize| -> Vec { + let mut data = vec![0u8; total_len]; + // uk_pos as BE32 at [0..4]. + data[0..4].copy_from_slice(&(uk_pos as u32).to_be_bytes()); + // num_unit_keys = 2 (BE16) at uk_pos. + data[uk_pos] = 0x00; + data[uk_pos + 1] = 0x02; + data + }; + + // Full buffer: room for both keys (keys_start 80, key1 at 128..144). + let full = build(144); + let ok = + parse_unit_key_ro(&full, AacsVersion::V10).expect("a full 2-key buffer must parse"); + assert_eq!(ok.encrypted_keys.len(), 2); + + // Truncated buffer: header still declares 2 keys, but only the first + // fits (len 128 — the second key's 16 bytes run off the end). Must be + // rejected, not silently accepted with one key. + let short = build(128); + assert!( + parse_unit_key_ro(&short, AacsVersion::V10).is_none(), + "a buffer declaring more keys than it contains must be rejected" + ); + } + #[test] fn derive_media_key_from_dk_survives_out_of_range_u_mask_shift() { // Regression: a crafted/corrupt MKB with a Subset-Difference @@ -2433,11 +2476,12 @@ mod tests { } #[test] - fn parse_unit_key_ro_stops_early_when_keys_run_off_end() { - // 3 keys declared but the buffer is sized to hold only 2 strides plus - // 8 trailing bytes (not a full 3rd 16-byte key) → the loop's - // `pos + 16 > len` guard breaks and returns the keys that fit, never - // reading OOB. + fn parse_unit_key_ro_rejects_when_keys_run_off_end() { + // Finding #5: 3 keys declared but the buffer holds only 2 strides plus + // 8 trailing bytes (not a full 3rd 16-byte key). The extraction loop + // breaks at the buffer end (never reading OOB), and the post-loop + // length check rejects the short list with None — a truncated/malformed + // .inf must NOT be silently accepted with fewer keys than declared. let uk_pos = 0x60usize; let stride = 48usize; // Room for keys at uk_pos+48 and uk_pos+48+48, then only 8 spare bytes @@ -2446,11 +2490,9 @@ mod tests { let mut data = vec![0u8; size]; data[0..4].copy_from_slice(&(uk_pos as u32).to_be_bytes()); data[uk_pos + 1] = 3; // declare 3 keys - let parsed = parse_unit_key_ro(&data, AacsVersion::V10).unwrap(); - assert_eq!( - parsed.encrypted_keys.len(), - 2, - "must stop at the buffer end, not read past it" + assert!( + parse_unit_key_ro(&data, AacsVersion::V10).is_none(), + "a buffer declaring more keys than it contains must be rejected" ); } diff --git a/src/css/auth.rs b/src/css/auth.rs index 702001a..42c8776 100644 --- a/src/css/auth.rs +++ b/src/css/auth.rs @@ -195,8 +195,14 @@ fn bus_auth(drive: &mut Drive) -> Result<(u8, [u8; 5])> { .map_err(|_| Error::CssAuthFailed)?; let agid = (buf[7] >> 6) & 0x03; - // Host sends challenge - let host_challenge: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; + // Host sends challenge. The spec wants a fresh per-session random nonce, + // not a fixed constant — a predictable challenge weakens the bus-auth + // handshake. + let mut host_challenge = [0u8; 10]; + { + use rand::RngCore; + rand::thread_rng().fill_bytes(&mut host_challenge); + } let mut hc_buf = [0u8; 16]; hc_buf[0] = 0x00; hc_buf[1] = 0x0E; @@ -587,10 +593,20 @@ mod tests { } } - // Walk up from this file (src/css/auth.rs) to the crate `src` root. - let src_root = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + // Scan this crate's `src` plus the sibling workspace crates so the + // key-material logging guard covers every crate that can reach the + // CSS/AACS internals, not just libfreemkv. Missing sibling dirs (e.g. + // when building the crate standalone) are simply skipped. + let manifest = Path::new(env!("CARGO_MANIFEST_DIR")); + let workspace = manifest.parent().unwrap_or(manifest); let mut violations = Vec::new(); - scan_dir(&src_root, FORBIDDEN, &mut violations); + scan_dir(&manifest.join("src"), FORBIDDEN, &mut violations); + for sibling in ["autorip", "freemkv", "freemkv-keysources"] { + let dir = workspace.join(sibling).join("src"); + if dir.is_dir() { + scan_dir(&dir, FORBIDDEN, &mut violations); + } + } assert!( violations.is_empty(), "key material logged in instrumentation:\n{}", diff --git a/src/css/mod.rs b/src/css/mod.rs index be59a19..19e3fa8 100644 --- a/src/css/mod.rs +++ b/src/css/mod.rs @@ -27,6 +27,14 @@ use crate::sector::SectorSource; pub struct CssState { /// 5-byte CSS title key (from SCSI auth or the crack fallback). pub title_key: [u8; 5], + /// LBA half-open span `[start, end)` of the extent set this key was + /// cracked from. CSS title keys are per-VTS: a key cracked from one + /// VTS does NOT descramble a title living in a different VTS. The mux + /// path checks whether the title being opened overlaps this span; if + /// not, it re-cracks from that title's own extents. `None` for keys + /// of unknown provenance (e.g. test fixtures) — treated as "applies + /// everywhere" for backward compatibility. + pub crack_span: Option<(u32, u32)>, } /// Recover the CSS title key with no keys, by scanning scrambled sectors and @@ -70,6 +78,14 @@ pub fn crack_key_halt( // scans nothing. Callers pass `detect_max_batch_sectors(device_path)` for a // live drive, a file-safe value for an image, or 1 to force per-sector. let batch = (batch_sectors.max(1)) as u32; + // Record the LBA span the key is being cracked from so the per-title mux + // path can tell whether a later title lives in the same VTS (overlaps the + // span → key applies) or a different one (→ re-crack). Half-open [min,max). + let crack_span = extents + .iter() + .filter(|e| e.sector_count > 0) + .map(|e| (e.start_lba, e.start_lba.saturating_add(e.sector_count))) + .reduce(|(amin, amax), (bmin, bmax)| (amin.min(bmin), amax.max(bmax))); let mut tried = 0u32; let max_tries = 50_000u32; let mut buf = vec![0u8; batch as usize * 2048]; @@ -107,7 +123,10 @@ pub fn crack_key_halt( let sect = &buf[s * 2048..(s + 1) * 2048]; if is_scrambled(sect) { if let Some(key) = stevenson::crack_title_key(sect) { - return Some(CssState { title_key: key }); + return Some(CssState { + title_key: key, + crack_span, + }); } } if tried >= max_tries { diff --git a/src/disc/mapfile.rs b/src/disc/mapfile.rs index a98fcea..b1ad24d 100644 --- a/src/disc/mapfile.rs +++ b/src/disc/mapfile.rs @@ -214,19 +214,34 @@ impl Mapfile { } continue; } - // First non-comment line is the "current" state line (pos status [pass] [pass_time]). - // We ignore its contents but skip over it. + // First non-comment line is the "current" state line + // (`pos status [pass] [pass_time]`). We ignore its contents but + // skip over it. if !saw_current_line { saw_current_line = true; - // But if the line looks like an entry (has at least 3 fields starting 0x...), - // it's probably actually an entry for a mapfile we wrote without a current line. - // Heuristic: current line has status char as 2nd field; entry has size as 2nd field. + // Discriminate by ddrescue's actual line shape, not by a + // `0x`-prefix heuristic (which dropped a valid first data line + // whose size field happened to lack `0x`). A *current* line's + // 2nd field is a single status char (`?*/-+`); a *data* line's + // 2nd field is the hex size, with the status char in the 3rd. + // So: single-char-and-valid-status 2nd field ⇒ current line + // (skip); anything else ⇒ fall through to entry parse. let fields: Vec<&str> = t.split_whitespace().collect(); - if fields.len() >= 3 && fields[1].starts_with("0x") { - // It's an entry, not a current line — fall through to entry parse. - } else { + let is_current_line = fields + .get(1) + .and_then(|f| { + let mut chars = f.chars(); + match (chars.next(), chars.next()) { + // Exactly one char that is a valid status char. + (Some(c), None) => SectorStatus::from_char(c), + _ => None, + } + }) + .is_some(); + if is_current_line { continue; } + // Otherwise it's a data line — fall through to entry parse. } // Entry: `pos size statuschar` let fields: Vec<&str> = t.split_whitespace().collect(); @@ -245,6 +260,13 @@ impl Mapfile { let e: io::Error = crate::error::Error::MapfileInvalid { kind: "range" }.into(); return Err(e); } + // A zero-size entry is degenerate: it contributes nothing to the + // partition yet trips overlap/coalesce arithmetic (two entries can + // share the same pos). Reject it rather than carry it through. + if size == 0 { + let e: io::Error = crate::error::Error::MapfileInvalid { kind: "zero_size" }.into(); + return Err(e); + } let status = fields[2] .chars() .next() @@ -261,18 +283,45 @@ impl Mapfile { entries.push(MapEntry { pos, size, status }); } entries.sort_by_key(|e| e.pos); - // Reject overlapping ranges. A well-formed ddrescue mapfile is a - // disjoint partition; overlaps (from a corrupt/hand-edited file) - // would make compute_stats double-count, so bytes_good / - // bytes_unreadable / bytes_pending could exceed bytes_total and - // inflate resume / abort-on-loss decisions and >100% progress. - for pair in entries.windows(2) { - let prev_end = pair[0].pos.saturating_add(pair[0].size); - if prev_end > pair[1].pos { - let e: io::Error = crate::error::Error::MapfileInvalid { kind: "overlap" }.into(); - return Err(e); + // Reject overlapping ranges, then COALESCE-FILL any internal gaps + // with synthetic NonTried entries. A well-formed ddrescue mapfile + // is a *gap-free* disjoint partition of [0, total_size). + // + // Overlaps (from a corrupt/hand-edited file) would make + // compute_stats double-count, so bytes_good / bytes_unreadable / + // bytes_pending could exceed bytes_total and inflate resume / + // abort-on-loss decisions and >100% progress — hard-reject those. + // + // GAPS are a subtler hazard: total_size is derived from the last + // entry's end, so a holed mapfile passes the caller's + // `covers_disc = (total_size == disc_size)` check and copy() would + // report complete=true even though the hole was never read. Rather + // than hard-reject (which would strand existing partial mapfiles), + // we fill every gap — leading, internal, and any between entries — + // with a NonTried entry so the gap is visible to the resume + // sweep's NonTried region list and actually gets read. (A trailing + // gap up to the disc size is filled by the caller's full-sweep + // path when total_size < disc_size; here we only have the mapfile's + // own extent to reason about.) + let mut filled: Vec = Vec::with_capacity(entries.len() + 1); + let mut cursor: u64 = 0; + for e in entries { + if e.pos < cursor { + let err: io::Error = crate::error::Error::MapfileInvalid { kind: "overlap" }.into(); + return Err(err); } + if e.pos > cursor { + // Leading or internal gap — fill it as NonTried. + filled.push(MapEntry { + pos: cursor, + size: e.pos - cursor, + status: SectorStatus::NonTried, + }); + } + cursor = e.pos.saturating_add(e.size); + filled.push(e); } + let entries = filled; let total_size = entries .last() .map(|e| e.pos.saturating_add(e.size)) @@ -564,6 +613,12 @@ impl Mapfile { )?; } w.flush()?; + // fsync the tmp file before the rename so the bytes are durable on + // disk (notably on NFS, where a rename can otherwise reach the + // server before the data does and leave a truncated mapfile after + // a crash). Recover the File from the BufWriter to call sync_all. + let file = w.into_inner().map_err(|e| e.into_error())?; + file.sync_all()?; } std::fs::rename(&tmp, &self.path)?; Ok(()) @@ -734,6 +789,30 @@ mod tests { let _ = std::fs::remove_file(&p); } + #[test] + fn write_to_disk_fsyncs_and_leaves_no_tmp() { + // Regression: write_to_disk must recover the File from the BufWriter + // and sync_all() it before rename (NFS durability). The .tmp file + // must not survive a successful write, and the renamed mapfile must + // load back identically. + let p = tmpfile("write_to_disk_fsyncs"); + let _ = std::fs::remove_file(&p); + let mut mf = Mapfile::create(&p, 1000, "test").unwrap(); + mf.record(100, 200, SectorStatus::Finished).unwrap(); + mf.write_to_disk().unwrap(); + + let mut tmp = p.clone().into_os_string(); + tmp.push(".tmp"); + assert!( + !PathBuf::from(&tmp).exists(), + "tmp file should be renamed away after a successful write" + ); + + let loaded = Mapfile::load(&p).unwrap(); + assert_eq!(loaded.entries(), mf.entries()); + let _ = std::fs::remove_file(&p); + } + #[test] fn stats_sum_correctly() { let p = tmpfile("stats_sum_correctly"); @@ -1028,6 +1107,72 @@ mod tests { let _ = std::fs::remove_file(&p); } + /// Regression: a mapfile with an INTERNAL hole (a byte range no entry + /// covers) must load with the hole filled as NonTried, so the hole is + /// visible to resume (counted as pending, not silently "complete"). + /// Without the fill, total_size (= last entry's end) would still equal + /// the disc size and copy()'s `covers_disc && bad_bytes == 0` check + /// would report a holed rip as complete. + #[test] + fn load_fills_internal_gap_as_nontried() { + let p = tmpfile("load_fills_internal_gap"); + let _ = std::fs::remove_file(&p); + // Two Finished entries: [0,0x100) and [0x200,0x300). The hole at + // [0x100,0x200) is never covered. + std::fs::write( + &p, + "# Rescue Logfile. Created by test\n\ + 0x000000000 ? 1 0\n\ + 0x000000000 0x00000100 +\n\ + 0x000000200 0x00000100 +\n", + ) + .unwrap(); + let mf = Mapfile::load(&p).expect("holed mapfile must load (gap filled, not rejected)"); + // The hole [0x100,0x200) must now be a NonTried entry. + let hole = mf + .entries() + .iter() + .find(|e| e.pos == 0x100) + .expect("internal gap must be filled with a synthetic entry"); + assert_eq!(hole.size, 0x100, "filled gap covers the whole hole"); + assert_eq!( + hole.status, + SectorStatus::NonTried, + "filled gap must be NonTried so resume reads it" + ); + // total_size unchanged (last entry end), but the hole is now pending. + assert_eq!(mf.total_size(), 0x300); + assert!( + mf.stats().bytes_pending >= 0x100, + "the hole must count as pending so copy() doesn't report complete" + ); + let _ = std::fs::remove_file(&p); + } + + /// Regression: a LEADING gap (first entry doesn't start at 0) is filled + /// as NonTried too, so resume reads the head of the disc. + #[test] + fn load_fills_leading_gap_as_nontried() { + let p = tmpfile("load_fills_leading_gap"); + let _ = std::fs::remove_file(&p); + std::fs::write( + &p, + "# Rescue Logfile. Created by test\n\ + 0x000000000 ? 1 0\n\ + 0x000000080 0x00000100 +\n", + ) + .unwrap(); + let mf = Mapfile::load(&p).expect("leading-gap mapfile must load"); + let head = mf + .entries() + .first() + .expect("must have a leading fill entry"); + assert_eq!(head.pos, 0, "fill must start at byte 0"); + assert_eq!(head.size, 0x80); + assert_eq!(head.status, SectorStatus::NonTried); + let _ = std::fs::remove_file(&p); + } + #[test] fn num_bad_ranges_counts_unreadable_entries() { let p = tmpfile("num_bad_ranges"); @@ -1266,6 +1411,33 @@ mod tests { let _ = std::fs::remove_file(&p); } + /// Regression (finding 4): a leading DATA line whose size field has NO + /// `0x` prefix (ddrescue/`parse_hex` both accept bare hex) must still be + /// parsed as an entry, not misclassified as the current-status line and + /// dropped. The shape-based discriminator keys off the 2nd field being a + /// single status char (current line) vs. a multi-char hex size (data line). + #[test] + fn load_treats_leading_data_line_without_0x_prefix_as_entry() { + let p = tmpfile("load_leading_entry_no_0x"); + let _ = std::fs::remove_file(&p); + // Note: sizes/positions written WITHOUT the `0x` prefix. + std::fs::write( + &p, + "# Rescue Logfile. Created by test\n\ + 000000000 200 +\n\ + 000000200 100 ?\n", + ) + .unwrap(); + let mf = Mapfile::load(&p).unwrap(); + // The old `0x`-prefix heuristic would have skipped the first line as a + // "current line" and lost a valid `+` entry. Both lines are entries. + assert_eq!(mf.entries().len(), 2); + assert_eq!(mf.entries()[0].size, 0x200); + assert_eq!(mf.entries()[0].status, SectorStatus::Finished); + assert_eq!(mf.entries()[1].status, SectorStatus::NonTried); + let _ = std::fs::remove_file(&p); + } + /// load() parses the version from the `# Rescue Logfile. Created by` /// header and exposes it (round-trips through write_to_disk). #[test] diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 1e732f1..422b5b0 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -606,7 +606,10 @@ impl Resolution { 6 => Resolution::R1080p, 7 => Resolution::R576p, 8 => Resolution::R2160p, - _ => Resolution::Unknown, + other => { + tracing::warn!(video_format = other, "unknown MPLS video_format byte"); + Resolution::Unknown + } } } @@ -675,7 +678,10 @@ impl FrameRate { 6 => FrameRate::F50, 7 => FrameRate::F59_94, 8 => FrameRate::F60, - _ => FrameRate::Unknown, + other => { + tracing::warn!(video_rate = other, "unknown MPLS video_rate byte"); + FrameRate::Unknown + } } } @@ -705,7 +711,10 @@ impl AudioChannels { 3 => AudioChannels::Stereo, 6 => AudioChannels::Surround51, 12 => AudioChannels::Surround71, - _ => AudioChannels::Unknown, + other => { + tracing::warn!(audio_format = other, "unknown MPLS audio_format byte"); + AudioChannels::Unknown + } } } @@ -751,7 +760,10 @@ impl SampleRate { 5 => SampleRate::S192, 12 => SampleRate::S48_192, 14 => SampleRate::S48_96, - _ => SampleRate::Unknown, + other => { + tracing::warn!(audio_rate = other, "unknown MPLS audio_rate byte"); + SampleRate::Unknown + } } } @@ -881,7 +893,12 @@ macro_rules! enum_str { for (s, v) in $name::ALL { if v == self { return f.write_str(s); } } - f.write_str("") + // The only variant not in ALL is the Unknown fallback (kept out + // of ALL so FromStr("unknown") round-trips to it via $default + // without ALL gaining a duplicate key). Display it visibly as + // "unknown" rather than an empty string, which produced blank + // metadata in labels and logs. + f.write_str("unknown") } } impl std::str::FromStr for $name { @@ -981,7 +998,10 @@ impl std::str::FromStr for HdrFormat { return Ok(*v); } } - Ok(HdrFormat::Sdr) + // An unrecognised string is an error, not silently SDR. ("sdr"/"SDR" + // already matched above.) Callers that want SDR-on-unknown opt in + // explicitly with `.unwrap_or(HdrFormat::Sdr)` (e.g. mux/meta.rs). + Err(()) } } @@ -1296,7 +1316,13 @@ impl Disc { // unreliable). The real descramble key is recovered from the // scrambled movie data itself via the known-plaintext attack — no // player keys, no disc-key crack, no REPORT-KEY-derived title key. - let _ = crate::css::auth::unlock_css_reads(session, unlock_lba); + if let Err(e) = crate::css::auth::unlock_css_reads(session, unlock_lba) { + tracing::warn!( + target: "freemkv::scan", + error_code = e.code(), + "CSS bus-auth unlock failed; scrambled sectors may be unavailable" + ); + } // Size the crack's batch reads to THIS drive's per-command max // (DVD ≈ 16; the USB bridge may be lower) — an over-large // READ(10) fails outright and would scan nothing. @@ -1805,12 +1831,6 @@ impl Disc { read_data_key: aacs.read_data_key, } } else if let Some(ref css) = self.css { - // KNOWN LIMITATION (post-1.0): one CSS title key is cracked from the - // main feature and used for every title. On a multi-VTS DVD where a - // secondary VTS carries a *different* per-VTS key, muxing that title - // (`freemkv -t N`) would descramble with the wrong key. The main - // feature, single-VTS discs, and autorip (always title 0) are - // unaffected; per-VTS key storage is tracked for a follow-up. crate::decrypt::DecryptKeys::Css { title_key: css.title_key, } @@ -1819,6 +1839,66 @@ impl Disc { } } + /// Resolve decryption keys for muxing a *specific* title. + /// + /// CSS title keys are per-VTS. The scan cracks one key (from the main + /// feature, title 0 for autorip). Applying it to a title that lives in + /// a *different* VTS would silently descramble with the wrong key + /// (garbage output). When the requested title's extents don't overlap + /// the span the cracked key came from, re-crack the key from this + /// title's own extents using `reader`. AACS / unencrypted / single-VTS + /// paths are identical to [`Self::decrypt_keys`]. + /// + /// `batch_sectors` sizes the crack's batched reads (file-safe value for + /// an ISO; `detect_max_batch_sectors` for a live drive). + pub fn decrypt_keys_for_title( + &self, + idx: usize, + reader: &mut dyn SectorSource, + batch_sectors: u16, + ) -> crate::decrypt::DecryptKeys { + let css = match self.css { + Some(ref c) => c, + None => return self.decrypt_keys(), + }; + let title = match self.titles.get(idx) { + Some(t) if !t.extents.is_empty() => t, + // No extents to crack from — fall back to the disc-wide key. + _ => return self.decrypt_keys(), + }; + // If the title overlaps the span the existing key was cracked from, + // it's the same VTS — the cracked key applies. `crack_span: None` + // (unknown provenance) is also treated as "applies". + let overlaps = match css.crack_span { + None => true, + Some((cs, ce)) => title.extents.iter().any(|e| { + let ts = e.start_lba; + let te = e.start_lba.saturating_add(e.sector_count); + ts < ce && cs < te + }), + }; + if overlaps { + return self.decrypt_keys(); + } + // Different VTS: re-crack from this title's extents, largest first + // (the movie body is the biggest scrambled chunk — same heuristic + // the scan uses). The disc-wide key provably does NOT apply here + // (crack_span is Some and this title doesn't overlap it), so a + // re-crack miss is a HARD failure: return None rather than fall + // back to the known-wrong-VTS key, which would silently descramble + // to garbage. The disc-wide fallback is reserved for the unknown- + // provenance case (crack_span == None), already handled above via + // overlaps == true. + let mut extents = title.extents.clone(); + extents.sort_by(|a, b| b.sector_count.cmp(&a.sector_count)); + match crate::css::crack_key(reader, &extents, batch_sectors) { + Some(state) => crate::decrypt::DecryptKeys::Css { + title_key: state.title_key, + }, + None => crate::decrypt::DecryptKeys::None, + } + } + /// Inject pre-resolved AACS unit keys into a scanned disc — the deferred-mux /// / resume path. The keys come from the mapfile's `# freemkv-uk:` header /// (persisted at sweep time when the disc was keyed), so the mux decrypts @@ -2084,6 +2164,24 @@ impl Disc { ); return self.sweep_internal(reader, path, opts, false); } + // NonTried bytes mean a prior sweep was halted mid-way and the + // mapfile still has un-attempted ranges (the un-swept tail). + // The sweep pass's job is to read those — route to a resume + // sweep FIRST, even when retryable bytes also exist. Checking + // retryable before this (and routing straight to patch) would + // silently abandon the un-swept tail: patch only revisits the + // mapfile's bad ranges, never the NonTried ones. The retry + // (patch) passes run after, driven separately by the caller's + // pass loop, and pick up the retryable bytes the sweep leaves. + if stats.bytes_nontried > 0 { + tracing::info!( + "copy dispatch: → sweep resume (covers_disc=true, \ + nontried={}, retryable={})", + stats.bytes_nontried, + stats.bytes_retryable, + ); + return self.sweep_internal(reader, path, opts, true); + } if stats.bytes_retryable > 0 { tracing::info!( "copy dispatch: → patch (retryable={})", @@ -2091,28 +2189,11 @@ impl Disc { ); return self.patch_internal(reader, path, opts); } - // Fallthrough: covers_disc=true, bytes_retryable=0. - // Two sub-cases: - // - // (a) bytes_nontried > 0: the mapfile covers the disc but - // some ranges were never attempted (e.g. a prior sweep - // was halted mid-way and the mapfile has NonTried gaps). - // Route to a resume sweep so those unread ranges are - // actually read. Returning terminal here would silently - // abandon readable data. - // - // (b) bytes_nontried == 0: all sectors were attempted; any - // remaining bad bytes are already Unreadable — a resume - // sweep would visit zero new sectors and be a no-op. - // Return the terminal result immediately. - if stats.bytes_nontried > 0 { - tracing::info!( - "copy dispatch: → sweep resume (covers_disc=true, \ - retryable=0, nontried={})", - stats.bytes_nontried, - ); - return self.sweep_internal(reader, path, opts, true); - } + // Fallthrough: covers_disc=true, nontried=0, retryable=0. + // All sectors were attempted; any remaining bad bytes are + // already Unreadable. A resume sweep would visit zero new + // sectors and patch has nothing retryable — return the + // terminal result immediately. tracing::info!( "copy dispatch: all bad sectors already Unreadable \ (retryable=0, nontried=0) — returning terminal result", @@ -2223,6 +2304,10 @@ impl Disc { } else { crate::decrypt::DecryptKeys::None }; + // Captured before `keys` moves into the decorator below. A decrypting + // AACS-keyed sweep needs unit-aligned (3-sector) batch sizing + region + // read-starts (see the batch computation further down). + let decrypt_is_aacs = matches!(keys, crate::decrypt::DecryptKeys::Aacs { .. }); // Wrap the producer-side reader once so every read_sectors call // yields plaintext. `DecryptKeys::None` makes the decorator a @@ -2235,8 +2320,73 @@ impl Disc { // Mapfile: load if resuming, else wipe + recreate. let mapfile_path = self.mapfile_for(path); - if !opts.resume { - let _ = std::fs::remove_file(&mapfile_path); + // covers_disc reconciliation. A resume against a mapfile whose total + // size != the real disc size is unsafe — exactly the case copy()'s + // dispatch forces to a fresh sweep (see Disc::copy). Under-cover + // (map < disc) abandons the disc tail [map.total_size(), disc); + // over-cover (map > disc) reads LBAs past capacity. When sweep() is + // called directly (not via copy()), apply the same downgrade: drop the + // stale mapfile and sweep [0, total_bytes) fresh. + let mut resume = opts.resume; + if resume && mapfile_path.exists() { + match mapfile::Mapfile::load(&mapfile_path) { + Ok(existing) => { + if existing.total_size() != total_bytes { + tracing::info!( + "sweep: mapfile total_size {} != disc {}; forcing fresh sweep", + existing.total_size(), + total_bytes, + ); + resume = false; + } else { + // Inconsistent-resume guard. The mapfile claims prior + // progress (some range past NonTried) but the ISO is + // missing or zero-length — the ISO was deleted or + // truncated while the mapfile survived (reachable via + // autorip ResumeMode::Require). The producer only builds + // work from NonTried ranges, so any Finished range would + // never be re-read and would stay ZERO in the fresh ISO, + // silently holed. Downgrade to a fresh full sweep (mirror + // the total_size-mismatch case) so the rip self-heals. + let iso_len = std::fs::metadata(path).map(|m| m.len()).unwrap_or(0); + let claims_progress = + existing.stats().bytes_pending != existing.total_size(); + if iso_len == 0 && claims_progress { + tracing::info!( + "sweep: mapfile claims prior progress (pending {} of {}) but ISO is missing/zero-length; forcing fresh sweep", + existing.stats().bytes_pending, + existing.total_size(), + ); + resume = false; + } + } + } + Err(_) => { + // The mapfile exists but is corrupt / unparseable. Proceeding + // with resume=true would hand a garbage (or empty) mapfile to + // open_or_create and silently skip already-Finished ranges or + // mis-track progress. Downgrade to a fresh sweep — consistent + // with the total_size-mismatch branch above — so the `!resume` + // path below drops the corrupt mapfile and the rip restarts + // clean. + tracing::info!( + "sweep: mapfile at {} is corrupt/unparseable; forcing fresh sweep", + mapfile_path.display(), + ); + resume = false; + } + } + } + if !resume { + // A fresh sweep MUST start from an empty mapfile. If the stale file + // can't be removed, open_or_create would load it and the new disc + // would inherit the old Finished ranges → silently zero-filled ISO. + // ENOENT is fine (nothing to remove); any other error aborts. + match std::fs::remove_file(&mapfile_path) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(Error::IoError { source: e }), + } } let mut map = mapfile::Mapfile::open_or_create( &mapfile_path, @@ -2262,7 +2412,7 @@ impl Disc { let is_regular = std::fs::metadata(path) .map(|m| m.file_type().is_file()) .unwrap_or(false); - let file = if opts.resume + let file = if resume && std::fs::metadata(path) .map(|m| m.len() > 0) .unwrap_or(false) @@ -2285,12 +2435,30 @@ impl Disc { // `crate::io`). The `WritebackFile` moves into the consumer // thread. let file = crate::io::WritebackFile::new(file).map_err(|e| Error::IoError { source: e })?; - let batch: u16 = match opts.batch_sectors { + let mut batch: u16 = match opts.batch_sectors { Some(b) => b, None if opts.skip_on_error => ecc_sectors(self.format), None => DEFAULT_BATCH_SECTORS_OPTICAL, }; + // AACS unit alignment for a DECRYPTING sweep. AACS aligned units are 3 + // sectors (6144 bytes); `decrypt_sectors` anchors units at buffer offset + // 0, so every read handed to the decrypting reader MUST start on a unit + // boundary AND span a whole number of units — otherwise units straddle + // batch/region boundaries and decrypt under the wrong CBC/unit alignment + // (the verify-gate then leaves content encrypted or aborts DecryptFailed). + // + // ecc_sectors() is 32 for UHD/BD, which is NOT a multiple of 3, so the + // default batch would start every batch-after-the-first mid-unit. Round + // the batch UP to the next multiple of 3 (32 → 33) when this sweep both + // decrypts and is AACS-keyed. Region read-starts are aligned DOWN to a + // unit boundary in the loop below; a fresh sweep starts at LBA 0 (already + // aligned), so alignment only bites on resume NonTried regions. + const UNIT_SECTORS: u16 = (crate::aacs::ALIGNED_UNIT_LEN / 2048) as u16; // 3 + if decrypt_is_aacs && batch % UNIT_SECTORS != 0 { + batch = batch.saturating_add(UNIT_SECTORS - (batch % UNIT_SECTORS)); + } + // Pre-compute the list of NonTried regions before handing the // mapfile to the consumer thread. Each region is processed by // the producer in order; the consumer mutates the mapfile per @@ -2322,7 +2490,7 @@ impl Disc { phase = "sweep", total_bytes, skip_on_error = opts.skip_on_error, - resume = opts.resume, + resume, "begin" ); let mut iter_count: u64 = 0; @@ -2360,7 +2528,19 @@ impl Disc { 'outer: for (region_pos, region_size) in regions { let region_end = region_pos + region_size; - let mut pos = region_pos; + // AACS unit alignment: anchor the region's read cursor DOWN to the + // nearest 6144-byte unit boundary so the decrypting reader never gets + // a buffer that starts mid-unit. Re-reading the few already-covered + // head sectors is idempotent (they re-decrypt identically and the + // consumer overwrites the same ISO offsets / mapfile ranges). A fresh + // sweep's NonTried region starts at 0, already unit-aligned; this only + // shifts resume regions that begin mid-unit. + let mut pos = if decrypt_is_aacs { + let unit_bytes = crate::aacs::ALIGNED_UNIT_LEN as u64; + region_pos - (region_pos % unit_bytes) + } else { + region_pos + }; tracing::trace!( target: "freemkv::disc", phase = "region_enter", @@ -3081,7 +3261,7 @@ pub fn detect_max_batch_sectors(device_path: &str) -> u16 { if let Ok(content) = std::fs::read_to_string(&sysfs_path) { if let Ok(kb) = content.trim().parse::() { // Convert KB to sectors (1 sector = 2 KB = 2048 bytes) - let sectors = (kb / 2) as u16; + let sectors = (kb / 2).min(u16::MAX as u32) as u16; // Align down to 3 (one aligned unit) let aligned = (sectors / 3) * 3; if aligned >= MIN_BATCH_SECTORS { @@ -3104,6 +3284,52 @@ pub fn detect_max_batch_sectors(device_path: &str) -> u16 { mod tests { use super::*; + /// AACS unit-alignment of the DECRYPTING multipass sweep. AACS aligned units + /// are 3 sectors (6144 bytes); `decrypt_sectors` anchors units at buffer + /// offset 0, so the sweep MUST (a) round its per-batch sector count UP to a + /// multiple of 3 and (b) align each NonTried region's read cursor DOWN to a + /// unit boundary — otherwise batches after the first start mid-unit and every + /// unit decrypts under the wrong CBC/unit alignment. + /// + /// This mirrors the exact arithmetic the sweep loop uses (the full path needs + /// a live AACS `Disc`, out of reach in a unit test). The decorator-level + /// reject for an unaligned start LBA is covered end-to-end in + /// `sector::decrypting::tests::aacs_unaligned_start_lba_rejected`. + #[test] + fn aacs_sweep_batch_and_region_are_unit_aligned() { + const UNIT_SECTORS: u16 = (crate::aacs::ALIGNED_UNIT_LEN / 2048) as u16; // 3 + let unit_bytes = crate::aacs::ALIGNED_UNIT_LEN as u64; // 6144 + + // (a) Batch rounding: ecc_sectors() for UHD/BD is 32, not a multiple of 3. + // The decrypting-AACS path rounds it up to the next multiple of 3 (33). + for format in [DiscFormat::Uhd, DiscFormat::BluRay] { + let mut batch = ecc_sectors(format); + assert_eq!(batch, 32); + if batch % UNIT_SECTORS != 0 { + batch = batch.saturating_add(UNIT_SECTORS - (batch % UNIT_SECTORS)); + } + assert_eq!(batch, 33, "batch must round 32 -> 33 (a multiple of 3)"); + assert_eq!(batch % UNIT_SECTORS, 0); + // Every full batch read is then a whole number of 6144-byte units. + assert_eq!((batch as u64 * 2048) % unit_bytes, 0); + } + + // (b) Region-start down-alignment. A resume NonTried region can begin + // mid-unit; aligning the read cursor DOWN to the nearest unit boundary + // makes block_lba % 3 == 0 for the first (and thus every) batch read. + // Re-reading the few head sectors is idempotent. + for region_pos in [0u64, 2048, 4096, 6144, 8192, 65536, 67_584] { + let pos = region_pos - (region_pos % unit_bytes); + assert_eq!(pos % unit_bytes, 0, "aligned cursor must be unit-aligned"); + assert!(pos <= region_pos, "alignment only moves the cursor down"); + // block_lba derived as pos/2048 must be a multiple of 3 sectors. + assert_eq!((pos / 2048) % UNIT_SECTORS as u64, 0); + } + // An already-aligned region (fresh sweep starts at 0) is unchanged. + assert_eq!(0u64 - (0u64 % unit_bytes), 0); + assert_eq!(6144u64 - (6144u64 % unit_bytes), 6144); + } + /// Helper: build a DiscTitle with a single video stream at the given resolution. fn title_with_video(codec: Codec, resolution: Resolution) -> DiscTitle { DiscTitle { @@ -3671,11 +3897,111 @@ mod tests { dvd.encrypted = true; dvd.css = Some(crate::css::CssState { title_key: [0u8; 5], + crack_span: None, }); dvd.inject_unit_keys(vec![(0, [0x33; 16])]); assert!(dvd.aacs.is_none(), "CSS disc must not gain an AACS state"); } + /// Records the LBAs read; returns all-zero (unscrambled) sectors so any + /// re-crack attempt finds no key and falls back, while we observe WHETHER + /// the title's extents were read at all. + struct RecordingSource { + reads: std::cell::RefCell>, + } + impl SectorSource for RecordingSource { + fn read_sectors( + &mut self, + lba: u32, + count: u16, + buf: &mut [u8], + _recovery: bool, + ) -> Result { + self.reads.borrow_mut().push(lba); + let n = (count as usize * 2048).min(buf.len()); + for b in buf[..n].iter_mut() { + *b = 0; + } + Ok(n) + } + } + + fn css_disc_with_two_vts() -> Disc { + // Title 0 (cracked VTS) at LBA 100..200; title 1 (other VTS) at + // 5000..5100. The cracked key's span is title 0's extents. + let mut t0 = title_with_video(Codec::Mpeg2, Resolution::R480p); + t0.extents = vec![Extent { + start_lba: 100, + sector_count: 100, + }]; + let mut t1 = title_with_video(Codec::Mpeg2, Resolution::R480p); + t1.playlist = "00801.mpls".into(); + t1.extents = vec![Extent { + start_lba: 5000, + sector_count: 100, + }]; + let mut disc = make_test_disc(6000, "DVD"); + disc.format = DiscFormat::Dvd; + disc.content_format = ContentFormat::MpegPs; + disc.encrypted = true; + disc.titles = vec![t0, t1]; + disc.css = Some(crate::css::CssState { + title_key: [0xAB; 5], + crack_span: Some((100, 200)), + }); + disc + } + + /// Regression (multi-VTS CSS): a title that OVERLAPS the cracked span is + /// the same VTS — the existing key is reused and the reader is NOT touched. + #[test] + fn decrypt_keys_for_title_reuses_key_for_same_vts() { + let disc = css_disc_with_two_vts(); + let mut src = RecordingSource { + reads: std::cell::RefCell::new(Vec::new()), + }; + match disc.decrypt_keys_for_title(0, &mut src, 16) { + crate::decrypt::DecryptKeys::Css { title_key } => { + assert_eq!(title_key, [0xAB; 5], "same-VTS title reuses cracked key"); + } + _ => panic!("expected Css keys for same-VTS title"), + } + assert!( + src.reads.borrow().is_empty(), + "an overlapping title must not trigger a re-crack read" + ); + } + + /// Regression (multi-VTS CSS): a title in a DIFFERENT VTS (no overlap with + /// the cracked span) must re-crack from its OWN extents — verified by the + /// reader being driven over that title's LBA range (5000..). The fixture + /// yields unscrambled sectors so the re-crack finds NO key; the fix + /// requires this to be a HARD failure (`DecryptKeys::None`), NOT a silent + /// fall-back to the known-wrong-VTS disc-wide key (which would descramble + /// to garbage). Both the read-attempt and the None result are asserted. + #[test] + fn decrypt_keys_for_title_recracks_for_other_vts() { + let disc = css_disc_with_two_vts(); + let mut src = RecordingSource { + reads: std::cell::RefCell::new(Vec::new()), + }; + let keys = disc.decrypt_keys_for_title(1, &mut src, 16); + assert!( + matches!(keys, crate::decrypt::DecryptKeys::None), + "a re-crack miss in a provably-different VTS must be a hard failure (None), \ + not the wrong-VTS disc-wide key" + ); + let reads = src.reads.borrow(); + assert!( + !reads.is_empty(), + "a non-overlapping title must trigger a re-crack read" + ); + assert!( + reads.iter().all(|&lba| lba >= 5000), + "re-crack must read title 1's own extents (>=5000), got {reads:?}" + ); + } + #[test] fn sweep_to_dev_null_no_enodev() { let tmp = tempfile::tempdir().unwrap(); @@ -3729,6 +4055,271 @@ mod tests { ); } + /// Regression (finding 6): sweep() resume against a mapfile whose + /// total_size != the real disc size must DOWNGRADE to a fresh full sweep + /// covering [0, capacity), not reuse the stale mapfile (which would + /// abandon the disc tail or read past capacity). Mirrors copy()'s + /// covers_disc reconciliation for the direct-sweep entry point. + #[test] + fn sweep_resume_downgrades_on_size_mismatch() { + let tmp = tempfile::tempdir().unwrap(); + let iso_path = tmp.path().join("mismatch.iso"); + + // First sweep: a small disc → mapfile sized to small_sectors. + let small_sectors: u32 = 500; + let mut small_reader = MockReader { + total_sectors: small_sectors, + bad_sectors: std::collections::HashSet::new(), + }; + let small_disc = make_test_disc(small_sectors, "SMALL"); + let opts0 = SweepOptions { + decrypt: false, + resume: false, + batch_sectors: None, + skip_on_error: true, + progress: None, + halt: None, + vid: None, + unit_keys: Vec::new(), + }; + small_disc + .sweep(&mut small_reader, &iso_path, &opts0) + .expect("initial small sweep"); + let mf = small_disc.mapfile_for(&iso_path); + assert_eq!( + mapfile::Mapfile::load(&mf).unwrap().total_size(), + small_sectors as u64 * 2048, + "precondition: mapfile reflects the small disc" + ); + + // Now a LARGER disc resumes against that stale (under-cover) mapfile. + // The reconciliation must force a fresh full sweep of the big disc. + let big_sectors: u32 = 2000; + let mut big_reader = MockReader { + total_sectors: big_sectors, + bad_sectors: std::collections::HashSet::new(), + }; + let big_disc = make_test_disc(big_sectors, "BIG"); + let opts_resume = SweepOptions { + resume: true, + ..opts0 + }; + let result = big_disc + .sweep(&mut big_reader, &iso_path, &opts_resume) + .expect("resume sweep on mismatched mapfile"); + + assert_eq!( + result.bytes_total, + big_sectors as u64 * 2048, + "fresh sweep must be sized to the real (big) disc" + ); + assert_eq!( + result.bytes_good, + big_sectors as u64 * 2048, + "the whole big disc (incl. the tail beyond the stale mapfile) must be swept" + ); + assert_eq!( + mapfile::Mapfile::load(&mf).unwrap().total_size(), + big_sectors as u64 * 2048, + "mapfile must be re-created at the real disc size, not the stale one" + ); + } + + /// Regression (resume/mapfile consistency, MED): a resume sweep against a + /// mapfile that claims prior progress (Finished ranges) while the ISO is + /// missing/zero-length must DOWNGRADE to a fresh full sweep — NOT reuse the + /// stale mapfile. The producer only builds work from NonTried ranges, so a + /// reused mapfile would leave every Finished range unread and ZERO in the + /// new ISO (a silent hole). Reachable via autorip ResumeMode::Require when + /// the ISO was deleted/truncated but the mapfile survived. The fresh-sweep + /// downgrade self-heals: all ranges are re-read and the ISO is fully + /// populated. + #[test] + fn sweep_resume_downgrades_on_zero_iso_with_progress_mapfile() { + let tmp = tempfile::tempdir().unwrap(); + let iso_path = tmp.path().join("zeroed.iso"); + + let sectors: u32 = 500; + let total_bytes = sectors as u64 * 2048; + let disc = make_test_disc(sectors, "ZEROED"); + + // First sweep: clean disc → ISO fully written, mapfile all-Finished. + let mut reader = MockReader { + total_sectors: sectors, + bad_sectors: std::collections::HashSet::new(), + }; + let opts0 = SweepOptions { + decrypt: false, + resume: false, + batch_sectors: None, + skip_on_error: true, + progress: None, + halt: None, + vid: None, + unit_keys: Vec::new(), + }; + disc.sweep(&mut reader, &iso_path, &opts0) + .expect("initial clean sweep"); + let mf = disc.mapfile_for(&iso_path); + let loaded = mapfile::Mapfile::load(&mf).unwrap(); + assert_eq!( + loaded.stats().bytes_pending, + 0, + "precondition: a clean sweep leaves no pending (all Finished) ranges" + ); + + // Truncate the ISO to zero length while the progress-claiming mapfile + // survives — exactly the inconsistent-resume case. + std::fs::OpenOptions::new() + .write(true) + .truncate(true) + .open(&iso_path) + .expect("truncate ISO to zero"); + assert_eq!( + std::fs::metadata(&iso_path).unwrap().len(), + 0, + "precondition: ISO is zero-length" + ); + + // Resume sweep: must downgrade to a fresh FULL sweep, re-reading every + // range (including the formerly-Finished ones). + let mut reader2 = MockReader { + total_sectors: sectors, + bad_sectors: std::collections::HashSet::new(), + }; + let opts_resume = SweepOptions { + resume: true, + ..opts0 + }; + let result = disc + .sweep(&mut reader2, &iso_path, &opts_resume) + .expect("resume sweep on zero-length ISO"); + + // A holed resume would re-read nothing (no NonTried ranges) → bytes_good + // == 0 and a zero ISO. The downgrade re-reads the whole disc. + assert_eq!( + result.bytes_good, total_bytes, + "downgrade must re-read the whole disc, not skip Finished ranges" + ); + assert_eq!( + std::fs::metadata(&iso_path).unwrap().len(), + total_bytes, + "ISO must be re-sized + fully written, not left zero/holed" + ); + + // The ISO must actually contain the swept data (0xAA) at LBA 0 — proof + // the formerly-Finished head range was re-read, not left as a hole. + let iso = std::fs::read(&iso_path).unwrap(); + assert_eq!( + &iso[..2048], + &[0xAAu8; 2048][..], + "head sector must hold re-read data, not a zero hole" + ); + } + + /// Regression (resume reconciliation, MED follow-on): a resume sweep against + /// a CORRUPT / unparseable mapfile must DOWNGRADE to a fresh full sweep — + /// not proceed with resume=true (which would hand a garbage/empty mapfile to + /// open_or_create and silently skip ranges). The `load()` Err arm sets + /// resume=false; the `!resume` path then drops the corrupt mapfile and the + /// rip restarts clean. Consistent with the total_size-mismatch downgrade. + #[test] + fn sweep_resume_downgrades_on_corrupt_mapfile() { + let tmp = tempfile::tempdir().unwrap(); + let iso_path = tmp.path().join("corrupt.iso"); + + let sectors: u32 = 500; + let total_bytes = sectors as u64 * 2048; + let disc = make_test_disc(sectors, "CORRUPT"); + let mf = disc.mapfile_for(&iso_path); + + // Write a non-empty ISO so the zero-length-ISO guard is NOT what triggers + // the downgrade — we want the corrupt-mapfile path specifically. + std::fs::write(&iso_path, vec![0u8; total_bytes as usize]).unwrap(); + // Plant a corrupt mapfile: garbage bytes that Mapfile::load can't parse. + std::fs::write(&mf, b"this is not a valid ddrescue mapfile\nxxxx\n").unwrap(); + assert!( + mapfile::Mapfile::load(&mf).is_err(), + "precondition: the planted mapfile must be unparseable" + ); + + let mut reader = MockReader { + total_sectors: sectors, + bad_sectors: std::collections::HashSet::new(), + }; + let opts = SweepOptions { + decrypt: false, + resume: true, + batch_sectors: None, + skip_on_error: true, + progress: None, + halt: None, + vid: None, + unit_keys: Vec::new(), + }; + let result = disc + .sweep(&mut reader, &iso_path, &opts) + .expect("resume sweep on corrupt mapfile"); + + // The downgrade must re-sweep the whole disc from a fresh mapfile. + assert_eq!( + result.bytes_good, total_bytes, + "corrupt-mapfile resume must downgrade to a fresh full sweep" + ); + let reloaded = mapfile::Mapfile::load(&mf) + .expect("a valid mapfile must have been written by the fresh sweep"); + assert_eq!( + reloaded.total_size(), + total_bytes, + "mapfile must be re-created at the real disc size" + ); + assert_eq!( + reloaded.stats().bytes_pending, + 0, + "the fresh sweep must leave all ranges Finished" + ); + } + + /// Regression: a fresh (non-resume) sweep MUST abort if the stale mapfile + /// cannot be removed, rather than swallowing the error and letting + /// `open_or_create` load the stale file (which would make the new disc + /// inherit old Finished ranges → silently zero-filled ISO). We force the + /// remove to fail with a non-ENOENT error by placing a NON-EMPTY DIRECTORY + /// at the mapfile path (`remove_file` on a dir fails, and a non-empty dir + /// can't be ENOENT). + #[test] + fn sweep_fresh_aborts_when_stale_mapfile_unremovable() { + let tmp = tempfile::tempdir().unwrap(); + let iso_path = tmp.path().join("blocked.iso"); + + let sectors: u32 = 500; + let disc = make_test_disc(sectors, "BLOCKED"); + let mf = disc.mapfile_for(&iso_path); + // Put a non-empty directory where the mapfile would live. + std::fs::create_dir_all(&mf).unwrap(); + std::fs::write(mf.join("occupant"), b"x").unwrap(); + + let mut reader = MockReader { + total_sectors: sectors, + bad_sectors: std::collections::HashSet::new(), + }; + let opts = SweepOptions { + decrypt: false, + resume: false, + batch_sectors: None, + skip_on_error: true, + progress: None, + halt: None, + vid: None, + unit_keys: Vec::new(), + }; + let result = disc.sweep(&mut reader, &iso_path, &opts); + assert!( + result.is_err(), + "fresh sweep must abort when the stale mapfile cannot be removed" + ); + } + struct CleanupGuard(std::path::PathBuf); impl Drop for CleanupGuard { fn drop(&mut self) { @@ -3764,6 +4355,98 @@ mod tests { assert_eq!(r.bytes_good, sectors as u64 * 2048); } + /// Finding #6 regression: on resume, copy() must NOT abandon the un-swept + /// NonTried tail when retryable (NonTrimmed) bytes also remain. The mapfile + /// covers the disc and has BOTH a NonTrimmed (retryable) range and a + /// NonTried tail; dispatch must route to a resume sweep first so the tail is + /// actually read. Before the fix, `bytes_retryable > 0` short-circuited to + /// patch and the NonTried tail was silently left unread. + #[test] + fn resume_sweeps_nontried_tail_even_with_retryable_present() { + use crate::disc::mapfile::{Mapfile, SectorStatus}; + use std::collections::HashSet; + use std::sync::{Arc, Mutex}; + + // Reader that records every LBA it is asked to read. + struct TrackingReader { + total_sectors: u32, + reads: Arc>>, + } + impl crate::sector::SectorSource for TrackingReader { + fn read_sectors( + &mut self, + lba: u32, + count: u16, + buf: &mut [u8], + _recovery: bool, + ) -> crate::error::Result { + { + let mut r = self.reads.lock().unwrap(); + for i in 0..count as u32 { + r.insert(lba + i); + } + } + let n = count as usize * 2048; + buf[..n].fill(0xAA); + Ok(n) + } + fn capacity_sectors(&self) -> u32 { + self.total_sectors + } + } + + let tmp = tempfile::tempdir().unwrap(); + let iso_path = tmp.path().join("test.iso"); + let sectors: u32 = 200; + let disc = make_test_disc(sectors, "T6Tail"); + + // Pre-build a mapfile covering the whole disc: + // [0..100) Finished + // [100..150) NonTrimmed (retryable) + // [150..200) NonTried (un-swept tail) + let mf_path = disc.mapfile_for(&iso_path); + { + let mut mf = Mapfile::create(&mf_path, sectors as u64 * 2048, "test").unwrap(); + mf.record(0, 100 * 2048, SectorStatus::Finished).unwrap(); + mf.record(100 * 2048, 50 * 2048, SectorStatus::NonTrimmed) + .unwrap(); + // [150..200) stays NonTried from create()'s initial region. + mf.flush().unwrap(); + + // Sanity on the constructed state. + let st = mf.stats(); + assert!(st.bytes_nontried > 0, "must have a NonTried tail"); + assert!(st.bytes_retryable > 0, "must have retryable bytes too"); + assert_eq!(mf.total_size(), sectors as u64 * 2048); + } + // The ISO file must exist for the sweep to write into. + std::fs::write(&iso_path, vec![0u8; sectors as usize * 2048]).unwrap(); + + let reads = Arc::new(Mutex::new(HashSet::new())); + let mut reader = TrackingReader { + total_sectors: sectors, + reads: reads.clone(), + }; + let opts = CopyOptions { + decrypt: false, + multipass: true, + progress: None, + halt: None, + vid: None, + unit_keys: Vec::new(), + }; + let result = disc.copy(&mut reader, &iso_path, &opts); + assert!(result.is_ok(), "resume copy failed: {:?}", result.err()); + + // The un-swept tail [150..200) MUST have been read by the resume sweep. + let got = reads.lock().unwrap(); + let tail_read = (150u32..200).any(|lba| got.contains(&lba)); + assert!( + tail_read, + "resume must sweep the NonTried tail; tail sectors were never read" + ); + } + #[test] fn patch_dev_null_after_sweep() { let tmp = tempfile::tempdir().unwrap(); diff --git a/src/disc/patch.rs b/src/disc/patch.rs index 62bd1a2..bb858a3 100644 --- a/src/disc/patch.rs +++ b/src/disc/patch.rs @@ -112,15 +112,23 @@ pub(super) struct SharedPatchState { } impl SharedPatchState { + /// Cap on the republished `bad_ranges` Vec. Consumers (progress display, + /// scheduler) only sample the head of the list; the full set is bounded by + /// the mapfile entry cap so a pathologically fragmented disc can't make + /// every per-record republish allocate unboundedly. + const MAX_BAD_RANGES: usize = 8192; + fn from_map(map: &Mapfile) -> Self { + let mut bad_ranges = map.ranges_with(&[ + SectorStatus::NonTrimmed, + SectorStatus::Unreadable, + SectorStatus::NonScraped, + SectorStatus::NonTried, + ]); + bad_ranges.truncate(Self::MAX_BAD_RANGES); Self { stats: map.stats(), - bad_ranges: map.ranges_with(&[ - SectorStatus::NonTrimmed, - SectorStatus::Unreadable, - SectorStatus::NonScraped, - SectorStatus::NonTried, - ]), + bad_ranges, } } } @@ -149,8 +157,15 @@ pub(super) struct PatchSink { /// `record()` call. `Mutex` rather than separate atomics because /// the producer wants stats + bad_ranges as a coherent pair. shared: Arc>, + /// Last time the shared snapshot was republished. `from_map` allocates + /// O(bad_ranges) every call, so the per-record path throttles to a time + /// cadence (`REPUBLISH_CADENCE`); the final close always forces a publish. + last_republish: Option, } +/// Minimum interval between per-record snapshot republishes. +const REPUBLISH_CADENCE: std::time::Duration = std::time::Duration::from_millis(250); + impl PatchSink { /// Open `path` as a [`crate::io::WritebackFile`] and pair it with /// `map` for the consumer. The producer holds onto the returned @@ -171,12 +186,29 @@ impl PatchSink { map, is_regular, shared, + last_republish: None, }, shared_clone, )) } - fn republish(&self) { + /// Republish the shared snapshot. When `force` is false the update is + /// throttled to `REPUBLISH_CADENCE`; `force` (used at close) always + /// publishes the final state. + fn republish(&mut self, force: bool) { + let now = std::time::Instant::now(); + if !force { + if let Some(prev) = self.last_republish { + if now.duration_since(prev) < REPUBLISH_CADENCE { + return; + } + } + } + self.last_republish = Some(now); + self.publish_now(); + } + + fn publish_now(&self) { // Best-effort lock — only the producer reads, only the consumer // writes; contention is single-acquire so the lock is never // poisoned in practice. If it ever did get poisoned we'd want @@ -220,7 +252,7 @@ impl Sink for PatchSink { .map_err(|e| Error::IoError { source: e })?; } } - self.republish(); + self.republish(false); Ok(Flow::Continue) } @@ -254,7 +286,7 @@ impl Sink for PatchSink { // returned `PatchSummary`, but the snapshot is part of the // public-ish contract of the consumer: it stays current // through close.) - self.republish(); + self.republish(true); Ok(PatchSummary { stats: self.map.stats(), }) @@ -824,6 +856,15 @@ pub(super) fn handle_read_success( } Err(_err) => { state.blocks_read_failed += 1; + // Feed the damage window / wedge counter exactly as the + // main loop does, so a string of backtrack failures + // escalates skip distance and trips wedge detection + // rather than being silently under-counted. + state.damage_window.push(false); + if state.damage_window.len() > PASSN_DAMAGE_WINDOW { + state.damage_window.remove(0); + } + state.consecutive_failures += 1; // Leave NonTrimmed (not Unreadable) so a // later pass gets another shot. Per the // project goal — "recover 100% of readable @@ -1763,6 +1804,12 @@ impl Disc { g.stats.bytes_good }; state.skip_count = 0; + // Reset the wedge counter at each range boundary too. Like + // consecutive_failures below, wedge_count is a "stuck on THIS + // range" signal; carrying it across boundaries lets wedges + // accumulated on earlier ranges trip WEDGE_ABORT_THRESHOLD + // prematurely on a later, healthy range. + state.wedge_count = 0; // Reset consecutive_failures at each range boundary. The // wedge-exit detector is for "stuck on the same range" — many // tiny ranges that each fail their one sampled sector should diff --git a/src/error.rs b/src/error.rs index ac8fc4c..8f6d3dd 100644 --- a/src/error.rs +++ b/src/error.rs @@ -85,6 +85,7 @@ pub const E_AACS_VUK_NOT_IN_KEYDB: u16 = 7019; pub const E_DRIVE_PROFILE_MISSING: u16 = 7020; pub const E_VID_CDB_UNAVAILABLE: u16 = 7021; pub const E_NO_DISC_KEY: u16 = 7022; +pub const E_CSS_KEY_MISSING: u16 = 7023; // Keydb (8xxx) pub const E_KEYDB_CONNECT: u16 = 8000; @@ -304,6 +305,12 @@ pub enum Error { NoDiscKey { disc_hash: String, }, + /// The disc is CSS-encrypted and decryption was requested, but the + /// known-plaintext crack resolved no usable title key for the chosen + /// title (e.g. a multi-VTS DVD where the title's VTS could not be + /// re-cracked). Muxing would emit scrambled ciphertext, so the caller + /// fails fast instead. CSS analogue of [`Error::NoDiscKey`]. + CssKeyMissing, // Keydb (8xxx) KeydbConnect { @@ -479,6 +486,7 @@ impl Error { Error::DriveProfileMissing => E_DRIVE_PROFILE_MISSING, Error::VidCdbUnavailable => E_VID_CDB_UNAVAILABLE, Error::NoDiscKey { .. } => E_NO_DISC_KEY, + Error::CssKeyMissing => E_CSS_KEY_MISSING, Error::KeydbConnect { .. } => E_KEYDB_CONNECT, Error::KeydbHttp { .. } => E_KEYDB_HTTP, Error::KeydbInvalid => E_KEYDB_INVALID, diff --git a/src/io/writeback/linux.rs b/src/io/writeback/linux.rs index 2bdbc7d..7d4e97d 100644 --- a/src/io/writeback/linux.rs +++ b/src/io/writeback/linux.rs @@ -191,12 +191,22 @@ impl WritebackPipeline { // path (NFS, degraded, normal) — it's nominally non-blocking // by spec and gives the kernel an early hint that this range // is ready to flush. - unsafe { + let kickoff_rc = unsafe { libc::sync_file_range( self.fd, chunk_off as i64, chunk_len as i64, libc::SYNC_FILE_RANGE_WRITE, + ) + }; + if kickoff_rc != 0 { + // Non-fatal: the async write-out hint failed, but the data is + // still in the page cache and will be flushed by later fsync / + // kernel writeback. Surface it for diagnosability. + tracing::warn!( + target: "freemkv::io", + errno = std::io::Error::last_os_error().raw_os_error().unwrap_or(0), + "sync_file_range(WRITE) kickoff failed" ); } if let Some((prev_off, prev_len)) = self.pending.take() { @@ -233,9 +243,16 @@ impl WritebackPipeline { // NOT call DONTNEED — if WAIT_AFTER hasn't // returned, the pages aren't safely flushed. self.degraded.store(true, Ordering::Relaxed); + // Once degraded we skip DONTNEED, so every subsequent + // chunk's pages stay resident until close — the same + // page-cache exposure profile as NFS. Shrink to the + // floor so that exposure window is as small as the NFS + // path keeps it, instead of whatever the adaptive sizing + // had grown chunk_bytes to (up to 256 MiB). + self.chunk_bytes = CHUNK_BYTES_MIN; tracing::error!( target: "mux", - "WritebackPipeline WAIT_AFTER timed out after {}s on chunk off={} len={}, marking writeback degraded (subsequent chunks will skip WAIT_AFTER + DONTNEED)", + "WritebackPipeline WAIT_AFTER timed out after {}s on chunk off={} len={}, marking writeback degraded (subsequent chunks will skip WAIT_AFTER + DONTNEED, chunk_bytes lowered to floor)", WAIT_AFTER_TIMEOUT.as_secs(), prev_off, prev_len diff --git a/src/keydb.rs b/src/keydb.rs index f48744e..93ed43f 100644 --- a/src/keydb.rs +++ b/src/keydb.rs @@ -90,12 +90,18 @@ pub fn save(data: &[u8]) -> Result { let path = default_path()?; if let Some(dir) = path.parent() { - std::fs::create_dir_all(dir).map_err(|_| Error::KeydbWrite { - path: path.display().to_string(), + std::fs::create_dir_all(dir).map_err(|e| { + tracing::warn!(error = %e, path = %path.display(), "keydb dir create failed"); + Error::KeydbWrite { + path: path.display().to_string(), + } })?; } - std::fs::write(&path, &text).map_err(|_| Error::KeydbWrite { - path: path.display().to_string(), + std::fs::write(&path, &text).map_err(|e| { + tracing::warn!(error = %e, path = %path.display(), "keydb write failed"); + Error::KeydbWrite { + path: path.display().to_string(), + } })?; Ok(UpdateResult { @@ -125,8 +131,10 @@ fn http_get(url: &str) -> Result> { .ok() .and_then(|mut it| it.next()) .ok_or_else(|| Error::KeydbConnect { host: host.clone() })?; - let mut stream = TcpStream::connect_timeout(&addr, NET_TIMEOUT) - .map_err(|_| Error::KeydbConnect { host: host.clone() })?; + let mut stream = TcpStream::connect_timeout(&addr, NET_TIMEOUT).map_err(|e| { + tracing::debug!(error = %e, host = %host, "keydb connect failed"); + Error::KeydbConnect { host: host.clone() } + })?; stream .set_read_timeout(Some(READ_TIMEOUT)) .map_err(|_| Error::KeydbConnect { host: host.clone() })?; @@ -144,27 +152,45 @@ fn http_get(url: &str) -> Result> { .write_all(request.as_bytes()) .map_err(|_| Error::KeydbConnect { host: host.clone() })?; - let mut response = Vec::new(); - stream - .take(100 * 1024 * 1024) - .read_to_end(&mut response) - .map_err(|_| Error::KeydbConnect { host: host.clone() })?; - - let header_end = find_header_end(&response).ok_or(Error::KeydbParse)?; + // Read the header block incrementally up to the \r\n\r\n terminator, + // bounded to ~64 KiB, BEFORE pulling any body. This avoids buffering up + // to 100 MiB per redirect hop just to inspect the status / Location. + const MAX_HEADER_BYTES: usize = 64 * 1024; + let mut reader = std::io::BufReader::new(stream); + let mut header_buf: Vec = Vec::with_capacity(1024); + let mut byte = [0u8; 1]; + loop { + let n = reader + .read(&mut byte) + .map_err(|_| Error::KeydbConnect { host: host.clone() })?; + if n == 0 { + // Connection closed before headers completed. + return Err(Error::KeydbParse); + } + header_buf.push(byte[0]); + if header_buf.ends_with(b"\r\n\r\n") { + break; + } + if header_buf.len() > MAX_HEADER_BYTES { + return Err(Error::KeydbParse); + } + } + // header_buf includes the trailing \r\n\r\n. + let header_end = header_buf.len() - 4; // Lossy: a stray non-UTF-8 byte in the header block must not blank // out the whole status line / Location header (which would surface // as an undiagnosable KeydbHttp{status:0}). - let headers = String::from_utf8_lossy(&response[..header_end]); - let body = &response[header_end + 4..]; + let headers = String::from_utf8_lossy(&header_buf[..header_end]).into_owned(); + let headers = headers.as_str(); - let status = parse_status(&headers); + let status = parse_status(headers).ok_or(Error::KeydbParse)?; // Only treat a Location header as a redirect when the status is // actually 3xx; a 200 carrying a stray Location (some proxies) is // not a redirect, and a 3xx without Location is a malformed redirect. if (300..=399).contains(&status) { let location = - extract_header(&headers, "Location").ok_or(Error::KeydbHttp { status })?; + extract_header(headers, "Location").ok_or(Error::KeydbHttp { status })?; let (next_host, next_port, next_path) = resolve_redirect(&location, &host, port)?; host = next_host; port = next_port; @@ -176,7 +202,14 @@ fn http_get(url: &str) -> Result> { return Err(Error::KeydbHttp { status }); } - return Ok(body.to_vec()); + // Now read the body, still bounded by the existing 100 MiB cap. The + // BufReader carries any bytes already buffered past the header. + let mut body = Vec::new(); + reader + .take(100 * 1024 * 1024) + .read_to_end(&mut body) + .map_err(|_| Error::KeydbConnect { host: host.clone() })?; + return Ok(body); } Err(Error::KeydbTooManyRedirects) @@ -244,15 +277,18 @@ fn parse_url(url: &str) -> Result<(String, u16, String)> { Ok((host.to_string(), port, path.to_string())) } -fn parse_status(headers: &str) -> u16 { +fn parse_status(headers: &str) -> Option { headers .lines() .next() .and_then(|l| l.split_whitespace().nth(1)) .and_then(|s| s.parse().ok()) - .unwrap_or(0) } +/// Locate the end of the HTTP header block (the index of the `\r\n\r\n`). +/// Retained for the framing unit tests; the live path now reads headers +/// incrementally in `http_get` so the whole response is never buffered. +#[cfg_attr(not(test), allow(dead_code))] fn find_header_end(data: &[u8]) -> Option { data.windows(4).position(|w| w == b"\r\n\r\n") } @@ -357,9 +393,9 @@ mod tests { #[test] fn parse_status_extracts_code() { - assert_eq!(parse_status("HTTP/1.0 200 OK\r\nFoo: bar"), 200); - assert_eq!(parse_status("HTTP/1.1 301 Moved Permanently"), 301); - assert_eq!(parse_status("garbage"), 0); + assert_eq!(parse_status("HTTP/1.0 200 OK\r\nFoo: bar"), Some(200)); + assert_eq!(parse_status("HTTP/1.1 301 Moved Permanently"), Some(301)); + assert_eq!(parse_status("garbage"), None); } // ── New comprehensive tests ──────────────────────────────────────────────── @@ -552,12 +588,12 @@ mod tests { assert!(result.is_ok(), "exactly MAX_KEYDB_BYTES must be accepted"); } - /// parse_status returns 0 for an empty status line (not a panic). - /// Mutation: calling unwrap() instead of unwrap_or(0) panics on empty input. + /// parse_status returns None for an empty/malformed status line (not a + /// meaningless 0). The call site maps None to Error::KeydbParse. #[test] - fn parse_status_empty_input_returns_0() { - assert_eq!(parse_status(""), 0); - assert_eq!(parse_status("\r\n"), 0); + fn parse_status_empty_input_returns_none() { + assert_eq!(parse_status(""), None); + assert_eq!(parse_status("\r\n"), None); } /// Regression: set_read_timeout / set_write_timeout failures must surface as diff --git a/src/labels/mod.rs b/src/labels/mod.rs index 6c983d2..29e61a8 100644 --- a/src/labels/mod.rs +++ b/src/labels/mod.rs @@ -288,8 +288,15 @@ pub fn fill_defaults(titles: &mut [crate::disc::DiscTitle]) { a.label = generate_audio_label(&a.codec, &a.channels, a.secondary); } Stream::Video(v) if v.label.is_empty() => { - v.label = - generate_video_label(&v.codec, v.resolution.pixels(), &v.hdr, v.secondary); + // Unknown resolution: pass (0, 0) so the label omits the + // resolution token rather than tagging it a fabricated + // 1080p. + let px = if matches!(v.resolution, crate::disc::Resolution::Unknown) { + (0, 0) + } else { + v.resolution.pixels() + }; + v.label = generate_video_label(&v.codec, px, &v.hdr, v.secondary); } Stream::Subtitle(s) if s.forced => { // Ensure forced subs are labeled even if BD-J didn't set a name diff --git a/src/mux/codec/hevc.rs b/src/mux/codec/hevc.rs index ba989c9..0ac169e 100644 --- a/src/mux/codec/hevc.rs +++ b/src/mux/codec/hevc.rs @@ -447,7 +447,10 @@ impl CodecParser for HevcParser { // numTemporalLayers u(3) = sps_max_sub_layers_minus1 + 1 // temporalIdNested u(1) = sps_temporal_id_nesting_flag // lengthSizeMinusOne u(2) = 3 (4-byte length prefix) - let num_temporal_layers = (chroma.max_sub_layers_minus1 + 1) & 0x07; + // sps_max_sub_layers_minus1 is u(3) (0..7), so +1 is 1..8. The hvcC + // numTemporalLayers field is u(3) (0..7); the max legal value (8) is + // saturated to 7 rather than wrapping to 0 via the & 0x07 mask. + let num_temporal_layers = chroma.max_sub_layers_minus1.saturating_add(1).min(7) & 0x07; let temporal_id_nested = chroma.temporal_id_nesting_flag & 0x01; record.push((num_temporal_layers << 3) | (temporal_id_nested << 2) | 0x03); // numOfArrays diff --git a/src/mux/codec/mod.rs b/src/mux/codec/mod.rs index 1b26bea..7e44c8c 100644 --- a/src/mux/codec/mod.rs +++ b/src/mux/codec/mod.rs @@ -105,7 +105,7 @@ impl PassthroughParser { impl CodecParser for PassthroughParser { fn parse(&mut self, pes: &PesPacket) -> Vec { - let pts_ns = pes.pts.map(pts_to_ns).unwrap_or(0); + let pts_ns = pes.pts.or(pes.dts).map(pts_to_ns).unwrap_or(0); vec![Frame { pts_ns, keyframe: self.keyframe, diff --git a/src/mux/codec/mpeg2.rs b/src/mux/codec/mpeg2.rs index 6f1d7f1..d93f3c9 100644 --- a/src/mux/codec/mpeg2.rs +++ b/src/mux/codec/mpeg2.rs @@ -58,6 +58,13 @@ const MAX_AU_BUFFER: usize = 8 * 1024 * 1024; /// ever arrives within the cap, buffered frames are released on a 0 base. const MAX_PENDING_FRAMES: usize = 600; +/// Byte cap on frames held awaiting the first PES PTS anchor. `MAX_PENDING_FRAMES` +/// alone bounds the *count*, but 600 full HD/UHD intra pictures can be ~1 GiB. +/// Mirror the AC-3/DTS/PGS byte caps: once the held data exceeds this, release +/// on the 0 base instead of accumulating further. 8 MiB ≈ a few large I-frames, +/// far more than the ~15 frames a well-formed DVD buffers before its first PTS. +const MAX_PENDING_BYTES: usize = 8 * 1024 * 1024; + /// Frame rate table (index from sequence header frame_rate_code). const FRAME_RATES: [(u32, u32); 9] = [ (0, 1), // 0: forbidden @@ -117,6 +124,9 @@ pub struct Mpeg2Parser { /// sequence whose PTS lands a few frames in; buffering until the anchor lets /// those leading frames take the disc's real timeline instead of a 0 base. pending: Vec<(u64, Frame)>, + /// Accumulated `data.len()` of frames currently in `pending`. Bounds the + /// pre-anchor hold by BYTES, not just frame count (see [`MAX_PENDING_BYTES`]). + pending_bytes: usize, } impl Default for Mpeg2Parser { @@ -139,6 +149,7 @@ impl Mpeg2Parser { anchor_index: None, anchor_pts: 0, pending: Vec::new(), + pending_bytes: 0, } } @@ -287,6 +298,7 @@ impl Mpeg2Parser { p + (di as i64 - display_index as i64) * self.frame_duration_ns; out.push(held); } + self.pending_bytes = 0; frame.pts_ns = p; out.push(frame); } @@ -296,13 +308,25 @@ impl Mpeg2Parser { + (display_index as i64 - ai as i64) * self.frame_duration_ns; out.push(frame); } - None if self.pending.len() < MAX_PENDING_FRAMES => { + None if self.pending.len() < MAX_PENDING_FRAMES + && self.pending_bytes < MAX_PENDING_BYTES => + { // No anchor yet — hold so leading frames get the // disc's real timeline once the first PTS arrives, // not a 0 base. + self.pending_bytes += frame.data.len(); self.pending.push((display_index, frame)); } None => { + // Hold cap (count OR bytes) reached without a PTS + // anchor ever arriving. Release everything held so + // far on the 0-base timeline rather than growing the + // buffer unbounded, then emit this frame the same way. + for (di, mut held) in self.pending.drain(..) { + held.pts_ns = di as i64 * self.frame_duration_ns; + out.push(held); + } + self.pending_bytes = 0; frame.pts_ns = display_index as i64 * self.frame_duration_ns; out.push(frame); } @@ -364,6 +388,7 @@ impl CodecParser for Mpeg2Parser { frame.pts_ns = di as i64 * self.frame_duration_ns; out.push(frame); } + self.pending_bytes = 0; } out } diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index 26333d5..108121e 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -6,7 +6,8 @@ use super::ebml; use crate::disc::{ - AudioStream, Chapter, Codec, ColorSpace, HdrFormat, SubtitleStream, VideoStream, + AudioChannels, AudioStream, Chapter, Codec, ColorSpace, HdrFormat, Resolution, SampleRate, + SubtitleStream, VideoStream, }; use std::io::{self, Seek, Write}; @@ -68,7 +69,14 @@ impl MkvTrack { Codec::Mpeg2 => ebml::CODEC_MPEG2, _ => ebml::CODEC_MPEG2, }; - let (w, h) = v.resolution.pixels(); + // An Unknown resolution has no real dimensions — emit (0, 0) so the + // serializer omits PixelWidth/PixelHeight (Matroska marks them + // optional) rather than writing a fabricated 1920x1080 default. + let (w, h) = if matches!(v.resolution, Resolution::Unknown) { + (0, 0) + } else { + v.resolution.pixels() + }; let (num, den) = v.frame_rate.as_fraction(); let default_duration_ns = if num > 0 { (1_000_000_000u64 * den as u64) / num as u64 @@ -139,8 +147,20 @@ impl MkvTrack { Codec::Lpcm => ebml::CODEC_PCM_BE, _ => ebml::CODEC_AC3, }; - let sr = a.sample_rate.hz(); - let ch = a.channels.count(); + // Unknown sample rate / channel layout: emit 0 so the serializer omits + // the SamplingFrequency / Channels element (Matroska supplies its own + // spec default) rather than writing a fabricated 48000 Hz / 6-channel + // value into the file. + let sr = if matches!(a.sample_rate, SampleRate::Unknown) { + 0.0 + } else { + a.sample_rate.hz() + }; + let ch = if matches!(a.channels, AudioChannels::Unknown) { + 0 + } else { + a.channels.count() + }; let name = a.label.clone(); @@ -592,7 +612,11 @@ impl MkvMuxer { if track.track_type == ebml::TRACK_TYPE_AUDIO && track.sample_rate > 0.0 { let aud_pos = ebml::start_master(&mut writer, ebml::AUDIO)?; ebml::write_float(&mut writer, ebml::SAMPLING_FREQUENCY, track.sample_rate)?; - ebml::write_uint(&mut writer, ebml::CHANNELS, track.channels as u64)?; + // Omit Channels when unknown (0) — Matroska defaults it to 1 + // rather than us fabricating a 6-channel count. + if track.channels > 0 { + ebml::write_uint(&mut writer, ebml::CHANNELS, track.channels as u64)?; + } if track.bit_depth > 0 { ebml::write_uint(&mut writer, ebml::BIT_DEPTH, track.bit_depth as u64)?; } diff --git a/src/mux/mkvstream.rs b/src/mux/mkvstream.rs index fb8def9..058caba 100644 --- a/src/mux/mkvstream.rs +++ b/src/mux/mkvstream.rs @@ -214,7 +214,14 @@ impl crate::pes::Stream for MkvStream { if cs == u64::MAX { return Err(crate::error::Error::MkvInvalid.into()); } - remaining = remaining.saturating_sub(hlen as u64 + cs); + // A child whose header + body exceeds the bytes left in + // the BlockGroup is malformed — reject it rather than + // saturating `remaining` to 0 and reading past the group. + let consumed = (hlen as u64).saturating_add(cs); + if consumed > remaining { + return Err(crate::error::Error::MkvInvalid.into()); + } + remaining -= consumed; match cid { ebml::BLOCK => { block = Some(ebml::read_binary_val( diff --git a/src/mux/network.rs b/src/mux/network.rs index 7e48cab..d43b12d 100644 --- a/src/mux/network.rs +++ b/src/mux/network.rs @@ -25,12 +25,19 @@ const NET_BUF_SIZE: usize = 256 * 1024; pub(crate) fn is_blocked_ip(ip: IpAddr) -> bool { match ip { IpAddr::V4(v4) => { + let o = v4.octets(); v4.is_loopback() || v4.is_private() || v4.is_link_local() || v4.is_unspecified() || v4.is_multicast() || v4.is_broadcast() + // carrier-grade NAT 100.64.0.0/10 + || (o[0] == 100 && (o[1] & 0xc0) == 0x40) + // "this network" 0.0.0.0/8 + || o[0] == 0 + // Class E reserved 240.0.0.0/4 + || o[0] >= 240 } IpAddr::V6(v6) => { v6.is_loopback() @@ -40,6 +47,10 @@ pub(crate) fn is_blocked_ip(ip: IpAddr) -> bool { || (v6.segments()[0] & 0xfe00) == 0xfc00 // link-local fe80::/10 || (v6.segments()[0] & 0xffc0) == 0xfe80 + // IPv4-mapped (::ffff:x.x.x.x) and IPv4-compatible (::x.x.x.x); + // to_ipv4() returns Some for both forms — re-check as IPv4 so an + // IPv4-mapped private/loopback address can't bypass the block above. + || v6.to_ipv4().map(|m| is_blocked_ip(IpAddr::V4(m))) == Some(true) } } } @@ -268,6 +279,25 @@ mod tests { IpAddr::V6(Ipv6Addr::new(0xff02, 0, 0, 0, 0, 0, 0, 1)), "multicast v6", ), + // CGNAT / 0.0.0.0/8 / Class E (finding 8). + (v4(100, 64, 0, 1), "carrier-grade NAT"), + (v4(100, 127, 255, 254), "carrier-grade NAT edge"), + (v4(0, 1, 2, 3), "0.0.0.0/8"), + (v4(240, 0, 0, 1), "Class E"), + (v4(255, 0, 0, 1), "Class E high"), + // IPv4-mapped / -compatible IPv6 bypass (finding 7). + ( + IpAddr::V6(Ipv6Addr::new(0, 0, 0, 0, 0, 0xffff, 0x0a00, 0x0001)), + "IPv4-mapped RFC1918 (::ffff:0a00:0001)", + ), + ( + IpAddr::V6(Ipv6Addr::new(0, 0, 0, 0, 0, 0xffff, 0x7f00, 0x0001)), + "::ffff:127.0.0.1 mapped", + ), + ( + IpAddr::V6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0x7f00, 0x0001)), + "::127.0.0.1 compatible", + ), ]; for (ip, label) in blocked { assert!(is_blocked_ip(*ip), "{label} ({ip}) must be blocked"); @@ -281,6 +311,10 @@ mod tests { IpAddr::V6(Ipv6Addr::new(0x2606, 0x2800, 0x220, 1, 0, 0, 0, 1)), "public v6", ), + ( + IpAddr::V6(Ipv6Addr::new(0, 0, 0, 0, 0, 0xffff, 0x0808, 0x0808)), + "::ffff:8.8.8.8 public mapped", + ), ]; for (ip, label) in allowed { assert!(!is_blocked_ip(*ip), "{label} ({ip}) must be allowed"); diff --git a/src/mux/ps.rs b/src/mux/ps.rs index 1122baf..df7ef83 100644 --- a/src/mux/ps.rs +++ b/src/mux/ps.rs @@ -351,10 +351,10 @@ fn parse_pes_packet(data: &[u8]) -> Option { // non-conformant packet that sets the flags but declares a too-short // header would otherwise read payload bytes as a bogus timestamp. if pts_dts_flags >= 2 && header_data_len >= 5 && data.len() >= 14 { - pts = Some(parse_pts(&data[9..14])); + pts = parse_pts(&data[9..14]); } if pts_dts_flags == 3 && header_data_len >= 10 && data.len() >= 19 { - dts = Some(parse_pts(&data[14..19])); + dts = parse_pts(&data[14..19]); } let payload = &data[header_end..]; @@ -393,15 +393,21 @@ fn parse_pes_packet(data: &[u8]) -> Option { /// byte3: [pts 14..7:8] /// byte4: [pts 6..0:7][marker:1] /// ``` -fn parse_pts(buf: &[u8]) -> u64 { +fn parse_pts(buf: &[u8]) -> Option { debug_assert!(buf.len() >= 5); + // Validate the three marker bits (bit 0 of bytes 0, 2, 4) per MPEG-2 + // Systems Table 2-17. A timestamp with a cleared marker is malformed — + // matching ts.rs::parse_timestamp, reject it rather than decode garbage. + if (buf[0] & 0x01) == 0 || (buf[2] & 0x01) == 0 || (buf[4] & 0x01) == 0 { + return None; + } let b0 = buf[0] as u64; let b1 = buf[1] as u64; let b2 = buf[2] as u64; let b3 = buf[3] as u64; let b4 = buf[4] as u64; - ((b0 >> 1) & 0x07) << 30 | b1 << 22 | (b2 >> 1) << 15 | b3 << 7 | b4 >> 1 + Some(((b0 >> 1) & 0x07) << 30 | b1 << 22 | (b2 >> 1) << 15 | b3 << 7 | b4 >> 1) } #[cfg(test)] @@ -748,7 +754,7 @@ mod tests { fn pts_zero() { // PTS = 0 encoded let pts = parse_pts(&encode_pts(0, 0x20)); - assert_eq!(pts, 0); + assert_eq!(pts, Some(0)); } #[test] @@ -757,7 +763,7 @@ mod tests { let val: u64 = (1 << 32) - 1; // 0xFFFFFFFF let encoded = encode_pts(val, 0x20); let decoded = parse_pts(&encoded); - assert_eq!(decoded, val); + assert_eq!(decoded, Some(val)); } // --- DVD PID mapping (track-routing collision regression) --- @@ -886,7 +892,23 @@ mod tests { // The PTS field is exactly 33 bits; 2^33-1 must round-trip — a // truncated shift/mask would lose the top bits. let max = (1u64 << 33) - 1; - assert_eq!(parse_pts(&encode_pts(max, 0x20)), max); + assert_eq!(parse_pts(&encode_pts(max, 0x20)), Some(max)); + } + + #[test] + fn parse_pts_rejects_bad_marker_bits() { + // A timestamp with any marker bit (bit 0 of bytes 0/2/4) cleared is + // malformed and must be rejected, matching ts.rs::parse_timestamp. + let mut buf = encode_pts(90000, 0x20); + assert!(parse_pts(&buf).is_some()); + buf[0] &= !0x01; + assert_eq!(parse_pts(&buf), None); + let mut buf = encode_pts(90000, 0x20); + buf[2] &= !0x01; + assert_eq!(parse_pts(&buf), None); + let mut buf = encode_pts(90000, 0x20); + buf[4] &= !0x01; + assert_eq!(parse_pts(&buf), None); } // ── pack header (0xBA) framing ──────────────────────────────────────── diff --git a/src/mux/resolve.rs b/src/mux/resolve.rs index 2f365d4..cead84f 100644 --- a/src/mux/resolve.rs +++ b/src/mux/resolve.rs @@ -179,6 +179,25 @@ fn validate_network_addr(addr: &str) -> io::Result<()> { } .into()); } + // Split host:port on the LAST ':' so a bracketed IPv6 literal + // (`[2001:db8::1]:9000`) splits at the port colon, not an address colon. + // Require the port substring to be a non-empty u16 — `host:` (empty) and + // `host:abc` (non-numeric) are invalid, despite containing ':'. + let port = match addr.rsplit_once(':') { + Some((_host, port)) => port, + None => { + return Err(crate::error::Error::StreamUrlMissingPort { + addr: addr.to_string(), + } + .into()); + } + }; + if port.is_empty() || port.parse::().is_err() { + return Err(crate::error::Error::StreamUrlInvalid { + url: addr.to_string(), + } + .into()); + } Ok(()) } @@ -213,6 +232,16 @@ fn aacs_key_missing(raw: bool, has_aacs: bool, keys: &crate::decrypt::DecryptKey !raw && has_aacs && matches!(keys, crate::decrypt::DecryptKeys::None) } +/// CSS analogue of [`aacs_key_missing`]. Returns `true` when decryption is +/// requested (`!raw`), the disc is CSS-encrypted (`has_css`), and per-title key +/// resolution yielded no usable key (`keys` is +/// [`crate::decrypt::DecryptKeys::None`] — e.g. a multi-VTS DVD whose chosen +/// title's VTS could not be re-cracked). Muxing that would emit scrambled +/// ciphertext, so the caller fails fast with [`Error::CssKeyMissing`]. +fn css_key_missing(raw: bool, has_css: bool, keys: &crate::decrypt::DecryptKeys) -> bool { + !raw && has_css && matches!(keys, crate::decrypt::DecryptKeys::None) +} + /// Open a PES input stream (produces PES frames). pub fn input(url: &str, opts: &InputOptions) -> io::Result> { let parsed = parse_url(url); @@ -276,13 +305,29 @@ pub fn input(url: &str, opts: &InputOptions) -> io::Result disc.decrypt_keys_for_title(idx, &mut crack_reader, 64), + Err(_) => disc.decrypt_keys(), + }; + // CSS no-key guard (parallel to the AACS gate above): on a CSS + // disc, decrypt_keys_for_title may return `None` when the chosen + // title's VTS could not be re-cracked. Muxing that would emit + // scrambled ciphertext verbatim, so fail loudly here instead. + if css_key_missing(opts.raw, disc.css.is_some(), &keys) { + return Err(crate::error::Error::CssKeyMissing.into()); + } // Correct TrueHD channel counts (MPLS understates 7.1/Atmos as 5.1) // by probing the first DECRYPTED access units of the chosen title. // A fresh reader avoids disturbing the mux reader below. Skipped in // --raw mode: the probe would re-open + decrypt for nothing (on an // AACS disc with no key the correction is a no-op on ciphertext, and // raw output isn't decoded anyway). - let keys = disc.decrypt_keys(); if !opts.raw { match crate::io::file_sector_source::FileSectorSource::open(path) { Ok(probe) => { @@ -593,6 +638,7 @@ fn build_m2ts_pipeline( #[cfg(test)] mod tests { use super::aacs_key_missing; + use super::css_key_missing; use super::validate_network_addr; use super::{build_demux_state, build_iso_pipeline, input, output}; use crate::decrypt::DecryptKeys; @@ -612,6 +658,26 @@ mod tests { assert!(validate_network_addr("host:9000").is_ok()); } + #[test] + fn validate_network_addr_requires_numeric_port() { + // An empty port (`host:`) and a non-numeric port (`host:abc`) both + // contain ':' but are NOT valid host:port — must be rejected. + assert!(validate_network_addr("host:").is_err()); + assert!(validate_network_addr("127.0.0.1:").is_err()); + assert!(validate_network_addr("host:abc").is_err()); + assert!(validate_network_addr("host:99x").is_err()); + // Out-of-u16-range port is rejected (parse:: fails). + assert!(validate_network_addr("host:70000").is_err()); + // Bracketed IPv6 with a valid port passes; split on the LAST ':' so the + // address colons are not mistaken for the port separator. + assert!(validate_network_addr("[2001:db8::1]:9000").is_ok()); + // Bracketed IPv6 WITHOUT a port is rejected (port substring not a u16). + assert!(validate_network_addr("[2001:db8::1]").is_err()); + // Valid numeric port (incl. 0 and max u16) passes. + assert!(validate_network_addr("host:0").is_ok()); + assert!(validate_network_addr("host:65535").is_ok()); + } + fn aacs_keys() -> DecryptKeys { DecryptKeys::Aacs { unit_keys: vec![(1, [0x11u8; 16])], @@ -644,6 +710,31 @@ mod tests { assert!(!aacs_key_missing(false, false, &css_keys())); } + #[test] + fn css_no_key_aborts() { + // CSS disc, decryption requested, per-title resolver yielded None + // (e.g. an un-re-crackable VTS) → abort instead of muxing ciphertext. + assert!(css_key_missing(false, true, &DecryptKeys::None)); + } + + #[test] + fn css_with_key_proceeds() { + // CSS disc with a resolved title key → proceed. + assert!(!css_key_missing(false, true, &css_keys())); + } + + #[test] + fn css_raw_never_aborts() { + // --raw skips decryption: never abort even with no CSS key. + assert!(!css_key_missing(true, true, &DecryptKeys::None)); + } + + #[test] + fn css_guard_ignores_non_css() { + // No CSS state (AACS / unencrypted): the CSS guard never fires. + assert!(!css_key_missing(false, false, &DecryptKeys::None)); + } + #[test] fn raw_never_aborts() { // --raw skips decryption — must never hit the no-key abort, even on an diff --git a/src/mux/ts.rs b/src/mux/ts.rs index cf1092b..552e5f6 100644 --- a/src/mux/ts.rs +++ b/src/mux/ts.rs @@ -44,6 +44,13 @@ struct PesAssembler { /// HEVC/H264 that reads as a spurious start code / corrupt slice /// payload. Tracks how many header bytes remain across packets. header_remaining: usize, + /// 4-bit continuity_counter of the last payload-bearing TS packet seen + /// on this PID. A non-PUSI continuation whose CC is not `(prev + 1) & 0xf` + /// — or whose adaptation field flags a discontinuity — means one or more + /// TS packets for this PID were dropped; splicing the new payload onto the + /// partial PES would inject corrupt bytes. The partial PES is dropped and + /// the assembler resyncs on the next PUSI. `None` until the first packet. + last_cc: Option, } /// Initial capacity for a fresh PES buffer. Sized to cover the @@ -76,6 +83,7 @@ impl PesAssembler { dts: None, active: false, header_remaining: 0, + last_cc: None, } } @@ -288,6 +296,37 @@ impl TsDemuxer { let payload = &ts[payload_start..]; + // Continuity check. The 4-bit continuity_counter increments by 1 on + // every payload-bearing packet of a PID; a gap means dropped TS + // packets. The adaptation field's discontinuity_indicator (first AF + // byte, bit 0x80) explicitly flags an intentional break. On a non-PUSI + // continuation that is discontinuous, the partial PES has a hole in it + // — splicing the new payload would corrupt the elementary stream — so + // drop the partial and resync on the next PUSI. + let cc = ts[3] & 0x0f; + let discontinuity_flag = + (adaptation == 0x03 || adaptation == 0x02) && ts[4] > 0 && (ts[5] & 0x80) != 0; + // A gap is a CC that is neither the expected `(prev + 1) & 0xf` nor a + // duplicate `prev` (ISO 13818-1 permits a packet to repeat its CC; a + // duplicate is not a loss). Anything else means one or more packets for + // this PID were dropped. + let cc_gap = match asm.last_cc { + Some(prev) => cc != ((prev + 1) & 0x0f) && cc != prev, + None => false, + }; + asm.last_cc = Some(cc); + if !pusi && (discontinuity_flag || cc_gap) && asm.active { + tracing::trace!( + target: "mux", + pid = asm.pid, + "TS continuity break on non-PUSI continuation; dropping partial PES", + ); + asm.buffer.clear(); + asm.active = false; + asm.header_remaining = 0; + return; + } + if pusi { // `header_len` is the FULL (uncapped) PES-header length: // 0 = malformed (payload is not a PES start), else 6/9+N. @@ -518,16 +557,33 @@ fn collect_psi_section(data: &[u8], target_pid: u16, table_id: u8) -> Option Option= total { section.truncate(total); return Some(section); @@ -697,6 +759,99 @@ mod tests { assert_eq!(parse_timestamp(&bad), None); } + /// Build a 192-byte BD-TS payload packet for `pid` with explicit PUSI and + /// continuity_counter, carrying `payload` (truncated/padded to 184 bytes, + /// payload-only adaptation). + fn ts_payload_packet(pid: u16, pusi: bool, cc: u8, payload: &[u8]) -> Vec { + let mut pkt = vec![0u8; BD_TS_PACKET_SIZE]; + pkt[4] = SYNC_BYTE; + pkt[5] = ((pid >> 8) as u8) & 0x1F; + if pusi { + pkt[5] |= 0x40; + } + pkt[6] = (pid & 0xFF) as u8; + pkt[7] = 0x10 | (cc & 0x0f); // payload-only adaptation + CC + let n = payload.len().min(184); + pkt[8..8 + n].copy_from_slice(&payload[..n]); + pkt + } + + /// A minimal valid PES start for a video stream id, with no PTS/DTS flags, + /// followed by `es` elementary-stream bytes. header_len = 9. + fn pes_start(es: &[u8]) -> Vec { + let mut v = vec![0x00, 0x00, 0x01, 0xE0, 0x00, 0x00, 0x80, 0x00, 0x00]; + v.extend_from_slice(es); + v + } + + /// Regression (finding 3): a non-PUSI continuation whose continuity_counter + /// is not (prev+1)&0xf means TS packets were dropped — the partial PES has a + /// hole and must be discarded, not spliced. We start a PES (cc=0), then feed + /// a continuation with a CC gap (cc=5 instead of 1); the assembler drops the + /// partial. A clean follow-on PUSI then produces exactly that next PES, + /// proving the corrupt splice didn't happen. + #[test] + fn continuity_gap_drops_partial_pes() { + let pid = 0x1011; + let mut demux = TsDemuxer::new(&[pid]); + + // Start a PES (cc=0) carrying "AAAA". + let mut out = demux.feed(&ts_payload_packet(pid, true, 0, &pes_start(b"AAAA"))); + assert!( + out.is_empty(), + "first PES still open, nothing completed yet" + ); + + // Discontinuous continuation (cc jumps 0 -> 5) carrying "BBBB". The gap + // must drop the partial PES rather than append "BBBB". + out = demux.feed(&ts_payload_packet(pid, false, 5, b"BBBB")); + assert!(out.is_empty(), "dropped partial PES is not emitted here"); + + // A fresh PUSI (cc=6) starts the next PES "CCCC"; starting it would + // normally flush the previous one — but it was dropped, so nothing is + // flushed yet. + out = demux.feed(&ts_payload_packet(pid, true, 6, &pes_start(b"CCCC"))); + assert!( + out.is_empty(), + "the dropped partial must NOT be flushed by the next PUSI" + ); + + // Flush: only the clean "CCCC" PES comes out — it must NOT begin with + // the dropped "AAAA" payload. (Payload-only packets pad to 184 bytes, + // so compare the leading ES bytes, not the whole padded buffer.) + let final_out = demux.flush(); + assert_eq!(final_out.len(), 1, "exactly one clean PES"); + assert_eq!( + &final_out[0].data[..4], + b"CCCC", + "surviving PES is the clean one, not the dropped partial" + ); + // The dropped "BBBB" continuation must not have been spliced anywhere. + assert!( + !final_out[0].data.windows(4).any(|w| w == b"BBBB"), + "dropped continuation must not appear in any emitted PES" + ); + } + + /// In-sequence continuation (cc 0 -> 1) must still splice normally — the + /// continuity check must not break the happy path. + #[test] + fn continuity_in_sequence_splices() { + let pid = 0x1011; + let mut demux = TsDemuxer::new(&[pid]); + demux.feed(&ts_payload_packet(pid, true, 0, &pes_start(b"AAAA"))); + demux.feed(&ts_payload_packet(pid, false, 1, b"BBBB")); + let out = demux.flush(); + assert_eq!(out.len(), 1); + // First payload's ES leads, and the in-sequence continuation's "BBBB" + // is present (spliced) — the padding zeros sit between them. + assert_eq!(&out[0].data[..4], b"AAAA", "first PES ES leads"); + assert!( + out[0].data.windows(4).any(|w| w == b"BBBB"), + "in-sequence continuation must be spliced in" + ); + } + #[test] fn test_demuxer_empty() { let mut demux = TsDemuxer::new(&[0x1011]); @@ -984,7 +1139,12 @@ mod tests { let tail = §ion[head_len..]; assert!(!tail.is_empty(), "test must actually span two packets"); p1[..tail.len()].copy_from_slice(tail); - let pkt1 = bdts_packet(p1, pmt_pid, false); + let mut pkt1 = bdts_packet(p1, pmt_pid, false); + // Continuity counter must increment from the PUSI packet (CC=0) to its + // continuation (CC=1) — `collect_psi_section` rejects a CC gap as a + // desync. The CC lives in the low nibble of TS-header byte 4 (offset 7 + // here, after the 4-byte BD-TS timecode prefix). + pkt1[7] = (pkt1[7] & 0xF0) | 0x01; let mut out = pkt0; out.extend(pkt1); @@ -1025,6 +1185,35 @@ mod tests { ); } + /// Regression for the PSI continuity-counter guard: a continuation packet + /// whose CC does NOT increment from the PUSI packet is a desync (dropped or + /// reordered packet). `collect_psi_section` must abandon that assembly + /// rather than splice misordered payload. Here the only continuation has a + /// bad CC, so the section never completes and no streams are found. + #[test] + fn scan_streams_rejects_pmt_with_cc_desync() { + let pmt_pid = 0x0100; + let mut entries: Vec<(u8, u16)> = Vec::new(); + entries.push((0x1B, 0x1011)); + for i in 0..40u16 { + entries.push((0x80, 0x1100 + i)); + } + let mut pmt = pmt_two_packets(pmt_pid, &entries); + // Corrupt the continuation packet's CC. pmt is exactly two BD-TS + // packets; the second starts at BD_TS_PACKET_SIZE. Its CC (low nibble + // of offset+7) was set to 1 by pmt_two_packets; flip it to a gap (5). + let cc_off = BD_TS_PACKET_SIZE + 7; + pmt[cc_off] = (pmt[cc_off] & 0xF0) | 0x05; + + let mut data = pat_packet(pmt_pid); + data.extend(pmt); + // The PMT section can't be reassembled (CC gap) → no program found. + assert!( + scan_streams(&data).is_none(), + "a CC-desynced PMT continuation must not yield streams" + ); + } + // ════════════════════════════════════════════════════════════════════ // Added hardening tests // ════════════════════════════════════════════════════════════════════ diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs index 4cab95f..6777a11 100644 --- a/src/scsi/windows.rs +++ b/src/scsi/windows.rs @@ -268,6 +268,16 @@ impl ScsiTransport for SptiTransport { DataDirection::FromDevice => SCSI_IOCTL_DATA_IN, DataDirection::ToDevice => SCSI_IOCTL_DATA_OUT, }; + // Match the macOS/Linux guard: a >=4 GiB buffer would wrap when cast to + // u32 below, producing a short transfer reported as success with the + // wrong byte count. + if data.len() > u32::MAX as usize { + return Err(Error::ScsiError { + opcode: cdb.first().copied().unwrap_or(0), + status: super::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, + }); + } sptwb.spt.DataTransferLength = data.len() as u32; // Round up to the next whole second so a 1500ms request gets at // least 2s, not 1s. SPTI's TimeOutValue is u32 seconds with no @@ -335,7 +345,10 @@ impl ScsiTransport for SptiTransport { Ok(ScsiResult { status: sptwb.spt.ScsiStatus, - bytes_transferred: sptwb.spt.DataTransferLength as usize, + // 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()), sense, }) } diff --git a/src/sector/decrypting.rs b/src/sector/decrypting.rs index dafa5c9..c9d94af 100644 --- a/src/sector/decrypting.rs +++ b/src/sector/decrypting.rs @@ -93,6 +93,18 @@ impl SectorSource for DecryptingSectorSource { buf: &mut [u8], recovery: bool, ) -> Result { + // Defense-in-depth: AACS aligned units are 3 sectors (6144 bytes) and + // `decrypt_sectors` anchors units at buffer offset 0. A read whose START + // LBA is not unit-aligned (lba % 3 != 0) would decrypt every unit under + // the wrong CBC/unit alignment and silently mis-decrypt. Reject loud + // (DecryptFailed) BEFORE reading rather than ever mis-decrypting — callers + // (e.g. the multipass sweep) must issue unit-aligned reads. + if matches!(self.keys, DecryptKeys::Aacs { .. }) { + const UNIT_SECTORS: u32 = (crate::aacs::ALIGNED_UNIT_LEN / 2048) as u32; // 3 + if lba % UNIT_SECTORS != 0 { + return Err(crate::error::Error::DecryptFailed); + } + } let n = self.inner.read_sectors(lba, count, buf, recovery)?; // Apply the crate-wide AACS/CSS/None decrypt entry point in-place // over the bytes just read. No-op for DecryptKeys::None. @@ -557,6 +569,62 @@ mod tests { ); } + /// Defense-in-depth: an AACS decrypting read whose START LBA is not + /// unit-aligned (lba % 3 != 0) must be rejected with DecryptFailed BEFORE + /// touching the cipher — a mid-unit start would decrypt every unit under the + /// wrong CBC/unit alignment and silently mis-decrypt. A unit-aligned start + /// (lba % 3 == 0) must pass the guard and proceed normally. + /// + /// Grounding: the `lba % UNIT_SECTORS != 0` guard in `read_sectors`. + #[test] + fn aacs_unaligned_start_lba_rejected() { + let keys = DecryptKeys::Aacs { + unit_keys: vec![(0u32, [0u8; 16])], + read_data_key: None, + }; + // Unaligned starts (1, 2, 4, 5, 32 — note 32 % 3 == 2) must all reject. + for lba in [1u32, 2, 4, 5, 32, 64] { + let mut wrapped = DecryptingSectorSource::new(ClearUnitSource, keys.clone()); + let mut buf = vec![0u8; 3 * 2048]; + let r = wrapped.read_sectors(lba, 3, &mut buf, false); + let err = r.expect_err("unaligned AACS start LBA must reject"); + assert_eq!( + err.code(), + crate::error::Error::DecryptFailed.code(), + "lba {lba} (% 3 = {}) must reject with DecryptFailed", + lba % 3 + ); + } + // Unit-aligned starts (0, 3, 33, 66) must pass the guard. ClearUnitSource + // yields TS-clear units, so decrypt is a no-op and the read succeeds. + for lba in [0u32, 3, 33, 66] { + let mut wrapped = DecryptingSectorSource::new(ClearUnitSource, keys.clone()); + let mut buf = vec![0u8; 3 * 2048]; + let n = wrapped + .read_sectors(lba, 3, &mut buf, false) + .unwrap_or_else(|_| panic!("aligned lba {lba} must pass the guard")); + assert_eq!(n, 3 * 2048); + } + } + + /// The unit-alignment guard is AACS-only. A CSS decrypting read (per-sector, + /// stateless — DVDs) must NOT be gated on a 3-sector boundary: a single + /// sector at lba 1 must read fine. Grounding: the guard is inside + /// `matches!(self.keys, DecryptKeys::Aacs { .. })`. + #[test] + fn css_start_lba_not_unit_gated() { + let mut wrapped = DecryptingSectorSource::new( + ClearUnitSource, + DecryptKeys::Css { + title_key: [0u8; 5], + }, + ); + let mut buf = vec![0u8; 2048]; + // lba 1 (not a multiple of 3) must succeed under CSS — no AACS gate. + let n = wrapped.read_sectors(1, 1, &mut buf, false).unwrap(); + assert_eq!(n, 2048, "CSS reads must not be unit-alignment gated"); + } + /// `into_inner` / `inner` / `inner_mut` must hand back the original /// source unchanged. Grounding: the accessor methods. #[test] diff --git a/tests/crypto_tests.rs b/tests/crypto_tests.rs index eb81f2c..afe9338 100644 --- a/tests/crypto_tests.rs +++ b/tests/crypto_tests.rs @@ -16,6 +16,7 @@ use libfreemkv::css; fn css_descramble_sector_roundtrip_via_public_api() { let state = css::CssState { title_key: [0x42, 0x13, 0x37, 0xBE, 0xEF], + crack_span: None, }; let mut sector = vec![0xAAu8; 2048];