From fb321f51ebb8066f6097ca393b16177b9f9db9c9 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Sat, 1 Aug 2026 11:00:49 -0700 Subject: [PATCH] Reject a short READ CAPACITY reply, and count only entry marks as chapters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cases of the same shape: one policy implemented twice, with only one copy hardened. Disc::read_capacity decoded buf[0..4] from READ CAPACITY (10) without checking that the transport actually delivered four bytes, even though its comment claims to mirror decode_read_capacity — which has exactly that check, and documents why. A drive answering GOOD with an empty data phase leaves the buffer zeroed, so last_lba decodes to 0 and the probe reports a one-sector disc instead of an error. It now calls the shared decoder rather than re-deriving it. collect_chapter_summary filtered chapters on mark_type <= 1, counting the reserved type 0. PlaylistMark's own doc says filters must test == 1, and disc/bluray.rs did; the labels path did not, inflating the public chapter_count and letting a playlist whose only marks are reserved pass the chapter_count == 0 skip. Both sites now share PlaylistMark::is_chapter_mark so the copies cannot drift again. Both fixes were confirmed red before green. --- src/disc/bluray.rs | 2 +- src/disc/mod.rs | 47 +++++++++++++++++++++++++++++++++++++++------- src/drive/mod.rs | 4 ++-- src/labels/mod.rs | 8 ++++++-- src/mpls.rs | 32 +++++++++++++++++++++++++++++++ 5 files changed, 81 insertions(+), 12 deletions(-) diff --git a/src/disc/bluray.rs b/src/disc/bluray.rs index cbaa8f5..cf6c90a 100644 --- a/src/disc/bluray.rs +++ b/src/disc/bluray.rs @@ -298,7 +298,7 @@ impl Disc { let chapters: Vec = parsed .marks .iter() - .filter(|m| m.mark_type == 1) + .filter(|m| m.is_chapter_mark()) .filter_map(|m| { let pi_idx = m.play_item_ref as usize; let pi = parsed.play_items.get(pi_idx)?; diff --git a/src/disc/mod.rs b/src/disc/mod.rs index aa2fe2e..f085b55 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -2327,18 +2327,17 @@ impl Disc { 0x00, ]; let mut buf = [0u8; 8]; - session.scsi_execute( + let result = session.scsi_execute( &cdb, crate::scsi::DataDirection::FromDevice, &mut buf, 5_000, )?; - let lba = u32::from_be_bytes([buf[0], buf[1], buf[2], buf[3]]); - // `last_lba + 1` = sector count. Guard the 0xFFFF_FFFF sentinel - // (capacity exceeds 32 bits) so it surfaces as an error instead of - // wrapping to 0 in release — mirrors the public `decode_read_capacity`. - lba.checked_add(1) - .ok_or(crate::error::Error::DiscCapacityOverflow) + // Share the decoder with `Drive::capacity` rather than re-deriving it. + // A hand-rolled copy here previously dropped the short-transfer check: + // a drive answering GOOD with an empty data phase leaves `buf` zeroed, + // which decodes to a 1-sector disc instead of an error. + crate::drive::decode_read_capacity(&buf, result.bytes_transferred) } } @@ -6396,4 +6395,38 @@ mod tests { std::env::temp_dir().join("VOLUME_ID.mapfile") ); } + + /// A drive that answers READ CAPACITY (10) with GOOD status but an empty + /// data phase leaves the caller's buffer zero-initialised. Decoding that + /// blind yields `last_lba = 0` -> a "1 sector" disc, which reads as a + /// successful capacity probe of an absurdly small disc rather than as the + /// malformed response it is. `Disc::read_capacity` must reject it. + #[test] + fn read_capacity_rejects_a_short_transfer_instead_of_reporting_one_sector() { + use crate::scsi::{DataDirection, ScsiResult, ScsiTransport}; + + /// GOOD status, no sense, and *nothing written* to `buf`. + struct EmptyDataPhase; + impl ScsiTransport for EmptyDataPhase { + fn execute( + &mut self, + _cdb: &[u8], + _dir: DataDirection, + _buf: &mut [u8], + _timeout_ms: u32, + ) -> crate::error::Result { + Ok(ScsiResult { + status: 0, + sense: [0u8; 32], + bytes_transferred: 0, + }) + } + } + + let mut drive = crate::drive::Drive::from_transport_for_test(Box::new(EmptyDataPhase)); + assert!(matches!( + Disc::read_capacity(&mut drive), + Err(crate::error::Error::DiscCapacityMalformed) + )); + } } diff --git a/src/drive/mod.rs b/src/drive/mod.rs index c826ff7..1baf1ad 100644 --- a/src/drive/mod.rs +++ b/src/drive/mod.rs @@ -158,7 +158,7 @@ impl Drive { /// fallback) so command-builder/response-parser logic can be exercised /// against a scripted mock transport. #[cfg(test)] - fn from_transport_for_test(scsi: Box) -> Self { + pub(crate) fn from_transport_for_test(scsi: Box) -> Self { Drive { scsi, unlocker_name: None, @@ -1291,7 +1291,7 @@ fn build_error_recovery_select_payload(sense: &[u8]) -> Option> { /// 32-bit" sentinel, whose `last_lba + 1` overflows `u32`, is reported as the /// distinct [`Error::DiscCapacityOverflow`] so callers can tell an unusable /// response apart from an over-large disc. -fn decode_read_capacity(buf: &[u8; 8], bytes_transferred: usize) -> Result { +pub(crate) fn decode_read_capacity(buf: &[u8; 8], bytes_transferred: usize) -> Result { if bytes_transferred < 4 { return Err(Error::DiscCapacityMalformed); } diff --git a/src/labels/mod.rs b/src/labels/mod.rs index 9ce3eff..408b8c4 100644 --- a/src/labels/mod.rs +++ b/src/labels/mod.rs @@ -865,7 +865,7 @@ pub fn analyze(reader: &mut dyn SectorSource, udf: &UdfFs) -> LabelAnalysis { } /// Scan `/BDMV/PLAYLIST/*.mpls`, parse each, return a row per playlist -/// with chapter count (mark_type ≤ 1) and total duration. Sorted by +/// with chapter count (entry marks only) and total duration. Sorted by /// playlist filename. Skipped entries (read error, parse error, no /// marks) silently dropped — this is a diagnostic field, not a /// correctness-critical one. @@ -890,7 +890,11 @@ fn collect_chapter_summary(reader: &mut dyn SectorSource, udf: &UdfFs) -> Vec bool { + self.mark_type == 1 + } +} + /// A play item — one clip reference with in/out times. #[derive(Debug)] pub(crate) struct PlayItem { @@ -1698,4 +1710,24 @@ mod tests { assert_eq!(entry.coding_type, 0x90); assert_eq!(entry.language, ""); } + + /// Type 0 is reserved and type 2 is a link point; neither is a chapter. + /// `labels::collect_chapter_summary` used to filter on `mark_type <= 1`, + /// which counted reserved marks and inflated the public `chapter_count` + /// (and let a playlist whose only marks are reserved pass the + /// `chapter_count == 0` skip). Both call sites now share this predicate. + #[test] + fn only_entry_marks_count_as_chapters() { + let mk = |mark_type| PlaylistMark { + mark_type, + play_item_ref: 0, + timestamp: 0, + }; + assert!( + !mk(0).is_chapter_mark(), + "type 0 is reserved, not a chapter" + ); + assert!(mk(1).is_chapter_mark()); + assert!(!mk(2).is_chapter_mark(), "type 2 is a link point"); + } }