Make four guards testable, and one guard see both spellings
The provenance guard knew one way to lose a frame's source offset — writing it as absent. Omitting the field entirely does the same thing, because the frame type fills it in by default, and the guard read straight past that. A parser rewritten into the second spelling would have kept its green light while its track silently went back to being placed by guesswork. It now reports which spelling it found and where. The title-count clamp was asserted over a fixture too small to hold more titles than the cap allows, so the walk stopped when the buffer ran out and the clamp was never what bounded it — the assertion held with the clamp deleted. The fixture now carries more entries than the cap. The subdirectory limit was checked by restating the constant's own definition; the guard itself had never run, and deleting it changed nothing. The limit is lowered under test so a real folder can exceed it, and the test now walks one and requires the refusal. One test also carried two unrelated grounding notes while the test they described had none, so reading the note above a test told you about a different one.
This commit is contained in:
+42
-15
@@ -57,7 +57,15 @@ const MAX_CS0_NAME_BYTES: usize = 254;
|
||||
/// A directory File Entry's link count is `u16` and counts one per child
|
||||
/// directory plus one for its own entry in the parent, so the last usable
|
||||
/// value is `u16::MAX - 1`.
|
||||
/// Lowered under `cfg(test)` ONLY so the guard can actually be executed.
|
||||
/// Building a folder with 65534 subdirectories to reach the real cap is not a
|
||||
/// test anyone can run, so the previous test asserted arithmetic about the
|
||||
/// constant instead and would have passed with the guard deleted. The
|
||||
/// production value is unchanged.
|
||||
#[cfg(not(test))]
|
||||
const MAX_SUBDIRS: usize = (u16::MAX - 1) as usize;
|
||||
#[cfg(test)]
|
||||
const MAX_SUBDIRS: usize = 4;
|
||||
|
||||
/// Largest image this planner will synthesize, in sectors (128 GiB).
|
||||
///
|
||||
@@ -859,16 +867,6 @@ mod tests {
|
||||
assert!(!is_excluded("VTS_01_1.VOB"));
|
||||
}
|
||||
|
||||
/// A name too long for the FID's one-byte length field is refused by the
|
||||
/// PLANNER, on a real folder.
|
||||
///
|
||||
/// Audit finding: the length was narrowed with `as u8`, so a 255-byte ASCII
|
||||
/// name — POSIX NAME_MAX, legal on ext4/APFS/NTFS — encoded to 256 bytes
|
||||
/// with the CS0 compression byte and wrote a length of ZERO, making every
|
||||
/// later entry in that directory read from the wrong offset.
|
||||
///
|
||||
/// 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.
|
||||
///
|
||||
@@ -912,6 +910,16 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// A name too long for the FID's one-byte length field is refused by the
|
||||
/// PLANNER, on a real folder.
|
||||
///
|
||||
/// Audit finding: the length was narrowed with `as u8`, so a 255-byte ASCII
|
||||
/// name — POSIX NAME_MAX, legal on ext4/APFS/NTFS — encoded to 256 bytes
|
||||
/// with the CS0 compression byte and wrote a length of ZERO, making every
|
||||
/// later entry in that directory read from the wrong offset.
|
||||
///
|
||||
/// An earlier version of this test asserted arithmetic about the constants
|
||||
/// and never called `plan`, so it would have passed with the guard deleted.
|
||||
#[test]
|
||||
fn an_over_long_name_is_refused_by_the_planner() {
|
||||
let dir = std::env::temp_dir().join(format!(
|
||||
@@ -971,11 +979,30 @@ mod tests {
|
||||
/// so this pins the arithmetic relationship the guard relies on — and says
|
||||
/// plainly that it does NOT exercise `walk`.
|
||||
#[test]
|
||||
fn the_subdir_cap_keeps_the_link_count_representable() {
|
||||
assert_eq!(
|
||||
(MAX_SUBDIRS as u16).checked_add(1),
|
||||
Some(u16::MAX),
|
||||
"the largest permitted fan-out must still fit the link count"
|
||||
fn the_subdir_cap_refuses_a_folder_with_too_many_subdirectories() {
|
||||
// Executes the guard, rather than restating the constant. 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 — so exceeding it
|
||||
// would wrap the count and produce an image whose directory structure
|
||||
// lies about itself.
|
||||
let dir = std::env::temp_dir().join(format!("fmkv-fanout-{}", std::process::id()));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
for i in 0..=MAX_SUBDIRS {
|
||||
std::fs::create_dir_all(dir.join(format!("d{i}"))).unwrap();
|
||||
}
|
||||
|
||||
let err = plan(&dir).expect_err("more subdirectories than the link count can represent");
|
||||
assert!(
|
||||
matches!(err, Error::DirImageFanout { .. }),
|
||||
"expected DirImageFanout, got {err:?}"
|
||||
);
|
||||
|
||||
// And the real production value is what ships: one per child plus the
|
||||
// parent's own entry must still fit in the 16-bit field.
|
||||
#[cfg(not(test))]
|
||||
const _: () = assert!(MAX_SUBDIRS + 1 == u16::MAX as usize);
|
||||
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
}
|
||||
|
||||
+16
-2
@@ -2746,11 +2746,25 @@ mod tests {
|
||||
/// unclamped 65535 is a ~540 MB allocation from a ~800 KB crafted file.
|
||||
#[test]
|
||||
fn tt_srpt_clamps_an_absurd_declared_title_count() {
|
||||
let mut vmg = vmg_with_tt_srpt(1, &[(1, 1, 1)]);
|
||||
// The fixture must actually CONTAIN more entries than the cap, or the
|
||||
// walk stops when the buffer runs out and the clamp is never what
|
||||
// bounded it. An earlier version of this test declared u16::MAX over a
|
||||
// two-kilobyte fixture: the loop broke on `base + 12 > len` after three
|
||||
// iterations and the assertion held with the clamp deleted entirely.
|
||||
const PRESENT: usize = MAX_TT_SRPT_TITLES + 40;
|
||||
let entries: Vec<(u16, u8, u8)> = (0..PRESENT)
|
||||
.map(|i| (1u16, 1u8, (i % 250 + 1) as u8))
|
||||
.collect();
|
||||
let mut vmg = vmg_with_tt_srpt(1, &entries);
|
||||
let off = crate::consts::SECTOR_BYTES;
|
||||
vmg[off..off + 2].copy_from_slice(&u16::MAX.to_be_bytes());
|
||||
|
||||
let map = parse_tt_srpt(&vmg, off).expect("parse");
|
||||
let total: usize = map.values().map(|v| v.len()).sum();
|
||||
assert!(total <= 99, "clamped to the format maximum, got {total}");
|
||||
assert!(
|
||||
total <= MAX_TT_SRPT_TITLES,
|
||||
"a declared count of u16::MAX over {PRESENT} real entries must be \
|
||||
clamped to {MAX_TT_SRPT_TITLES}, got {total}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
+16
-2
@@ -465,9 +465,23 @@ mod provenance_guard {
|
||||
// `PesFrame` in the tests of other modules is not ours; only
|
||||
// codec `Frame` literals are scanned, and a test fixture that
|
||||
// builds a PesPacket with `source: None` is legitimate.
|
||||
if blk.contains("source: None") {
|
||||
// Two spellings, not one. `source: None` is the obvious way to
|
||||
// lose provenance; OMITTING the field entirely is the quiet
|
||||
// one, because `Frame` derives Default, so
|
||||
// `Frame { pts_ns, .. Default::default() }` compiles and
|
||||
// yields `source: None` while containing no such text. A guard
|
||||
// that only knew the first spelling would have watched a
|
||||
// parser be rewritten into the second and stayed green.
|
||||
let explicit_none = blk.contains("source: None");
|
||||
let no_source_field = !blk.contains("source:");
|
||||
if explicit_none || no_source_field {
|
||||
let line = src[..src.find(blk).unwrap_or(0)].lines().count() + 1;
|
||||
offenders.push(format!("{name}:{line}"));
|
||||
let how = if explicit_none {
|
||||
"source: None"
|
||||
} else {
|
||||
"no source field (Default fills in None)"
|
||||
};
|
||||
offenders.push(format!("{name}:{line} ({how})"));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user