From 54d038e47872716e366055ac50b2b604053e12f9 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Thu, 30 Jul 2026 20:00:15 -0700 Subject: [PATCH] fix(udf): file_start_lba must skip a leading unrecorded extent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression from the round-8 change that started RETAINING ECMA-167 4/14.14.1.1 type-1 (allocated, not recorded) descriptors. Retaining them is correct — dropping one slides every later extent's data down by the hole's length, corrupting the file silently. But read_icb_extent still took extents.first(), so the value it returns can now be a hole. A type-1 extent's lba is where SPACE is allocated, not where bytes live. IcbExtent's own doc says exactly that. file_start_lba hands the value out as "the absolute starting LBA of a file's first data extent", and ifo.rs uses it as the base for every VTS VOB extent: file_start_lba(IFO) + vtstt_vobs + cell.first_sector So a DVD whose IFO's first descriptor is type-1 reads its entire video title set from the wrong place on disc. No error anywhere — the reads succeed, they just land on unrelated sectors. Verified: the test reports 2900 instead of 2040 with the old code. Same shape as file_extents/extents_abs_at dropping the recorded flag, and the same root cause: one change taught read_icb_extents about a new extent type and did not visit the accessors that consume its output. Three of them; two are still open (task filed). Found by the round-9 opus escalation over the API contract, dispatched because the sonnet pass over the same scope returned zero findings. Seven of its eight items were absences rather than wrong lines — the class a wrong-line scan structurally cannot see. --- src/udf.rs | 85 +++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 8 deletions(-) diff --git a/src/udf.rs b/src/udf.rs index faa13a1..c3951bc 100644 --- a/src/udf.rs +++ b/src/udf.rs @@ -442,16 +442,34 @@ impl UdfFs { /// Read an Extended File Entry (tag 266) or File Entry (tag 261) /// and return its first allocation extent: (data_lba, data_length). /// The data_lba is partition-relative. + /// The file's first RECORDED extent — where its readable data actually + /// begins. + /// + /// Not `extents.first()`. `read_icb_extents` retains ECMA-167 4/14.14.1.1 + /// type-1 (allocated, not recorded) descriptors, because dropping one would + /// slide every later extent's data down by the hole's length. But a type-1 + /// extent's `lba` is where SPACE is allocated, not where bytes live, and + /// this accessor's one caller — `file_start_lba` — hands its result out as + /// "the absolute starting LBA of a file's first data extent". + /// + /// `ifo.rs` then uses that as the base for every VTS VOB extent + /// (`file_start_lba(IFO) + vtstt_vobs + cell.first_sector`), so a file whose + /// FIRST descriptor is type-1 would put the entire video title set at the + /// wrong place on disc, with no error anywhere. fn read_icb_extent(&self, reader: &mut dyn SectorSource, meta_lba: u32) -> Result { let extents = self.read_icb_extents(reader, meta_lba)?; - extents.first().copied().ok_or(Error::DiscRead { - // Diagnostic sector only; meta_to_abs can overflow on a crafted - // meta_lba, in which case 0 is a harmless placeholder for the - // error-context field. - sector: self.meta_to_abs(meta_lba).unwrap_or(0) as u64, - status: None, - sense: None, - }) + extents + .iter() + .find(|e| e.recorded) + .copied() + .ok_or(Error::DiscRead { + // Diagnostic sector only; meta_to_abs can overflow on a crafted + // meta_lba, in which case 0 is a harmless placeholder for the + // error-context field. + sector: self.meta_to_abs(meta_lba).unwrap_or(0) as u64, + status: None, + sense: None, + }) } /// If this ICB stores its file data INLINE (embedded — ICB Tag flags low @@ -1891,6 +1909,57 @@ mod tests { s } + /// `file_start_lba` must skip a leading UNRECORDED extent and report where + /// the file's DATA starts. + /// + /// ECMA-167 4/14.14.1.1 type 1 is allocated-but-not-recorded: the space + /// belongs to the file and occupies its byte range, but nothing was written + /// there and its `lba` is where SPACE lives, not bytes. `read_icb_extents` + /// retains such descriptors — dropping one would slide every later extent's + /// data down by the hole's length — so `.first()` can now be one. + /// + /// `ifo.rs` uses this value as the base for every VTS VOB extent + /// (`file_start_lba(IFO) + vtstt_vobs + cell.first_sector`), so getting it + /// wrong puts the entire video title set at the wrong place on disc, with + /// no error anywhere — a silent wrong answer on an ordinary DVD. + #[test] + fn file_start_lba_skips_a_leading_unrecorded_extent() { + use fixture::{DirSpec, MemDisc, PART_START, build_udf_skeleton, lay_dir}; + + const HOLE_LBA: u32 = 900; + const DATA_LBA: u32 = 40; + + let mut disc = MemDisc::new(); + build_udf_skeleton(&mut disc, 10); + lay_dir( + &mut disc, + &DirSpec { + name: String::new(), + icb_lba: 10, + dir_data_lba: 11, + files: vec![fixture::file("VIDEO_TS.IFO", 12, DATA_LBA, 2048, false)], + subdirs: Vec::new(), + }, + ); + + // Rewrite the file's ICB: a type-1 hole FIRST, then the real data. + let icb = build_entry_ads(266, 0, 16, &[(1, 2048, HOLE_LBA), (0, 2048, DATA_LBA)], &[]); + disc.put_bytes(PART_START + 12, &icb); + + let mut fs = super::read_filesystem(&mut disc).expect("volume mounts"); + let lba = fs + .file_start_lba(&mut disc, "/VIDEO_TS.IFO") + .expect("a file whose first descriptor is a hole still has data"); + + assert_eq!( + lba, + PART_START + DATA_LBA, + "must report the first RECORDED extent; reporting the hole's LBA \\ + ({}) would rebase every VTS VOB extent onto unrelated sectors", + PART_START + HOLE_LBA + ); + } + #[test] fn icb_extents_reads_both_entry_types_at_their_own_descriptor_offsets() { // ECMA-167 4/14.9 puts a File Entry's L_EA at byte 168 and its