Audit round 2 fixes: an unsafe default, four omissions, and two swallowed errors

The folder encryption probe returned "not encrypted" when it had sampled
nothing at all — a title shorter than one aligned unit skipped the loop
entirely. That verdict CLEARS the structural one an AACS directory
raised, so a genuinely encrypted folder would have been ripped as clear
and written ciphertext as video at exit 0. With no evidence it now keeps
the structural verdict, and its bounds arithmetic no longer trusts
disc-derived values not to wrap.

Reading an IFO header swallowed every I/O error and returned an empty
buffer, which sent each placement offset through unwrap_or(0) and
recorded no constraint at all — a permission error on one file produced a
silently misplaced VOB. The directory walk swallowed the same class while
claiming to skip only vanished files. Both now propagate; only NotFound
is skipped.

Four things the round-1 changes left inconsistent: two new error codes had
no doc comments, were absent from the io::Error mapping, printed no path
in Display, and were missing from the test that proves codes are distinct.
The demux sink dropped frames silently while the MKV muxer reported them.
And set_clips had been inserted INTO write_frame's doc comment, leaving
write_frame undocumented and its paragraphs describing the wrong function.

uid/gid used 0 as "not specified"; UDF's sentinel is 0xFFFFFFFF, and 0 is
root.
This commit is contained in:
Matthew Jackson
2026-08-05 17:16:03 -07:00
parent cbb3517afe
commit 60d9cc1bac
6 changed files with 88 additions and 36 deletions
+5 -2
View File
@@ -434,8 +434,11 @@ fn file_entry(
// s[34..36] ICB flags: 0 => short allocation descriptors. `udf.rs:601`
// reads exactly this word to pick its AD stride.
s[34..36].copy_from_slice(&0u16.to_le_bytes());
s[36..40].copy_from_slice(&0u32.to_le_bytes()); // uid: invalid/none
s[40..44].copy_from_slice(&0u32.to_le_bytes()); // gid: invalid/none
// UDF's sentinel for "not specified" is 0xFFFFFFFF, not 0 — 0 is a real
// uid/gid (root). A synthesized image has no meaningful owner, and a driver
// that maps these through would otherwise report every file as root-owned.
s[36..40].copy_from_slice(&u32::MAX.to_le_bytes()); // uid: not specified
s[40..44].copy_from_slice(&u32::MAX.to_le_bytes()); // gid: not specified
s[44..48].copy_from_slice(&PERM_R_X.to_le_bytes());
s[48..50].copy_from_slice(&link_count.to_le_bytes());
s[56..64].copy_from_slice(&info_len.to_le_bytes());
+18 -9
View File
@@ -228,7 +228,11 @@ fn walk(dir: &Path, disc_path: &str, depth: u32, entries: &mut usize) -> Result<
Ok(m) => m,
// A broken symlink or a file that vanished between readdir and
// stat: skip it rather than plan an extent that cannot be read.
Err(_) => continue,
// Anything else — permission denied, an I/O error on the host
// volume — is NOT a missing file, and skipping it would drop a
// real file out of the image with nothing reported.
Err(e) if e.kind() == std::io::ErrorKind::NotFound => continue,
Err(e) => return Err(Error::from(e)),
};
if !meta.is_file() {
continue;
@@ -335,16 +339,21 @@ fn be_u32(buf: &[u8], off: usize) -> Option<u32> {
}
/// Read the first `n` bytes of a host file.
fn read_head(path: &Path, n: usize) -> Vec<u8> {
/// Read the first `n` bytes of an IFO so its placement offsets can be resolved.
///
/// Errors propagate. Returning an empty buffer instead would send every offset
/// through `unwrap_or(0)`, recording NO placement constraint — and a VOB placed
/// without its constraint yields an image that reads at the wrong offset with
/// nothing reported. An IFO that cannot be read is a folder that cannot be
/// planned.
fn read_head(path: &Path, n: usize) -> Result<Vec<u8>> {
use std::io::Read;
let mut buf = vec![0u8; n];
match std::fs::File::open(path).and_then(|mut f| f.read(&mut buf)) {
Ok(got) => {
let got = std::fs::File::open(path)
.and_then(|mut f| f.read(&mut buf))
.map_err(Error::from)?;
buf.truncate(got);
buf
}
Err(_) => Vec::new(),
}
Ok(buf)
}
/// Placement order and constraints for a `VIDEO_TS` folder.
@@ -418,7 +427,7 @@ fn place_video_ts(vts: &mut DirNode, start: u32) -> Result<u32> {
if let Some(c) = class
&& c.role == Role::Ifo
{
let head = read_head(&vts.files[i].host, 0xC8);
let head = read_head(&vts.files[i].host, 0xC8)?;
let menu = be_u32(&head, 0xC0).unwrap_or(0);
if menu != 0 {
menu_req.insert(c.group, lba.saturating_add(menu));
+19 -6
View File
@@ -195,7 +195,11 @@ pub const E_DIR_IMAGE_FILE_CHANGED: u16 = 9065;
/// A `dir://` SOURCE folder does not fit a 32-bit sector address space
/// (> 2^32 sectors ≈ 8 TiB), or holds more entries than a UDF tree can carry.
pub const E_DIR_IMAGE_TOO_LARGE: u16 = 9066;
/// A name in the folder is too long to record in a UDF directory entry: the
/// File Identifier Descriptor stores the encoded length in one byte.
pub const E_DIR_NAME_TOO_LONG: u16 = 9067;
/// One directory in the folder holds more subdirectories than a UDF link count
/// can express (it is 16 bits, one per child plus one for its own entry).
pub const E_DIR_IMAGE_FANOUT: u16 = 9068;
pub const E_M2TS_PACKET_MALFORMED: u16 = 9021;
/// A `network://` output target resolved to no address that is safe to
@@ -1029,7 +1033,10 @@ impl std::fmt::Display for Error {
},
Error::Halted => write!(f, "E{}", self.code()),
Error::UdfNotFound { path } => write!(f, "E{}: {}", self.code(), path),
Error::DirImagePlacement { path } | Error::DirImageFileChanged { path } => {
Error::DirImagePlacement { path }
| Error::DirImageFileChanged { path }
| Error::DirNameTooLong { path }
| Error::DirImageFanout { path } => {
write!(f, "E{}: {}", self.code(), path)
}
Error::DiscTitleRange { index, count } => {
@@ -1183,15 +1190,19 @@ impl From<Error> for std::io::Error {
// 9027 insufficient space / 9029 write failed: a filesystem-level
// failure, not bad input.
E_DIR_INSUFFICIENT_SPACE | E_DIR_WRITE_FAILED => std::io::ErrorKind::Other,
// dir:// SOURCE gates (90619064, 9066): the folder handed in cannot
// be turned into a disc image — a 3D SSIF tree, an unsatisfiable
// VIDEO_TS placement, still-encrypted content, an unrecognized tree,
// or one too large to address. All are properties of the input.
// dir:// SOURCE gates (90619064, 90669068): the folder handed in
// cannot be turned into a disc image — a 3D SSIF tree, an
// unsatisfiable VIDEO_TS placement, still-encrypted content, an
// unrecognized tree, one too large to address, a name too long to
// record, or a directory whose fan-out overflows its link count.
// All are properties of the input, decided before a byte is read.
E_DIR_IMAGE_SSIF_UNSUPPORTED
| E_DIR_IMAGE_PLACEMENT
| E_DIR_IMAGE_ENCRYPTED
| E_DIR_IMAGE_UNSUPPORTED_TREE
| E_DIR_IMAGE_TOO_LARGE => std::io::ErrorKind::InvalidInput,
| E_DIR_IMAGE_TOO_LARGE
| E_DIR_NAME_TOO_LONG
| E_DIR_IMAGE_FANOUT => std::io::ErrorKind::InvalidInput,
// 9065: the folder changed underneath a running read. Not bad input
// at plan time — a mid-flight mutation of the source.
E_DIR_IMAGE_FILE_CHANGED => std::io::ErrorKind::InvalidData,
@@ -1539,6 +1550,8 @@ mod tests {
Error::DirImageUnsupportedTree.code(),
Error::DirImageFileChanged { path: "x".into() }.code(),
Error::DirImageTooLarge.code(),
Error::DirNameTooLong { path: "x".into() }.code(),
Error::DirImageFanout { path: "x".into() }.code(),
];
let mut sorted = codes.to_vec();
sorted.sort();
+12
View File
@@ -889,6 +889,18 @@ impl Stream for DemuxSink {
return Ok(());
}
self.finished = true;
// Same reporting as the MKV muxer's finish. Frames the playlist's clip
// marks exclude are dropped on purpose, but the count must not be
// write-only in one sink and reported in the other — an unexpected
// volume here is how a demux ends up quietly short.
let seam_dropped = self.timeline.dropped_total();
if seam_dropped > 0 {
tracing::info!(
target: "mux",
dropped = seam_dropped,
"frames outside the playlist's clip marks were dropped at clip joins"
);
}
// Flush each track's codec writer, then the buffered file.
for slot in self.tracks.iter_mut() {
if let Some(t) = slot.as_mut() {
+15 -16
View File
@@ -1323,6 +1323,21 @@ impl<W: Write + Seek> MkvMuxer<W> {
pub fn set_opening_capture(&mut self, capture: Option<crate::diag::OpeningCapture>) {
self.opening_capture = capture;
}
/// Drive seam correction from the title's PlayItem marks instead of
/// inferring it from PTS jumps.
///
/// A multi-clip Blu-ray playlist joins its clips with overlaps and skips
/// that PTS inspection cannot recover: a forward jump is indistinguishable
/// from frames lost to damaged media, and an overlap smaller than the
/// reorder threshold is invisible. Given the marks, each clip is placed at
/// the sum of the earlier clips' durations, so the output runs exactly as
/// long as the playlist says the title is.
///
/// No-op for a title with fewer than two clips or without usable marks —
/// DVD, HD-DVD and file sources keep the inference path.
pub fn set_clips(&mut self, clips: &[crate::disc::Clip]) {
self.continuity = TimelineContinuity::with_clips(clips);
}
/// Write a single frame.
///
@@ -1349,22 +1364,6 @@ impl<W: Write + Seek> MkvMuxer<W> {
/// BlockAdditional under the track's `mvcC` mapping. Such a frame is always a
/// `BlockGroup` (never a SimpleBlock), with a `ReferenceBlock` when it is not
/// a keyframe. `None` for every non-3D frame.
/// Drive seam correction from the title's PlayItem marks instead of
/// inferring it from PTS jumps.
///
/// A multi-clip Blu-ray playlist joins its clips with overlaps and skips
/// that PTS inspection cannot recover: a forward jump is indistinguishable
/// from frames lost to damaged media, and an overlap smaller than the
/// reorder threshold is invisible. Given the marks, each clip is placed at
/// the sum of the earlier clips' durations, so the output runs exactly as
/// long as the playlist says the title is.
///
/// No-op for a title with fewer than two clips or without usable marks —
/// DVD, HD-DVD and file sources keep the inference path.
pub fn set_clips(&mut self, clips: &[crate::disc::Clip]) {
self.continuity = TimelineContinuity::with_clips(clips);
}
pub fn write_frame(
&mut self,
track_idx: usize,
+18 -2
View File
@@ -520,17 +520,33 @@ fn probe_folder_encryption(reader: &mut dyn SectorSource, disc: &Disc) -> Result
};
let base = extent.start_lba;
let mut unit = vec![0u8; UNIT_SECTORS as usize * SECTOR_BYTES];
let mut sampled = 0u32;
for i in 0..AACS_PROBE_UNITS as u32 {
let lba = base + i * UNIT_SECTORS;
if lba + UNIT_SECTORS > base + extent.sector_count {
// Saturating: `start_lba` and `sector_count` come off the medium, and a
// crafted or corrupt extent must not wrap this bound into a read past
// the end of the content.
let Some(lba) = base.checked_add(i.saturating_mul(UNIT_SECTORS)) else {
break;
};
let end = base.saturating_add(extent.sector_count);
if lba.saturating_add(UNIT_SECTORS) > end {
break;
}
debug_assert!(is_unit_aligned(lba, base));
reader.read_sectors(lba, UNIT_SECTORS as u16, &mut unit, false)?;
sampled += 1;
if aacs_unit_needs_decrypt(&unit, disc.content_format) {
return Ok(true);
}
}
// Nothing was actually sampled — the largest title is shorter than one
// aligned unit, so there is no evidence either way. "Not encrypted" is the
// dangerous default here: it would clear the structural verdict an `AACS`
// directory raised and rip ciphertext as though it were video, at exit 0.
// With no evidence, keep the structural verdict.
if sampled == 0 {
return Ok(true);
}
Ok(false)
}