From d444afbdfc2b7f0bef4de9dceb4d41f7d0f22df3 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 29 Jul 2026 22:56:33 -0700 Subject: [PATCH] Turn a release-only slice panic into an error, and stop calling 32 kHz 48 kHz MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four round-6 findings. FileSectorSource::read_sectors guarded its output buffer with a debug_assert, which is compiled out in release — so an undersized buffer panicked with 'range end index out of range' instead of returning an error, out of a public SectorSource impl where the length is caller input. Drive::read_fua already carries this exact guard, with a comment recording the same panic being fixed there, and PrefetchedSectorSource has a regression test for the same case; this impl had been given neither. The new test is red in release for precisely the predicted reason: 'range end index 8192 out of range for slice of length 2049'. parse_track's sample-rate ladder ended in an unconditional S48, so any SamplingFrequency below 44100 was recorded as 48 kHz. A 32000 Hz AC-3 or DTS track is legal and common in broadcast-sourced content, and the wrong rate then propagated into the reconstructed AudioStream. Anything below the lowest mapped rate is now Unknown, which is what the crate's canonical SampleRate::from_hz already returned — the ladder disagreed with it. The ladder itself stays, because the MKV element is a float and wants tolerance rather than exact equality. shim_open_exclusive used the mach port from IOMainPort without checking the return; on failure the port is left untouched and every IOKit call below ran against an uninitialised value. shim_list_drives in the same file does check it. build.rs treated cc and ar as successful if the process merely SPAWNED, so a genuine compile error in the macOS C shim produced no object file and surfaced later as an unexplained link failure against a missing symbol. The shim is macOS-only and is neither linted nor compiled on the other two platforms, so a mistake in it has exactly one chance to be noticed. The last two have no test: one needs IOMainPort to fail, the other needs a deliberately broken C shim, and neither is reachable from the test harness. Both mirror a correct sibling in the same file, which is the evidence available. --- build.rs | 15 ++++++-- src/io/file_sector_source/mod.rs | 54 +++++++++++++++++++++++--- src/mux/mkvstream.rs | 65 +++++++++++++++++++++++++++++++- src/scsi/macos_shim.c | 5 ++- 4 files changed, 127 insertions(+), 12 deletions(-) diff --git a/build.rs b/build.rs index 0a71ae7..410d04b 100644 --- a/build.rs +++ b/build.rs @@ -22,7 +22,7 @@ fn main() { &target_arch // x86_64 → x86_64 }; - std::process::Command::new("cc") + let cc_status = std::process::Command::new("cc") .args([ "-arch", clang_arch, @@ -38,12 +38,19 @@ fn main() { "-O2", ]) .status() - .expect("failed to compile macos_shim.c"); + .expect("failed to spawn cc for macos_shim.c"); + // `.status()` succeeding only means the process RAN. A real compile error + // exits non-zero, and ignoring that left no object file, which surfaced + // much later as an unexplained link failure against a missing symbol. The + // shim is macOS-only and is neither linted nor compiled on the other two + // platforms, so a mistake in it has exactly one chance to be noticed. + assert!(cc_status.success(), "cc failed to compile macos_shim.c"); - std::process::Command::new("ar") + let ar_status = std::process::Command::new("ar") .args(["rcs", &lib, &obj]) .status() - .expect("failed to create static lib"); + .expect("failed to spawn ar"); + assert!(ar_status.success(), "ar failed to create the static lib"); println!("cargo:rustc-link-search=native={out_dir}"); println!("cargo:rustc-link-lib=static=macos_scsi"); diff --git a/src/io/file_sector_source/mod.rs b/src/io/file_sector_source/mod.rs index d1465b6..bb8c865 100644 --- a/src/io/file_sector_source/mod.rs +++ b/src/io/file_sector_source/mod.rs @@ -187,12 +187,20 @@ impl SectorSource for FileSectorSource { ) -> Result { let count = count as u32; let bytes = count as usize * SECTOR_BYTES; - debug_assert!( - out.len() >= bytes, - "FileSectorSource::read_sectors: out len {} < requested {}", - out.len(), - bytes - ); + // A real check, not a debug_assert: this is a public `SectorSource` impl, + // so an undersized `out` is caller input, and `out[..bytes]` below would + // panic with 'range end index out of range' in release where the assert is + // compiled away. `Drive::read_fua` already carries exactly this guard, with + // a comment recording the same panic being fixed there — this impl was + // simply never given it, and `PrefetchedSectorSource` has a regression test + // for the case that this one lacked. + if out.len() < bytes { + return Err(Error::DiscRead { + sector: lba as u64, + status: None, + sense: None, + }); + } if count == 0 { return Ok(0); } @@ -235,6 +243,40 @@ mod tests { use std::io::Write; use tempfile::tempdir; + /// An undersized output buffer must return an error, never panic. This is a + /// public `SectorSource` impl, so buffer length is caller input, and the guard + /// used to be a `debug_assert!` — compiled out in release, where the + /// `out[..bytes]` slice then panicked with 'range end index out of range'. + /// + /// `Drive::read_fua` already carries this exact guard with a comment recording + /// the same panic being fixed there, and `PrefetchedSectorSource` has + /// `direct_read_too_small_buffer_errors` for the same case; this impl had + /// neither. + #[test] + fn read_sectors_with_an_undersized_buffer_errors_rather_than_panicking() { + let dir = tempdir().unwrap(); + let iso = dir.path().join("t.iso"); + make_iso(&iso, 8); + let mut src = FileSectorSource::open(&iso).expect("iso opens"); + + // Ask for four sectors but supply room for barely more than one. + let mut out = vec![0u8; SECTOR_BYTES + 1]; + let err = src + .read_sectors(0, 4, &mut out, false) + .expect_err("an undersized buffer must be an error, not a panic"); + assert!( + matches!(err, Error::DiscRead { .. }), + "expected DiscRead, got {err:?}" + ); + + // Exactly-sized still works, so the guard is not off by one. + let mut out = vec![0u8; 4 * SECTOR_BYTES]; + assert_eq!( + src.read_sectors(0, 4, &mut out, false).unwrap(), + 4 * SECTOR_BYTES + ); + } + /// Build a deterministic ISO of `sectors` sectors where sector `n` /// is filled with the byte pattern `((n & 0xff) as u8)`. Lets us /// verify any sector by content alone. diff --git a/src/mux/mkvstream.rs b/src/mux/mkvstream.rs index 18a8643..d08db83 100644 --- a/src/mux/mkvstream.rs +++ b/src/mux/mkvstream.rs @@ -1226,8 +1226,18 @@ fn parse_track(r: &mut impl Read, size: u64) -> io::Result { SampleRate::S88_2 } else if (44100.0..48000.0).contains(&sr) { SampleRate::S44_1 - } else { + } else if sr >= 48000.0 { SampleRate::S48 + } else { + // Anything below the lowest rate this enum maps is UNKNOWN, not 48 kHz. + // The ladder's final `else` used to be S48, so a legal 32000 Hz AC-3 or + // DTS track — common in broadcast-sourced content — was recorded as + // 48 kHz, and the wrong rate then propagated into the reconstructed + // AudioStream. `SampleRate::from_hz` in disc/mod.rs is the crate's + // canonical mapping and returns Unknown here; this ladder exists only + // because the MKV element is a float and needs tolerance rather than + // exact equality. + SampleRate::Unknown }; // Map MKV track numbers to BD-TS PIDs. A 13-bit TS PID tops out at @@ -3301,6 +3311,59 @@ mod tests { /// `TrackNumber - 1`, so the audio blocks of TrackNumber 3 resolved to index /// 2 in a 2-stream title and were DISCARDED — a remux with no audio, reported /// as success. + /// A legal SamplingFrequency below the lowest rate this enum maps must come + /// back as Unknown, not silently as 48 kHz. + /// + /// The ladder's final `else` was `SampleRate::S48`, so a 32000 Hz AC-3 or DTS + /// track — legal, and common in broadcast-sourced content — was recorded as + /// 48 kHz and the wrong rate propagated into the reconstructed AudioStream. + /// The crate's canonical mapping, `SampleRate::from_hz`, returns Unknown for + /// 32000; this ladder disagreed with it. + #[test] + fn a_sub_44100_sampling_frequency_is_unknown_not_48k() { + /// One TrackEntry body: an audio track with the given sampling frequency. + fn audio_track_body(freq: f64) -> Vec { + let mut audio = Vec::new(); + audio.push(super::ebml::SAMPLING_FREQUENCY as u8); + audio.push(0x88); // 8-byte float payload + audio.extend_from_slice(&freq.to_be_bytes()); + audio.push(super::ebml::CHANNELS as u8); + audio.extend_from_slice(&[0x81, 0x02]); + + let mut body = Vec::new(); + body.push(super::ebml::TRACK_NUMBER as u8); + body.extend_from_slice(&[0x81, 0x01]); + body.push(super::ebml::TRACK_TYPE as u8); + body.extend_from_slice(&[0x81, super::ebml::TRACK_TYPE_AUDIO as u8]); + body.push(super::ebml::CODEC_ID as u8); + let cid = b"A_AC3"; + body.push(0x80 | cid.len() as u8); + body.extend_from_slice(cid); + body.push(super::ebml::AUDIO as u8); + body.push(0x80 | audio.len() as u8); + body.extend_from_slice(&audio); + body + } + + for (freq, want) in [ + (32000.0f64, SampleRate::Unknown), + (16000.0, SampleRate::Unknown), + (44100.0, SampleRate::S44_1), + (48000.0, SampleRate::S48), + (96000.0, SampleRate::S96), + ] { + let body = audio_track_body(freq); + let mut cur = std::io::Cursor::new(body.clone()); + let parsed = super::parse_track(&mut cur, body.len() as u64) + .unwrap_or_else(|e| panic!("track with {freq} Hz must parse: {e}")); + let got = match parsed.0.as_ref().expect("an audio track yields a stream") { + Stream::Audio(a) => a.sample_rate, + other => panic!("expected an audio stream, got {other:?}"), + }; + assert_eq!(got, want, "{freq} Hz must map to {want:?}, got {got:?}"); + } + } + #[test] fn sparse_track_numbers_route_to_the_right_stream() { let video = [0x81u8, 0x00, 0x00, 0x80, 0x11]; // TrackNumber 1 diff --git a/src/scsi/macos_shim.c b/src/scsi/macos_shim.c index 751e803..b4330d4 100644 --- a/src/scsi/macos_shim.c +++ b/src/scsi/macos_shim.c @@ -244,7 +244,10 @@ int shim_open_exclusive(const char *bsd_name) { usleep(500000); mach_port_t mp; - IOMainPort(0, &mp); + // Check the return before using the port. On failure IOMainPort leaves `mp` + // untouched, so every IOKit call below would run against an uninitialised + // mach port. shim_list_drives does check it; this path did not. + if (IOMainPort(0, &mp) != kIOReturnSuccess) return -1; io_service_t svc = find_bdsvc_by_bsd_name(mp, bsd_name); if (!svc) {