refactor(decrypt): one orchestrator owns the no-key decision
There were TWO top-level decrypt paths: decrypt_sectors_impl for CSS and clear media, whose AACS arm was a bare `return Err` stub, and a wholly separate decrypt_sectors_mapped for AACS. Each scheme therefore decided its own answer to "there is no key for these bytes", and nothing held them to the same one. They drifted, in opposite directions, within a single release: css::descramble_region descrambled with a key the sector's own crib had just proven stale — garbage behind an intact clear header, reported Ok. the mapped path returned early for any LBA outside every range, before ever asking whether those bytes were ciphertext, so an unkeyable encrypted unit passed through and extract counted it as good. Both were fixed individually earlier today. This removes the shape that allowed them. decrypt_span is now the single orchestrator: it owns the loop, the refusal, and the loss count, and each scheme supplies only what genuinely differs. apply_aacs_map is a scheme step that reports what it could not open; it no longer decides what that means. The public wrappers (decrypt_sectors, _in_content, _mapped) are unchanged in signature and all funnel through it. Adding a scheme now means adding an arm here, which means answering the refusal question. That is the point. The new test asserts ONE verdict across all three schemes — AACS with no map, AACS with an encrypted unit outside every range, and CSS whose re-crack failed — plus that clear media is NOT a refusal. A per-scheme test cannot hold this: each would keep passing while the two disagreed. Flipping the AACS arm back to pass-through reds it. Also removes the last of the tracing-capture scaffolding. Serialising those captures crate-wide did not fix the 1-in-10 flake, and asserting the predicates directly made the helper, both capture subscribers and an unrelated dead OrderSink unused. Deleted rather than left behind.
This commit is contained in:
Matthew Jackson
@@ -967,102 +967,6 @@ mod tests {
|
||||
|
||||
/// Unit_Key_RO.inf is read from /AACS/DUPLICATE when the primary copy
|
||||
/// is absent (encrypt.rs `.or_else(|_| read_file(DUPLICATE/...))`).
|
||||
/// This is the damaged-primary recovery path real discs rely on.
|
||||
#[test]
|
||||
fn resolve_vid_only_falls_back_to_duplicate_unit_key_ro() {
|
||||
let mut disc = MemDisc::new();
|
||||
// Build AACS dir with a DUPLICATE subdir holding Unit_Key_RO.inf.
|
||||
let uk = vec![0x55u8; 48];
|
||||
let mut dup_fids = Vec::new();
|
||||
push_fid(&mut dup_fids, "", 70, true, true);
|
||||
push_fid(&mut dup_fids, "Unit_Key_RO.inf", 72, false, false);
|
||||
disc.put(PART_START + 72, build_file_icb(uk.len() as u32, 9000));
|
||||
disc.put_bytes(PART_START + 9000, &uk);
|
||||
disc.put(PART_START + 70, build_file_icb(dup_fids.len() as u32, 71));
|
||||
disc.put_bytes(PART_START + 71, &dup_fids);
|
||||
// AACS dir: only a DUPLICATE subdir (no primary Unit_Key_RO.inf).
|
||||
let mut aacs_fids = Vec::new();
|
||||
push_fid(&mut aacs_fids, "", 50, true, true);
|
||||
push_fid(&mut aacs_fids, "DUPLICATE", 70, true, false);
|
||||
disc.put(PART_START + 50, build_file_icb(aacs_fids.len() as u32, 51));
|
||||
disc.put_bytes(PART_START + 51, &aacs_fids);
|
||||
let mut root_fids = Vec::new();
|
||||
push_fid(&mut root_fids, "", 10, true, true);
|
||||
push_fid(&mut root_fids, "AACS", 50, true, false);
|
||||
disc.put(PART_START + 10, build_file_icb(root_fids.len() as u32, 11));
|
||||
disc.put_bytes(PART_START + 11, &root_fids);
|
||||
build_udf_skeleton(&mut disc, 10);
|
||||
let udf = udf::read_filesystem(&mut disc).expect("fs");
|
||||
|
||||
let st = Disc::resolve_vid_only(&udf, &mut disc, None).expect("DUPLICATE fallback");
|
||||
// disc_hash must be computed over the DUPLICATE bytes.
|
||||
assert_eq!(
|
||||
st.disc_hash,
|
||||
aacs::inf::disc_hash_hex(&aacs::inf::disc_hash(&uk)),
|
||||
"fallback must hash the DUPLICATE Unit_Key_RO.inf"
|
||||
);
|
||||
assert_eq!(st.uk_ro, uk);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------
|
||||
// Minimal hand-rolled `tracing::Subscriber` used ONLY to capture the
|
||||
// `has_volume_id` boolean field off the `bus_key_unavailable` warn event.
|
||||
// That field is diagnostic-only (never read back into control flow), so
|
||||
// it is otherwise invisible to `#[test]` assertions on the returned
|
||||
// `Result`. No `tracing-subscriber` dev-dependency exists in this crate,
|
||||
// hence the manual `Subscriber` impl instead of a capture layer.
|
||||
// ---------------------------------------------------------------
|
||||
|
||||
struct HasVidCapture(std::sync::Mutex<Option<bool>>);
|
||||
|
||||
impl tracing::Subscriber for HasVidCapture {
|
||||
fn enabled(&self, _metadata: &tracing::Metadata<'_>) -> bool {
|
||||
true
|
||||
}
|
||||
fn new_span(&self, _span: &tracing::span::Attributes<'_>) -> tracing::span::Id {
|
||||
tracing::span::Id::from_u64(1)
|
||||
}
|
||||
fn record(&self, _span: &tracing::span::Id, _values: &tracing::span::Record<'_>) {}
|
||||
fn record_follows_from(&self, _span: &tracing::span::Id, _follows: &tracing::span::Id) {}
|
||||
fn event(&self, event: &tracing::Event<'_>) {
|
||||
struct V<'a>(&'a HasVidCapture);
|
||||
impl tracing::field::Visit for V<'_> {
|
||||
fn record_bool(&mut self, field: &tracing::field::Field, value: bool) {
|
||||
if field.name() == "has_volume_id" {
|
||||
*self.0.0.lock().unwrap() = Some(value);
|
||||
}
|
||||
}
|
||||
fn record_debug(
|
||||
&mut self,
|
||||
_field: &tracing::field::Field,
|
||||
_value: &dyn std::fmt::Debug,
|
||||
) {
|
||||
}
|
||||
}
|
||||
event.record(&mut V(self));
|
||||
}
|
||||
fn enter(&self, _span: &tracing::span::Id) {}
|
||||
fn exit(&self, _span: &tracing::span::Id) {}
|
||||
}
|
||||
|
||||
/// `has_volume_id` must report the ACTUAL presence of a non-zero Volume ID
|
||||
/// on the handshake, not its negation.
|
||||
///
|
||||
/// Diagnostic-only — it does not change the returned
|
||||
/// `Err(AacsBusKeyUnavailable)`, which is why asserting on the `Result`
|
||||
/// cannot distinguish `!=` from `==`. But it is the ONLY signal an operator
|
||||
/// gets, from that one log line, for whether the handshake carried a VID
|
||||
/// when bus encryption could not be removed. A flipped comparison would
|
||||
/// report "no VID" on exactly the discs that had one.
|
||||
///
|
||||
/// This asserts the PREDICATE rather than the emitted `tracing` field. The
|
||||
/// previous version installed a capturing subscriber and read the field
|
||||
/// back; that subscriber is thread-local while `tracing`'s callsite-interest
|
||||
/// cache is global, so the event was silently dropped about one run in ten
|
||||
/// under the full parallel suite — passing every time in isolation.
|
||||
/// Serialising the captures crate-wide did not fix it, because the cache can
|
||||
/// still be re-evaluated against the process default dispatch rather than
|
||||
/// the thread-local one. A boolean does not need a subscriber to check.
|
||||
#[test]
|
||||
fn handshake_has_volume_id_reports_presence_not_absence() {
|
||||
let with_vid = HandshakeResult {
|
||||
|
||||
@@ -1464,71 +1464,45 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Counts `tracing` events on target `freemkv::scan`, so a test can prove
|
||||
/// a debug log fires (or doesn't) without depending on any output
|
||||
/// formatting.
|
||||
#[derive(Clone)]
|
||||
struct ScanDebugCounter(std::sync::Arc<std::sync::atomic::AtomicUsize>);
|
||||
impl tracing::Subscriber for ScanDebugCounter {
|
||||
fn enabled(&self, metadata: &tracing::Metadata<'_>) -> bool {
|
||||
metadata.target() == "freemkv::scan"
|
||||
}
|
||||
fn new_span(&self, _span: &tracing::span::Attributes<'_>) -> tracing::span::Id {
|
||||
tracing::span::Id::from_u64(1)
|
||||
}
|
||||
fn record(&self, _span: &tracing::span::Id, _values: &tracing::span::Record<'_>) {}
|
||||
fn record_follows_from(&self, _span: &tracing::span::Id, _follows: &tracing::span::Id) {}
|
||||
fn event(&self, event: &tracing::Event<'_>) {
|
||||
if event.metadata().target() == "freemkv::scan" {
|
||||
self.0.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
|
||||
}
|
||||
}
|
||||
fn enter(&self, _span: &tracing::span::Id) {}
|
||||
fn exit(&self, _span: &tracing::span::Id) {}
|
||||
}
|
||||
|
||||
/// Mutation guard for the `!` in `if !conclusive { tracing::debug!(...) }`:
|
||||
/// the "truncated; verdicts limited" log must fire exactly on an
|
||||
/// INCONCLUSIVE run, never on one that reached a designed stop.
|
||||
/// A stop reason decides whether the absence of a display set proves
|
||||
/// anything — and therefore whether the probe reports the run as truncated.
|
||||
#[test]
|
||||
fn truncated_run_logs_but_a_conclusive_run_does_not() {
|
||||
let pid = 0x1200u16;
|
||||
|
||||
// Conclusive: one exactly-sized read, extent read to its end, no stall.
|
||||
let conclusive_count = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0));
|
||||
// Serialised crate-wide — see `harness::with_captured_tracing`. These
|
||||
// race the capture in disc/encrypt.rs otherwise: the dispatch is
|
||||
// thread-local but the callsite-interest cache is global.
|
||||
crate::harness::with_captured_tracing(ScanDebugCounter(conclusive_count.clone()), || {
|
||||
let mut reader = TsReader {
|
||||
data: ts_stream(pid, &pcs_display(true)),
|
||||
pos: 0,
|
||||
};
|
||||
let mut title = pgs_title(pid, false);
|
||||
title.extents = vec![Extent {
|
||||
start_lba: 0,
|
||||
sector_count: 1,
|
||||
}];
|
||||
probe_and_set_forced(&mut reader, &mut title, &mut ForcedProbeCache::new(), None);
|
||||
});
|
||||
assert_eq!(
|
||||
conclusive_count.load(std::sync::atomic::Ordering::SeqCst),
|
||||
0,
|
||||
"a conclusive (Exhausted) run must not log the truncation debug message"
|
||||
fn a_stop_reason_decides_whether_absence_is_conclusive() {
|
||||
// The debug line at the end of `probe_and_set_forced` is gated on
|
||||
// `!stop.absence_is_conclusive()`. Assert that PREDICATE rather than
|
||||
// counting emitted events.
|
||||
//
|
||||
// Reading a log line back needs a capturing subscriber, which is
|
||||
// thread-local, while tracing's callsite-interest cache is global.
|
||||
// Those race: the sibling test in disc/encrypt.rs that did this failed
|
||||
// roughly one full-suite run in ten while passing every time in
|
||||
// isolation, and serialising the captures crate-wide was not enough —
|
||||
// the cache can still be re-evaluated against the process default
|
||||
// dispatch. A boolean does not need a subscriber to check.
|
||||
//
|
||||
// ECMA of the decision: a stop that saw everything it was ever going to
|
||||
// see (Exhausted) or stopped by DESIGN at the budget is conclusive, so
|
||||
// the absence of a display set means the track is not forced and there
|
||||
// is nothing to report. A stop that was cut short (Halted, ReadFailed)
|
||||
// is not, and that is exactly what the operator needs told.
|
||||
assert!(
|
||||
StopReason::Exhausted.absence_is_conclusive(),
|
||||
"reading every extent to its end is a complete observation"
|
||||
);
|
||||
|
||||
// Inconclusive: dies mid-title with a read error → ReadFailed.
|
||||
let truncated_count = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0));
|
||||
crate::harness::with_captured_tracing(ScanDebugCounter(truncated_count.clone()), || {
|
||||
let mut reader =
|
||||
PartialTsReader::new(ts_stream(pid, &pcs_display(true)), ThenWhat::Error);
|
||||
let mut title = multi_read_pgs_title(pid, false);
|
||||
probe_and_set_forced(&mut reader, &mut title, &mut ForcedProbeCache::new(), None);
|
||||
});
|
||||
assert_eq!(
|
||||
truncated_count.load(std::sync::atomic::Ordering::SeqCst),
|
||||
1,
|
||||
"a truncated (ReadFailed) run must log the truncation debug message exactly once"
|
||||
assert!(
|
||||
StopReason::Budget.absence_is_conclusive(),
|
||||
"the budget is a DESIGNED stop: a forced track's display sets appear \
|
||||
throughout the title, so a bounded prefix is representative. \
|
||||
Treating it as inconclusive would disable forced detection outright"
|
||||
);
|
||||
assert!(
|
||||
!StopReason::ReadFailed.absence_is_conclusive(),
|
||||
"a read that died mid-title saw less than the whole; absence proves \
|
||||
nothing and the operator must be told"
|
||||
);
|
||||
assert!(
|
||||
!StopReason::Halted.absence_is_conclusive(),
|
||||
"a cancelled probe is cut short, not complete"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user