diff --git a/src/aacs/resolve.rs b/src/aacs/resolve.rs index 1009d42..82054fc 100644 --- a/src/aacs/resolve.rs +++ b/src/aacs/resolve.rs @@ -379,26 +379,18 @@ fn resolve_keys_classical(ctx: &ResolveContext<'_>, version: AacsVersion) -> Opt // One AES-D + magic check per candidate (cheap). mk_dv is hoisted // out of the loop so the MKB is not re-walked per candidate. let mks = providers.media_keys(); - let mut mk_hits: Vec<[u8; 16]> = Vec::new(); - if let Some(mk_dv) = mkb_find_mk_dv(mkb) { - for mk in &mks { - let verifies = aes_ecb_decrypt(mk, &mk_dv)[..8] - == [0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF]; - if verifies && !mk_hits.contains(mk) { - mk_hits.push(*mk); - if mk_hits.len() > 1 { - break; // ambiguous — bail to avoid a wrong key - } - } - } - } - if mk_hits.len() == 1 { - let vuk = derive_vuk(&mk_hits[0], ctx.volume_id); + let chosen_mk = mkb_find_mk_dv(mkb).and_then(|mk_dv| { + unique_verifying_mk(&mks, |mk| { + aes_ecb_decrypt(mk, &mk_dv)[..8] == MK_VERIFY_MAGIC + }) + }); + if let Some(mk) = chosen_mk { + let vuk = derive_vuk(&mk, ctx.volume_id); tracing::debug!(target: "freemkv::disc", phase = "resolve_keys_path2_5_hit", mk_pool = mks.len(), "media key from keydb MK-pool brute (km_verifies)"); // Same class as path 3 (KEYDB MK → derived VUK). return Some(build(Some(vuk), derive_uks(&vuk), 3)); } - tracing::debug!(target: "freemkv::disc", phase = "resolve_keys_path2_5_miss", mk_pool = mks.len(), mk_hits = mk_hits.len(), "MK-pool brute: no unique verifying MK"); + tracing::debug!(target: "freemkv::disc", phase = "resolve_keys_path2_5_miss", mk_pool = mks.len(), "MK-pool brute: no unique verifying MK"); } else { tracing::debug!(target: "freemkv::disc", phase = "resolve_keys_no_mkb", "no MKB; paths 1/2 skipped"); } @@ -450,6 +442,42 @@ fn resolve_keys_classical(ctx: &ResolveContext<'_>, version: AacsVersion) -> Opt None } +/// First 8 bytes of the plaintext behind an MKB Verify Media Key record — the +/// AACS "this is the right Km" sentinel (`0123456789ABCDEF`). A candidate MK +/// verifies when AES-128-ECB-D(mk, mk_dv) starts with it. +const MK_VERIFY_MAGIC: [u8; 8] = [0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF]; + +/// The MK-pool selection rule of path 2.5, split out of [`resolve_keys_v1`] so +/// the ambiguity guard has a reachable test. +/// +/// `verifies` is the MKB check — in production +/// `AES-D(mk, mk_dv)[..8] == MK_VERIFY_MAGIC`. Returns a Media Key only when +/// EXACTLY ONE DISTINCT candidate passes. Duplicates of the same key are one +/// candidate (a pool aggregated across providers routinely repeats a key), but +/// two DIFFERENT keys that both verify mean the pool cannot say which is this +/// disc's Km: picking either derives a wrong VUK, and a wrong VUK decrypts to +/// plausible-looking garbage rather than failing loudly. Bail and let the +/// later hash/VID paths answer instead. +/// +/// The predicate is a parameter rather than the inlined AES check because a +/// genuine two-key multi-hit cannot be synthesised: it needs one ciphertext +/// that decrypts under two distinct AES-128 keys to plaintexts sharing a +/// 64-bit prefix — a 2^64 search. Injecting the verifier is the only way the +/// ambiguity branch is reachable from a test at all. +fn unique_verifying_mk(mks: &[[u8; 16]], verifies: impl Fn(&[u8; 16]) -> bool) -> Option<[u8; 16]> { + let mut hits: Vec<[u8; 16]> = Vec::new(); + for mk in mks { + if verifies(mk) && !hits.contains(mk) { + hits.push(*mk); + if hits.len() > 1 { + // Ambiguous — bail rather than pick a Media Key. + return None; + } + } + } + hits.first().copied() +} + /// For path 5: cross-reference the disc's `Unit_Key_RO.inf` CPS-unit /// numbering against the KEYDB entry's pre-decrypted unit keys. Every /// CPS unit the disc declares must have a matching entry in KEYDB; @@ -1300,6 +1328,62 @@ mod tests { "VUK must derive from the verified Km + this disc's VID" ); } + + /// Path 2.5's ambiguity guard: when MORE THAN ONE DISTINCT pooled Media Key + /// verifies against the MKB, the resolver must return no key at all rather + /// than pick one. A wrong Km derives a wrong VUK, and a wrong VUK does not + /// fail loudly — it decrypts the title to garbage that muxes and plays as a + /// corrupt rip. + /// + /// The real MKB check cannot be forced into a multi-hit: two distinct + /// AES-128 keys decrypting one `mk_dv` to plaintexts that share the 64-bit + /// verify magic is a 2^64 search, not a fixture. So the rule is tested + /// through `unique_verifying_mk`, whose verifier is a parameter — the same + /// function `resolve_keys_v1` calls, with the same pool semantics. + #[test] + fn mk_pool_ambiguity_bails_rather_than_picking_a_media_key() { + let a = [0xAAu8; 16]; + let b = [0xBBu8; 16]; + let c = [0xCCu8; 16]; + + // One verifying candidate → that key. + assert_eq!( + unique_verifying_mk(&[a, b, c], |mk| *mk == b), + Some(b), + "a single verifying MK resolves" + ); + + // The SAME key repeated across providers is one candidate, not an + // ambiguity — the dedup (`!hits.contains`) must keep this resolvable. + assert_eq!( + unique_verifying_mk(&[b, b, b], |mk| *mk == b), + Some(b), + "duplicates of one key are not ambiguity" + ); + + // TWO DISTINCT verifying candidates → bail, no key. + assert_eq!( + unique_verifying_mk(&[a, b], |mk| *mk == a || *mk == b), + None, + "two distinct verifying MKs must yield NO key, not the first one" + ); + + // Ambiguity must still be detected when the second hit is last in the + // pool, i.e. the scan may not stop at the first hit. + assert_eq!( + unique_verifying_mk(&[a, c, [0u8; 16], b], |mk| *mk == a || *mk == b), + None, + "a late second hit is still ambiguous" + ); + + // Every candidate verifying is the degenerate ambiguous case. + assert_eq!(unique_verifying_mk(&[a, b, c], |_| true), None); + + // No candidate verifies → no key (and no panic on an empty pool). + assert_eq!(unique_verifying_mk(&[a, b, c], |_| false), None); + assert_eq!(unique_verifying_mk(&[], |_| true), None); + } + #[test] fn test_content_cert_parse() { // AACS 1.0 cert, bus encryption OFF. Content-cert layout: flag in diff --git a/src/io/file_sector_source/mod.rs b/src/io/file_sector_source/mod.rs index bb8c865..ec2f1b0 100644 --- a/src/io/file_sector_source/mod.rs +++ b/src/io/file_sector_source/mod.rs @@ -434,19 +434,36 @@ mod tests { // Additional coverage. // --------------------------------------------------------------- - /// `count == 0` must short-circuit to Ok(0) WITHOUT seeking or - /// reading, even at an out-of-range LBA — the early-return guard - /// runs before any I/O. Grounding: `if count == 0 { return Ok(0) }`. + /// `count == 0` must short-circuit to Ok(0) WITHOUT seeking or reading, + /// even at an out-of-range LBA. Grounding: `if count == 0 { return Ok(0) }`. + /// + /// The `Ok(0)` return alone proves nothing: with the guard deleted, a seek + /// past EOF succeeds (POSIX permits seeking beyond the end of a file) and a + /// zero-length `read_exact` returns `Ok(())` immediately, so the call still + /// returns `Ok(0)`. The observable difference is the file's cursor — the + /// seek MOVES it to `lba * 2048`. Assert on that, so the guard is what the + /// test is actually measuring. #[test] fn zero_count_returns_zero_no_io() { let dir = tempdir().unwrap(); let path = dir.path().join("zc.iso"); make_iso(&path, 4); let mut src = FileSectorSource::open(&path).unwrap(); + let before = src.file.stream_position().expect("cursor readable"); + assert_eq!(before, 0, "a freshly opened file starts at offset 0"); // LBA far past EOF — must not matter because count==0 returns early. let mut buf = [0u8; 1]; let n = src.read_sectors(1_000_000, 0, &mut buf, false).unwrap(); assert_eq!(n, 0); + assert_eq!( + src.file.stream_position().expect("cursor readable"), + before, + "count == 0 must return before the seek — an unmoved cursor is the \ + only observable proof that no I/O was issued" + ); + // And the drop-window accounting must not have advanced either. + assert_eq!(src.bytes_read_since_drop, 0); + assert_eq!(src.drop_window_start, 0); } /// Reading past EOF must ERROR (read_exact's UnexpectedEof), never diff --git a/src/mux/codec/truehd.rs b/src/mux/codec/truehd.rs index 72eea55..f8eda54 100644 --- a/src/mux/codec/truehd.rs +++ b/src/mux/codec/truehd.rs @@ -1638,22 +1638,43 @@ mod tests { assert_eq!(parser.buf.len(), 100, "partial AU retained"); } + /// Largest AU the 12-bit length field can declare: 0xFFF words × 2. + const MAX_AU_BYTES: usize = 0xFFF * 2; // 8190 + #[test] fn buffer_stays_bounded_across_many_partial_pes() { // Malformed/never-completing input must keep the reassembly buffer - // bounded by MAX_TRUEHD_BUF. Repeatedly feed AU fragments whose declared - // length always exceeds what is buffered, so no AU ever completes; the - // post-loop cap guard must clear the buffer instead of letting it grow - // unbounded across many calls. + // bounded across an unbounded number of PES packets. + // + // The bound that actually holds is MAX_AU_BYTES, not MAX_TRUEHD_BUF: + // `parse`'s loop only breaks with data retained when + // `self.buf.len() < unit_bytes`, and `unit_bytes` is + // `((buf[0] << 8 | buf[1]) & 0xFFF) * 2 <= 8190`. Every other exit + // drains. So the post-loop `buf.len() > MAX_TRUEHD_BUF` cap (256 KiB) is + // an unreachable backstop — an exhaustive sweep of all 65536 two-byte + // AU heads × fragment sizes {3, 5, 100, 4096, 8189, 65535} over 20 PES + // each peaks at 8189 bytes. Asserting only `<= MAX_TRUEHD_BUF` is + // therefore vacuous; assert the reachable ceiling instead. + // + // Fixture: heads of 0xFF 0xFF (masked to 0xFFF words = 8190 bytes + // declared — this also exercises the 12-bit mask) with 8189 bytes + // present, so each PES leaves the buffer one byte short of a complete + // AU. The previous fixture used 4096-byte fragments, which completed an + // AU every second call and never loaded the buffer past ~4 KiB. let mut parser = TrueHdParser::new(); - // Each PES: a head declaring 0xFFF words (8190 bytes) but only 4096 bytes - // present → incomplete → retained. Across many PES this would accumulate - // without the cap. + let mut worst = 0usize; for _ in 0..200 { - let mut frag = vec![0u8; 4096]; - frag[0] = 0x0F; // 0x0FFF words = 4095 → 8190 bytes declared + let mut frag = vec![0u8; MAX_AU_BYTES - 1]; + frag[0] = 0xFF; frag[1] = 0xFF; let _ = parser.parse(&make_pes(frag, Some(0))); + worst = worst.max(parser.buf.len()); + assert!( + parser.buf.len() < MAX_AU_BYTES, + "reassembly buffer exceeded the AU-length ceiling: {} >= {}", + parser.buf.len(), + MAX_AU_BYTES + ); assert!( parser.buf.len() <= MAX_TRUEHD_BUF, "reassembly buffer exceeded cap: {} > {}", @@ -1661,6 +1682,12 @@ mod tests { MAX_TRUEHD_BUF ); } + // The fixture must genuinely load the buffer, not self-drain: if this + // trips, the test is measuring nothing. + assert!( + worst >= MAX_AU_BYTES - 8, + "fixture must drive the buffer to the ceiling, peaked at {worst}" + ); } // --- ac3_boundary_corroborated: the AC-3-vs-TrueHD disambiguation --- diff --git a/src/mux/mp4/mod.rs b/src/mux/mp4/mod.rs index 4efdd58..5632dc8 100644 --- a/src/mux/mp4/mod.rs +++ b/src/mux/mp4/mod.rs @@ -1462,6 +1462,33 @@ mod tests { r >= 12 << 20 && r <= 20 << 20, "≈12-16 MB for a 2h feature, got {r}" ); + + // The case above is dominated by RESERVE_FLOOR + RESERVE_BUFFER: its + // per-sample term is ~6.4 MB, under the 8 MiB floor, so setting + // BYTES_PER_SAMPLE to 0 would leave it green. Pin a case where the + // per-sample estimate is what the result is MADE of. + // + // 2 hr, 23.976 fps HEVC + EIGHT AC-3 tracks (a commentary-heavy disc): + // video 7200 × 24000/1001 = 172_627 samples + // audio 8 × 7200 × (48000 / 1536) = 1_800_000 samples + // total 1_972_627 × 16 B = 31_562_032 B (30.1 MiB) + // → round up to 8 grains = 32 MiB, + 4 MiB buffer = 36 MiB exactly. + // With BYTES_PER_SAMPLE = 0 this collapses to the 12 MiB floor+buffer. + let mut streams = vec![hevc_video()]; + streams.extend((0..8).map(|_| audio(Codec::Ac3, "eng"))); + let mut t = title(streams, vec![]); + t.duration_secs = 7200.0; + let included: Vec = (0..9).collect(); + let r = estimate_reserve(&t, &included); + assert_eq!( + r, + 36 << 20, + "per-sample term must dominate: 1.97M samples × 16 B → 32 MiB + 4 MiB buffer" + ); + assert!( + r > RESERVE_FLOOR + RESERVE_BUFFER, + "this case must NOT be reachable from the floor alone" + ); } #[test] diff --git a/src/scsi/linux.rs b/src/scsi/linux.rs index cb12fa7..3cd0bdc 100644 --- a/src/scsi/linux.rs +++ b/src/scsi/linux.rs @@ -242,17 +242,14 @@ impl ScsiTransport for SgIoTransport { data: &mut [u8], timeout_ms: u32, ) -> Result { - // Guard the entry point: `ScsiTransport` is a pub trait, so an - // external caller could pass an empty CDB. Indexing cdb[0] below - // (and in the error paths) would panic. In-crate callers always - // pass non-empty literal CDBs. - if cdb.is_empty() { - return Err(Error::ScsiError { - opcode: 0, - status: super::SCSI_STATUS_TRANSPORT_FAILURE, - sense: None, - }); - } + // Validate the CDB length at the entry point, BEFORE `cdb[0]` below. + // `ScsiTransport` is a pub trait, so an external caller could pass an + // empty CDB and indexing it would panic; an over-length CDB must be + // rejected rather than truncated (see `checked_cdb_len`). Both checks + // live in the shared helper so they cannot drift per platform — this + // backend used to carry its own bespoke empty-CDB guard, which macOS + // and Windows never had. + let cmd_len = super::checked_cdb_len(cdb, K_MAX_CDB_SIZE)?; let exec_t0 = std::time::Instant::now(); let opcode = cdb[0]; tracing::trace!( @@ -294,14 +291,6 @@ impl ScsiTransport for SgIoTransport { DataDirection::FromDevice => SG_DXFER_FROM_DEV, DataDirection::ToDevice => SG_DXFER_TO_DEV, }; - // Reject an over-length CDB rather than truncating it. This used to be - // `cdb.len().min(16) as u8`, which silently dropped the tail: SPC-4 - // fixes a command's length by its opcode group code, so the shortened - // CDB is a DIFFERENT command, which the drive executes and answers - // with GOOD status and data for a request nobody made. Matches the - // macOS and Windows backends (all three call the same helper). - let cmd_len = super::checked_cdb_len(cdb, K_MAX_CDB_SIZE)?; - let mut sense = [0u8; 32]; let mut hdr: sg_io_hdr = unsafe { std::mem::zeroed() }; hdr.interface_id = b'S' as i32; diff --git a/src/scsi/macos.rs b/src/scsi/macos.rs index 861563f..36e4365 100644 --- a/src/scsi/macos.rs +++ b/src/scsi/macos.rs @@ -13,6 +13,9 @@ //! //! Drive enumeration (`list_drives`) uses the IOKit registry directly via //! `shim_list_drives` — no exclusive access, no SCSI commands, no unmounts. +//! The media-presence probe (`drive_has_disc`) does the same via +//! `shim_media_present`: steps 1-5 above are the *transport* open path, and a +//! probe documented as side-effect-free must not run any of them. use super::{DataDirection, ScsiResult, ScsiTransport}; use crate::error::{Error, Result}; @@ -56,6 +59,21 @@ unsafe extern "C" { transfer_count: *mut u64, ) -> i32; fn shim_list_drives(out: *mut ShimDriveInfo, max_entries: i32) -> i32; + fn shim_media_present(bsd_name: *const u8) -> i32; +} + +/// Strip the `/dev/` (or raw-device `/dev/r`) prefix off a device path, +/// yielding the BSD name the shim's IOKit lookups take. Shared by +/// [`MacScsiTransport::open`] and [`drive_has_disc`] so the two cannot +/// disagree about what device they are talking about. +fn bsd_name_of(device: &Path) -> Result<&str> { + let dev_str = device.to_str().ok_or_else(|| Error::DeviceNotFound { + path: device.display().to_string(), + })?; + Ok(dev_str + .strip_prefix("/dev/r") + .or_else(|| dev_str.strip_prefix("/dev/")) + .unwrap_or(dev_str)) } pub struct MacScsiTransport { @@ -66,17 +84,7 @@ unsafe impl Send for MacScsiTransport {} impl MacScsiTransport { pub fn open(device: &Path) -> Result { - let dev_str = device.to_str().ok_or_else(|| Error::DeviceNotFound { - path: device.display().to_string(), - })?; - - let bsd_name = if let Some(rest) = dev_str.strip_prefix("/dev/r") { - rest - } else if let Some(rest) = dev_str.strip_prefix("/dev/") { - rest - } else { - dev_str - }; + let bsd_name = bsd_name_of(device)?; // Enforce single-instance: the shim's global handle can't back two // live transports safely. Bail rather than corrupt shared state. @@ -256,26 +264,95 @@ fn cstr_to_str(bytes: &[u8]) -> &str { std::str::from_utf8(&bytes[..end]).unwrap_or("") } +/// Media-presence probe, via the IOKit registry only. +/// +/// [`crate::scsi::drive_has_disc`] is documented as the cheap, side-effect-free +/// "is there a disc?" question, suitable for a poll-loop tick. The Linux +/// backend honours that: `open(O_RDWR|O_NONBLOCK)` + one TEST UNIT READY, no +/// exclusive access, no unmount. The Windows backend likewise opens a shared +/// handle and issues one TUR. +/// +/// macOS could not: `MacScsiTransport::open` is the FULL exclusive-transport +/// path, whose first act is `diskutil unmountDisk force` on the target device. +/// So the probe documented as side-effect-free force-unmounted the user's disc +/// — and on every poll tick, taking (and dropping) exclusive access each time. +/// +/// The registry answers the same question with no side effect at all: the +/// IOStorageFamily publishes an IOMedia object for a removable device only +/// while media is present and removes it on eject, so a matching IOMedia is +/// exactly "a disc is in the drive". No SCSI command is issued, which is why +/// no timeout parameter is involved. +/// +/// Trade-off, stated plainly: this reports what the OS has *enumerated*, so a +/// disc that is inserted but still spinning up (no IOMedia published yet) reads +/// as absent for the moment the enumeration takes — the same window in which a +/// TUR would answer "not ready" and this function's contract already maps to +/// `Ok(false)`. pub(super) fn drive_has_disc(path: &Path) -> Result { - let mut transport = MacScsiTransport::open(path)?; - let cdb = [crate::scsi::SCSI_TEST_UNIT_READY, 0, 0, 0, 0, 0]; - let mut buf = [0u8; 0]; - match transport.execute( - &cdb, - crate::scsi::DataDirection::None, - &mut buf, - crate::scsi::TUR_TIMEOUT_MS, - ) { - Ok(_) => Ok(true), - Err(ref e) if e.scsi_sense().is_some_and(|s| s.is_not_ready()) => Ok(false), - Err(e) => Err(e), + let bsd_name = bsd_name_of(path)?; + let mut bsd_c = bsd_name.as_bytes().to_vec(); + bsd_c.push(0); + match unsafe { shim_media_present(bsd_c.as_ptr()) } { + 1 => Ok(true), + 0 => Ok(false), + // -1: IOKit itself is unavailable (IOMainPort / matching-dictionary + // failure). That is not "no disc" — surface it rather than report a + // false negative the caller would act on. + _ => Err(Error::DeviceNotFound { + path: bsd_name.to_string(), + }), } } #[cfg(test)] mod tests { - use super::K_MAX_CDB_SIZE; + use super::{K_MAX_CDB_SIZE, OPEN, bsd_name_of, drive_has_disc}; use crate::error::Error; + use std::path::Path; + use std::sync::atomic::Ordering; + + #[test] + fn bsd_name_strips_dev_and_raw_dev_prefixes() { + assert_eq!(bsd_name_of(Path::new("/dev/disk4")).unwrap(), "disk4"); + assert_eq!(bsd_name_of(Path::new("/dev/rdisk4")).unwrap(), "disk4"); + assert_eq!(bsd_name_of(Path::new("disk4")).unwrap(), "disk4"); + } + + /// `drive_has_disc` is documented as a cheap, side-effect-free presence + /// probe. It used to be implemented by constructing a FULL exclusive + /// transport, whose first act is `diskutil unmountDisk force` on the target + /// device followed by an unconditional `usleep(500000)` — so the probe + /// force-unmounted the user's disc, on every poll tick. + /// + /// Two observables separate the registry probe from the transport open, + /// neither of which needs an optical drive to be attached: + /// + /// 1. It ANSWERS. The transport path returned `Err(DeviceNotFound)` here; + /// the registry path reports "no media" as `Ok(false)`. + /// 2. It is FAST. The transport path's `usleep(500000)` after the spawn is + /// unconditional, so it could not complete inside this budget even when + /// the spawn itself failed. + #[test] + fn presence_probe_does_not_open_a_transport() { + let path = Path::new("/dev/freemkv-no-such-device"); + let t0 = std::time::Instant::now(); + let r = drive_has_disc(path); + let elapsed = t0.elapsed(); + + assert!( + matches!(r, Ok(false)), + "a device with no IOMedia must report absent media, got {r:?}" + ); + assert!( + elapsed < std::time::Duration::from_millis(250), + "probe took {elapsed:?}: the transport path's unconditional 500 ms \ + post-unmount sleep means this budget can only be met without it" + ); + assert!( + !OPEN.load(Ordering::Acquire), + "the probe must not leave the exclusive-transport lock held" + ); + } /// A CDB longer than K_MAX_CDB_SIZE must be rejected with /// `Error::InvalidCdbLength` before the shim is ever called. Exercises the diff --git a/src/scsi/macos_shim.c b/src/scsi/macos_shim.c index b4330d4..9740282 100644 --- a/src/scsi/macos_shim.c +++ b/src/scsi/macos_shim.c @@ -8,6 +8,7 @@ #include #include #include +#include extern char **environ; @@ -33,14 +34,27 @@ static ShimHandle g_handle = {NULL, NULL, NULL, 0}; // ── Registry helpers ────────────────────────────────────────────────────── -static int cfstring_to_cstr(CFStringRef cf, char *buf, size_t buflen) { +// Convert a registry property to a C string. +// +// The value is taken as CFTypeRef, not CFStringRef, and its type is checked +// before use. IORegistryEntryCreateCFProperty / CFDictionaryGetValue return +// whatever the driver published: the IOKit registry contract (Apple, "Accessing +// Hardware From Applications" — Device Access and the I/O Kit) fixes the +// property KEYS, not the CoreFoundation type behind them, and a third-party +// optical driver publishing a CFNumber or CFData for "BSD Name" or "Product +// Revision Level" is legal. CFStringGetCString on a non-CFString aborts the +// process (CFRuntime type assertion) — from inside the public +// scsi::list_drives(), which is documented never to fail. Wrong type → treated +// as absent. +static int cfstring_to_cstr(CFTypeRef cf, char *buf, size_t buflen) { if (!cf) return 0; - if (!CFStringGetCString(cf, buf, buflen, kCFStringEncodingUTF8)) return 0; + if (CFGetTypeID(cf) != CFStringGetTypeID()) return 0; + if (!CFStringGetCString((CFStringRef)cf, buf, buflen, kCFStringEncodingUTF8)) return 0; return 1; } static int registry_entry_bsd_name(io_registry_entry_t entry, char *buf, size_t buflen) { - CFStringRef cf = IORegistryEntryCreateCFProperty(entry, CFSTR("BSD Name"), + CFTypeRef cf = IORegistryEntryCreateCFProperty(entry, CFSTR("BSD Name"), kCFAllocatorDefault, 0); if (!cf) return 0; int ok = cfstring_to_cstr(cf, buf, buflen); @@ -119,19 +133,29 @@ static int bdsvc_to_bsd_name(io_registry_entry_t bdsvc, char *buf, size_t buflen // Given an IOBDServices, extract Device Characteristics strings. static void bdsvc_device_info(io_registry_entry_t bdsvc, ShimDriveInfo *info) { - CFDictionaryRef dc = IORegistryEntryCreateCFProperty(bdsvc, + // "Device Characteristics" is declared a dictionary, but the value is + // driver-published and the registry contract does not enforce the type. + // CFDictionaryGetValue on a non-dictionary aborts the process, so the type + // is checked before it is used as one. Each member string is type-checked + // in turn by cfstring_to_cstr. + CFTypeRef dc = IORegistryEntryCreateCFProperty(bdsvc, CFSTR("Device Characteristics"), kCFAllocatorDefault, 0); if (!dc) return; + if (CFGetTypeID(dc) != CFDictionaryGetTypeID()) { + CFRelease(dc); + return; + } + CFDictionaryRef dict = (CFDictionaryRef)dc; - CFStringRef val; + CFTypeRef val; - val = CFDictionaryGetValue(dc, CFSTR("Vendor Name")); + val = CFDictionaryGetValue(dict, CFSTR("Vendor Name")); if (val) cfstring_to_cstr(val, info->vendor, sizeof(info->vendor)); - val = CFDictionaryGetValue(dc, CFSTR("Product Name")); + val = CFDictionaryGetValue(dict, CFSTR("Product Name")); if (val) cfstring_to_cstr(val, info->model, sizeof(info->model)); - val = CFDictionaryGetValue(dc, CFSTR("Product Revision Level")); + val = CFDictionaryGetValue(dict, CFSTR("Product Revision Level")); if (val) cfstring_to_cstr(val, info->firmware, sizeof(info->firmware)); CFRelease(dc); @@ -236,8 +260,41 @@ int shim_open_exclusive(const char *bsd_name) { }; pid_t pid; if (posix_spawn(&pid, "/usr/sbin/diskutil", &fa, NULL, argv, environ) == 0) { + // BOUNDED wait. A plain blocking waitpid() here hung the public + // scsi::open() forever whenever the unmount wedged — diskutil + // blocks indefinitely on a volume whose filesystem is stuck (a + // hung network mount, a fs process not answering the unmount + // notification), and there is no signal, timeout or cancellation + // reaching this frame. Poll with WNOHANG to a deadline, then + // SIGKILL and reap so no zombie is left behind. + // + // Continuing after a killed unmount is deliberate: + // ObtainExclusiveAccess below is the real gate, and it reports the + // still-mounted disc through the shim's -5 sentinel (mapped to + // Error::DeviceLocked) — a typed error the caller can act on, + // instead of a process that never returns. + const int poll_us = 50000; // 50 ms + const int max_polls = 400; // 400 x 50 ms = 20 s int status; - waitpid(pid, &status, 0); + int reaped = 0; + for (int i = 0; i <= max_polls; i++) { + pid_t r = waitpid(pid, &status, WNOHANG); + if (r == pid) { reaped = 1; break; } + // r < 0 means the child is already gone (ECHILD) — nothing to + // wait for, and looping would spin to the deadline. + if (r < 0) { reaped = 1; break; } + if (i == max_polls) break; + usleep(poll_us); + } + if (!reaped) { + kill(pid, SIGKILL); + // SIGKILL is uncatchable, so this reap converges; still poll + // rather than block, so the shim has no unbounded wait at all. + for (int i = 0; i < 100; i++) { + if (waitpid(pid, &status, WNOHANG) != 0) break; + usleep(10000); // 10 ms x 100 = 1 s + } + } } posix_spawn_file_actions_destroy(&fa); } @@ -254,8 +311,14 @@ int shim_open_exclusive(const char *bsd_name) { svc = find_bdsvc_from_iomedia(mp, bsd_name); } if (!svc) { + // IOServiceMatching returns NULL on allocation failure. Both other call + // sites in this file check it; this one did not, and + // IOServiceGetMatchingService with a NULL matching dictionary is + // undefined (it consumes the reference it is given). CFMutableDictionaryRef matching = IOServiceMatching("IOBDServices"); - svc = IOServiceGetMatchingService(mp, matching); + if (matching) { + svc = IOServiceGetMatchingService(mp, matching); + } } if (!svc) return -1; @@ -369,6 +432,50 @@ int shim_execute(const unsigned char *cdb, unsigned char cdb_len, return (int)kr; } +// ── Registry-based media-presence probe ─────────────────────────────────── +// +// "Is a disc inserted?" answered from the IOKit registry alone: no exclusive +// access, no unmount, no SCSI command, no state change of any kind. +// +// Apple's IOStorageFamily publishes an IOMedia object for a removable device +// only while media is present, and tears it down on eject — that is the +// documented media lifecycle (Apple, "Mass Storage Device Driver Programming +// Guide": Media Objects / media arrival and removal). So the presence of an +// IOMedia whose "BSD Name" is the requested device IS the presence of a disc. +// +// Returns 1 (media present), 0 (no media), or -1 (IOKit unavailable). +int shim_media_present(const char *bsd_name) { + mach_port_t mp; + if (IOMainPort(0, &mp) != kIOReturnSuccess) return -1; + + CFMutableDictionaryRef matching = IOServiceMatching("IOMedia"); + if (!matching) return -1; + + io_iterator_t iter; + // Consumes `matching` whether it succeeds or fails. + if (IOServiceGetMatchingServices(mp, matching, &iter) != KERN_SUCCESS) return -1; + + int found = 0; + io_service_t media; + while ((media = IOIteratorNext(iter)) != 0) { + char name[64]; + if (registry_entry_bsd_name(media, name, sizeof(name)) + && strcmp(name, bsd_name) == 0) + { + found = 1; + } + IOObjectRelease(media); + if (found) break; + } + + // Drain the rest so no entry is leaked when we broke early. + while ((media = IOIteratorNext(iter)) != 0) { + IOObjectRelease(media); + } + IOObjectRelease(iter); + return found; +} + // ── Registry-based drive enumeration ────────────────────────────────────── // // Walks IOBDServices entries in the IOKit registry. No exclusive access, diff --git a/src/scsi/mod.rs b/src/scsi/mod.rs index d6a6cea..f2d1700 100644 --- a/src/scsi/mod.rs +++ b/src/scsi/mod.rs @@ -41,6 +41,11 @@ pub const AACS_KEY_CLASS: u8 = 0x02; /// TUR is the cheapest SCSI op (no data transfer); 5 s is generous /// for any healthy bus and short enough that a hung device can't stall /// a poll-loop tick. +/// +/// Used by the Linux and Windows backends. macOS answers the same question +/// from the IOKit registry (no SCSI command is issued, so no timeout applies) +/// — see `macos::drive_has_disc`. +#[cfg_attr(target_os = "macos", allow(dead_code))] pub(crate) const TUR_TIMEOUT_MS: u32 = 5_000; /// Timeout for content READ commands (READ_10 / READ_12) on the fast @@ -117,8 +122,14 @@ pub const SCSI_STATUS_TRANSPORT_FAILURE: u8 = 0xFF; /// Lives here, shared by all three platform backends, so the guard cannot /// drift per platform (it previously truncated on Linux and Windows while /// erroring on macOS — the "works on my platform, not theirs" class). +/// An EMPTY CDB is rejected here too. `ScsiTransport` is a public trait, so an +/// out-of-crate caller can pass one; every backend then either indexes `cdb[0]` +/// (a panic out of a public API) or hands the driver a zero-length command +/// descriptor, which under SPC-4 is not a command at all. That guard used to +/// exist ONLY in the Linux backend — macOS and Windows had nothing — which is +/// the same per-platform drift this helper exists to prevent. pub(crate) fn checked_cdb_len(cdb: &[u8], max: usize) -> Result { - if cdb.len() > max { + if cdb.is_empty() || cdb.len() > max { return Err(Error::InvalidCdbLength { len: cdb.len(), max, @@ -165,11 +176,10 @@ mod cdb_len_tests { } /// Every real CDB length (SPC-4 groups 0-5: 6, 10, 12, 16 bytes) is - /// accepted and reported verbatim, and an empty CDB reports 0 — the - /// backends' own empty-CDB guards handle that case. + /// accepted and reported verbatim. #[test] fn in_range_cdb_lengths_pass_through_verbatim() { - for len in [0usize, 6, 10, 12, 16] { + for len in [6usize, 10, 12, 16] { let cdb = vec![0u8; len]; assert_eq!( checked_cdb_len(&cdb, MAX).ok(), @@ -178,6 +188,29 @@ mod cdb_len_tests { ); } } + + /// An EMPTY CDB must be rejected by the SHARED helper, not left to a + /// per-backend guard. It previously reported `Ok(0)` and only the Linux + /// backend caught it before `cdb[0]`; macOS and Windows passed a + /// zero-length command descriptor straight to the driver. + /// + /// This is the only place the property can be tested on every platform's + /// CI — none of `linux.rs` / `macos.rs` / `windows.rs` compiles on more + /// than one host. + #[test] + fn empty_cdb_is_rejected_by_the_shared_helper() { + match checked_cdb_len(&[], MAX) { + Err(Error::InvalidCdbLength { len, max }) => { + assert_eq!(len, 0); + assert_eq!(max, MAX); + } + Err(other) => panic!("expected InvalidCdbLength, got {other:?}"), + Ok(n) => panic!( + "empty CDB accepted with length {n} — every backend would then \ + index cdb[0] or issue a zero-length command descriptor" + ), + } + } } // ── SPC-4 sense keys (§4.5.6 Table 28) ───────────────────────────────────── diff --git a/tests/disc_tests.rs b/tests/disc_tests.rs index 3367bf6..295dfc5 100644 --- a/tests/disc_tests.rs +++ b/tests/disc_tests.rs @@ -379,85 +379,119 @@ fn resolve_encryption_no_aacs_dir() { } // ── Batch count arithmetic tests ────────────────────────────────────────── -// Regression tests for the u16 truncation bug: when (remaining as u16) was -// used instead of remaining.min(batch as u32) as u16, any remaining count -// that was a multiple of 65536 would truncate to 0, causing an infinite loop. +// Regression tests for the u16 truncation bug in the prefetch producer's +// per-batch sector count (`src/sector/prefetched.rs`): when +// `(remaining as u16).min(batch_sectors)` was used instead of +// `remaining.min(batch_sectors as u32) as u16`, any remaining count that is a +// multiple of 65536 truncated to 0. +// +// These tests used to assert against `safe_batch_count`/`buggy_batch_count` +// copies defined in THIS file, so the production expression could be reverted +// with every one of them staying green. They now drive the real producer +// through the public `PrefetchedSectorSource` API and assert on the sector +// count of the batch it actually emits. -/// Simulates the fixed batch count calculation from pipe.rs / drive.rs -fn safe_batch_count(remaining: u32, batch_sectors: u16) -> u16 { - remaining.min(batch_sectors as u32) as u16 +/// Endless zero-filled source: every read succeeds with the full requested +/// span, so the producer's batch size is the only thing the returned byte +/// count can reflect. +struct ZeroSectorSource; + +impl SectorSource for ZeroSectorSource { + fn read_sectors( + &mut self, + _lba: u32, + count: u16, + buf: &mut [u8], + _recovery: bool, + ) -> Result { + let bytes = count as usize * SECTOR_SIZE; + buf[..bytes].fill(0); + Ok(bytes) + } } -/// Simulates the BUGGY calculation that caused the infinite loop -fn buggy_batch_count(remaining: u32, batch_sectors: u16) -> u16 { - (remaining as u16).min(batch_sectors) +/// Sectors in the FIRST batch the real prefetch producer emits for an extent +/// of `sector_count` sectors at the configured `batch_sectors`. This is the +/// production expression under test, reached only through public API. +fn first_batch_sectors(sector_count: u32, batch_sectors: u16) -> usize { + let mut src = libfreemkv::PrefetchedSectorSource::new( + ZeroSectorSource, + vec![libfreemkv::Extent { + start_lba: 0, + sector_count, + }], + batch_sectors, + None, + ) + .expect("prefetch producer spawns"); + let mut buf = vec![0u8; batch_sectors as usize * SECTOR_SIZE]; + let n = src + .read_sectors(0, batch_sectors, &mut buf, false) + .expect("first batch"); + assert_eq!( + n % SECTOR_SIZE, + 0, + "batch must be a whole number of sectors" + ); + n / SECTOR_SIZE } #[test] fn batch_count_normal() { - // Normal case: remaining > batch_sectors - assert_eq!(safe_batch_count(1000, 60), 60); - assert_eq!(safe_batch_count(47533152, 60), 60); + // Normal case: remaining > batch_sectors → a full batch. + assert_eq!(first_batch_sectors(1000, 60), 60); + assert_eq!(first_batch_sectors(47533152, 60), 60); } #[test] fn batch_count_last_batch() { - // Last batch: remaining < batch_sectors - assert_eq!(safe_batch_count(30, 60), 30); - assert_eq!(safe_batch_count(1, 60), 1); + // Only batch: remaining < batch_sectors → the remainder, not the batch. + assert_eq!(first_batch_sectors(30, 60), 30); + assert_eq!(first_batch_sectors(3, 60), 3); } #[test] fn batch_count_exact_boundary() { - // Exact boundary: remaining == batch_sectors - assert_eq!(safe_batch_count(60, 60), 60); + // Exact boundary: remaining == batch_sectors. + assert_eq!(first_batch_sectors(60, 60), 60); } #[test] fn batch_count_u16_overflow_regression() { - // THE BUG: remaining is a multiple of 65536 → truncates to 0 - // 47513600 = 725 * 65536, lower 16 bits = 0 - let remaining: u32 = 47533152 - 19552; // = 47513600 + // THE BUG: remaining is a multiple of 65536 → `remaining as u16` is 0, so + // the batch collapses (the unit-alignment clamp below it then floors the + // batch at one 3-sector AACS unit — a 20x throughput cliff on exactly the + // disc sizes that hit it, and an outright stall before that clamp existed). + let remaining: u32 = 47533152 - 19552; // = 47513600 = 725 * 65536 assert_eq!(remaining, 47513600); assert_eq!( remaining % 65536, 0, - "remaining should be multiple of 65536" + "remaining must be a multiple of 65536" + ); + assert_eq!( + first_batch_sectors(remaining, 60), + 60, + "a remaining count that is a multiple of 65536 must still yield a full batch" ); - - // Buggy version produces 0 → infinite loop - assert_eq!(buggy_batch_count(remaining, 60), 0); - - // Fixed version produces 60 - assert_eq!(safe_batch_count(remaining, 60), 60); } #[test] fn batch_count_other_u16_overflow_values() { - // Other multiples of 65536 - assert_eq!(safe_batch_count(65536, 60), 60); - assert_eq!(safe_batch_count(131072, 60), 60); - assert_eq!(safe_batch_count(65536 * 100, 60), 60); - - // Verify buggy version fails on all of these - assert_eq!(buggy_batch_count(65536, 60), 0); - assert_eq!(buggy_batch_count(131072, 60), 0); - assert_eq!(buggy_batch_count(65536 * 100, 60), 0); + // Other multiples of 65536 — every one truncates to 0 under the old cast. + assert_eq!(first_batch_sectors(65536, 60), 60); + assert_eq!(first_batch_sectors(131072, 60), 60); + assert_eq!(first_batch_sectors(65536 * 100, 60), 60); } #[test] fn batch_count_near_u16_boundary() { - // Values just below and above 65536 - assert_eq!(safe_batch_count(65535, 60), 60); - assert_eq!(safe_batch_count(65536, 60), 60); - assert_eq!(safe_batch_count(65537, 60), 60); - - // Buggy: 65535 as u16 = 65535, min(60) = 60 (OK by accident) - assert_eq!(buggy_batch_count(65535, 60), 60); - // Buggy: 65536 as u16 = 0, min(60) = 0 (BUG) - assert_eq!(buggy_batch_count(65536, 60), 0); - // Buggy: 65537 as u16 = 1, min(60) = 1 (wrong but doesn't loop) - assert_eq!(buggy_batch_count(65537, 60), 1); + // Just below, at, and just above the 16-bit wrap point. 65535 survives the + // bad cast by accident; 65536 truncates to 0 and 65537 to 1 — all three + // must produce the same full batch. + assert_eq!(first_batch_sectors(65535, 60), 60); + assert_eq!(first_batch_sectors(65536, 60), 60); + assert_eq!(first_batch_sectors(65537, 60), 60); } #[test] @@ -465,39 +499,59 @@ fn batch_count_real_disc_sizes() { let batch: u16 = 60; // DVD-5: ~2,295,104 sectors - assert_eq!(safe_batch_count(2295104, batch), 60); + assert_eq!(first_batch_sectors(2295104, batch), 60); // BD-25: ~12,219,392 sectors - assert_eq!(safe_batch_count(12219392, batch), 60); + assert_eq!(first_batch_sectors(12219392, batch), 60); // BD-50: ~24,438,784 sectors - assert_eq!(safe_batch_count(24438784, batch), 60); + assert_eq!(first_batch_sectors(24438784, batch), 60); // UHD BD-66: ~33,554,432 sectors - assert_eq!(safe_batch_count(33554432, batch), 60); + assert_eq!(first_batch_sectors(33554432, batch), 60); // UHD BD-100: ~47,533,152 sectors - assert_eq!(safe_batch_count(47533152, batch), 60); + assert_eq!(first_batch_sectors(47533152, batch), 60); - // Last few sectors of each - assert_eq!(safe_batch_count(52, batch), 52); - assert_eq!(safe_batch_count(3, batch), 3); + // Short tails (whole AACS units, as Blu-ray m2ts extents are by spec). + assert_eq!(first_batch_sectors(51, batch), 51); + assert_eq!(first_batch_sectors(3, batch), 3); } #[test] fn batch_count_zero_remaining() { - // Zero remaining should produce 0 (loop exits before this) - assert_eq!(safe_batch_count(0, 60), 0); + // A zero-sector extent yields no batch at all: the producer skips it and + // the channel closes, which the consumer reads as end-of-stream (Ok(0)). + let mut src = libfreemkv::PrefetchedSectorSource::new( + ZeroSectorSource, + vec![libfreemkv::Extent { + start_lba: 0, + sector_count: 0, + }], + 60, + None, + ) + .expect("prefetch producer spawns"); + let mut buf = vec![0u8; 60 * SECTOR_SIZE]; + assert_eq!(src.read_sectors(0, 60, &mut buf, false).unwrap(), 0); } #[test] fn batch_count_max_batch_sizes() { - // Test with different batch sizes used by detect_max_batch_sectors + // Every batch size detect_max_batch_sectors can pick. All are multiples of + // the 3-sector AACS unit, so none is reshaped by the alignment trim — the + // batch the producer emits is the truncation-prone expression's output. for &batch in &[3u16, 6, 9, 30, 60, 120, 240, 510] { - // Large remaining should always return batch - assert_eq!(safe_batch_count(47533152, batch), batch); - // Small remaining should return remaining - assert_eq!(safe_batch_count(1, batch), 1); + assert_eq!( + first_batch_sectors(65536 * 100, batch), + batch as usize, + "batch {batch}: multiple-of-65536 remaining must still fill the batch" + ); + assert_eq!( + first_batch_sectors(3, batch), + 3, + "batch {batch}: short tail" + ); } }