Turn a release-only slice panic into an error, and stop calling 32 kHz 48 kHz
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.
This commit is contained in:
@@ -187,12 +187,20 @@ impl SectorSource for FileSectorSource {
|
||||
) -> Result<usize> {
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user