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"); + } }