From 03d088abfc7468905d40130057829d320d25eb04 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Thu, 6 Aug 2026 08:00:23 -0700 Subject: [PATCH] Catch names the reader cannot tell apart, and in-place content changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two dirimage findings, both silent-wrong-output. The per-directory uniqueness check compared raw host names, but the reader does not see raw host names: parse_udf_name trims leading and trailing whitespace and drops any code unit char::from_u32 rejects — which is every half of the surrogate pairs the encoder emits for non-BMP characters. So " 00000.m2ts" and "00000.m2ts", or "A.m2ts" and "A.m2ts", were two entries at plan time and ONE name at read time. find/read_file take the first match, so a title resolved to the wrong file extents and muxed the wrong bytes at exit 0 — the exact shadowing DirNameCollision exists to prevent. The key is now derived by round-tripping the name through the very encoder and parser that will be used, so it cannot drift from them. A name that survives that round trip as empty is refused outright: it would exist in the image and be addressable by nothing. The new test builds a real folder, calls plan, and was confirmed to FAIL with the fix reverted. Separately, the plan-vs-read revalidation compared file LENGTH only, while the plan depends on CONTENT: a DVD VOB placement comes from bytes 0xC0/0xC4 of its IFO, and IFOs occupy a whole number of sectors so an in-place rewrite keeps the length. A re-authoring tool touching the folder mid-rip would pass the size check while every title extent pointed at stale sectors. mtime is now compared alongside size, and only when both sides report one, so a filesystem without timestamps falls back to the old behaviour rather than failing every read. --- src/dirimage/layout.rs | 76 +++++++++++++++++++++++++++++++++++++++++- src/dirimage/mod.rs | 23 +++++++++++-- 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/src/dirimage/layout.rs b/src/dirimage/layout.rs index bb7aec8..a9221f0 100644 --- a/src/dirimage/layout.rs +++ b/src/dirimage/layout.rs @@ -102,6 +102,15 @@ pub(super) struct FileNode { pub(super) disc_path: String, pub(super) host: PathBuf, pub(super) size: u64, + /// Host mtime at PLAN time, when the platform reports one. + /// + /// Size alone is content-blind, and the plan depends on CONTENT: a DVD's + /// VOB placement is derived from bytes 0xC0/0xC4 of its IFO (`read_head`). + /// A re-authoring tool rewriting an IFO in place keeps the length — IFOs + /// are a whole number of sectors — so a size check passes while the + /// placement the image was built around is stale, and every cell of the + /// title then resolves to the wrong sectors at exit 0. + pub(super) mtime: Option, pub(super) icb_lba: u32, pub(super) unique_id: u64, pub(super) extents: Vec, @@ -211,7 +220,26 @@ fn walk(dir: &Path, disc_path: &str, depth: u32, entries: &mut usize) -> Result< if *entries > MAX_ENTRIES { return Err(Error::DirImageTooLarge); } - names.push(name.to_ascii_uppercase()); + // Key the uniqueness check on the name AS THE READER WILL SEE IT, by + // round-tripping through the very encoder and parser that will be used. + // + // The host name is not that name. `parse_udf_name` trims leading and + // trailing whitespace and drops any code unit `char::from_u32` rejects + // (a lone surrogate half, i.e. every non-BMP character the encoder + // emits as a surrogate pair). So " A.M2TS" and "A.M2TS", or "A.M2TS" + // and "A.M2TS", are distinct hosts that collapse to ONE name on read — + // and `find`/`read_file` take the first match, so a title silently + // resolves to the wrong file's extents and muxes the wrong bytes at + // exit 0. Comparing the raw host names cannot see that; deriving the + // key from the same two functions cannot drift from it. + let as_read = crate::udf::parse_udf_name(&crate::dirimage::encode::encode_cs0(&name)); + // Nothing left after the reader is done with it: the entry would exist + // in the image and be unaddressable by any name. Refuse rather than + // write something no consumer can reach. + if as_read.is_empty() { + return Err(Error::DirNameCollision { host: child_path }); + } + names.push(as_read.to_ascii_uppercase()); if ft.is_dir() { // A directory's File Entry records its link count in 16 bits: one // per child directory plus one for its own entry in the parent. @@ -242,6 +270,7 @@ fn walk(dir: &Path, disc_path: &str, depth: u32, entries: &mut usize) -> Result< disc_path: child_path, host: entry.path(), size: meta.len(), + mtime: meta.modified().ok(), icb_lba: 0, unique_id: 0, extents: Vec::new(), @@ -746,6 +775,7 @@ mod tests { disc_path: "/BIG.M2TS".into(), host: PathBuf::new(), size: MAX_AD_BYTES + 4096, + mtime: None, icb_lba: 0, unique_id: 0, extents: Vec::new(), @@ -778,6 +808,7 @@ mod tests { disc_path: "/EMPTY".into(), host: PathBuf::new(), size: 0, + mtime: None, icb_lba: 0, unique_id: 0, extents: Vec::new(), @@ -805,6 +836,49 @@ mod tests { /// /// An earlier version of this test asserted arithmetic about the constants /// and never called `plan`, so it would have passed with the guard deleted. + /// Two host names that the READER collapses into one must be refused by + /// the planner, not written into the image. + /// + /// Audit finding: the uniqueness check compared raw host names, while + /// `parse_udf_name` trims whitespace and drops code units `char::from_u32` + /// rejects. So " 00000.m2ts" and "00000.m2ts" were two distinct entries at + /// plan time and ONE name at read time; `find`/`read_file` take the first + /// match, so a title resolved to the wrong file's extents and muxed the + /// wrong bytes at exit 0. + /// + /// This calls `plan` on a real folder. It fails if the round-trip key is + /// reverted to comparing host names. + #[test] + fn two_names_the_reader_cannot_tell_apart_are_refused() { + let dir = std::env::temp_dir().join(format!( + "fmkv-shadow-{}-{:?}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let stream = dir.join("BDMV/STREAM"); + std::fs::create_dir_all(&stream).expect("mkdir"); + std::fs::write(stream.join("00000.m2ts"), b"a").expect("write"); + // Same name to the reader: it trims the leading space. + std::fs::write(stream.join(" 00000.m2ts"), b"b").expect("write"); + + // Precondition — if this stops holding the fixture is wrong, not the code. + assert_eq!( + crate::udf::parse_udf_name(&crate::dirimage::encode::encode_cs0(" 00000.m2ts")), + crate::udf::parse_udf_name(&crate::dirimage::encode::encode_cs0("00000.m2ts")), + "fixture: these two host names must read back identically" + ); + + let got = plan(&dir); + let _ = std::fs::remove_dir_all(&dir); + assert!( + matches!(got, Err(Error::DirNameCollision { .. })), + "a name the reader cannot distinguish must be refused, got {got:?}" + ); + } + #[test] fn an_over_long_name_is_refused_by_the_planner() { let dir = std::env::temp_dir().join(format!( diff --git a/src/dirimage/mod.rs b/src/dirimage/mod.rs index 38705ac..e8abd31 100644 --- a/src/dirimage/mod.rs +++ b/src/dirimage/mod.rs @@ -79,6 +79,9 @@ struct FileRef { host: PathBuf, disc_path: String, size: u64, + /// Host mtime at plan time — see `layout::FileNode::mtime` for why size + /// alone is not enough. + mtime: Option, } /// A synthesized UDF disc image over a host directory. @@ -128,6 +131,7 @@ impl DirImage { host: node.host.clone(), disc_path: node.disc_path.clone(), size: node.size, + mtime: node.mtime, }); let mut offset = 0u64; for e in &node.extents { @@ -206,8 +210,23 @@ impl DirImage { return Ok(&mut self.open[0].1); } let f = File::open(&self.files[file].host).map_err(Error::from)?; - let live = f.metadata().map_err(Error::from)?.len(); - if live != self.files[file].size { + let md = f.metadata().map_err(Error::from)?; + // Size AND mtime. Size alone is content-blind, and this plan depends on + // content: a DVD's VOB placement comes from bytes 0xC0/0xC4 of its IFO, + // and an IFO rewritten in place keeps its length because IFOs occupy a + // whole number of sectors. The size check would pass while every title + // extent pointed at the wrong sectors — corrupt video behind an intact + // structure, reported complete at exit 0. + // + // Only compared when both sides have a timestamp; a platform or + // filesystem that reports none simply falls back to the size check + // rather than failing every read. + let changed_size = md.len() != self.files[file].size; + let changed_mtime = match (self.files[file].mtime, md.modified().ok()) { + (Some(planned), Some(live)) => planned != live, + _ => false, + }; + if changed_size || changed_mtime { return Err(Error::DirImageFileChanged { path: self.files[file].disc_path.clone(), });