Fix rc5 audit findings: keydb doc, pipeline ordering, hot-loop Arc, tests
- keydb.rs: separate default_path()/no_home_dir() doc blocks; correct the false XDG lock-step claim (Linux write path uses $HOME, ignores XDG_CONFIG_HOME; read-side search also checks XDG_CONFIG_HOME). - io/pipeline.rs: use Release/Acquire on the abandoned flag so a leaked consumer reliably skips close() on weak memory models (ARM64/POWER), not just x86 TSO. - mux/disc.rs: cache the decrypt-loss Arc at construction; lost_bytes() no longer clones an Arc per frame on the mux hot path. - disc/dvd.rs: assert display_aspect mapping for both 16:9 (PAL test) and 4:3 (NTSC test). - mux/resolve.rs: extract css_error_aborts() helper and unit-test the scrambled-but-uncracked CSS guard (Fix 6) incl. the --raw exemption. - aacs/keys.rs: add unit tests for mkb_type_raw/mkb_type/mkb_is_uhd and MkbType (Category C 2.0 UHD, prerecorded 1.0, no-0x10-record None). - release.yml: publish job needs [verify, test] so a failing test suite blocks crates.io publication.
This commit is contained in:
@@ -42,10 +42,13 @@ jobs:
|
||||
# crates.io publish is an INDEPENDENT job: it serves EXTERNAL consumers only.
|
||||
# The freemkv binaries no longer depend on it (they git-tag-pin libfreemkv via
|
||||
# a committed [patch.crates-io]), so this publish runs in parallel with their
|
||||
# release builds rather than gating them. It only `needs: verify` (the version
|
||||
# check) — NOT `test` — so publish isn't serialized behind the test suite.
|
||||
# release builds rather than gating them. It `needs: [verify, test]` so a
|
||||
# failing test suite still blocks publication to crates.io — external
|
||||
# consumers who `cargo add libfreemkv` must never receive a release whose
|
||||
# tests were failing. (The two upstream jobs run in parallel, so this gate
|
||||
# does not serialize publish behind test beyond their own completion.)
|
||||
publish:
|
||||
needs: verify
|
||||
needs: [verify, test]
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: actions/checkout@v5
|
||||
|
||||
@@ -2650,6 +2650,46 @@ mod tests {
|
||||
assert_eq!(mkb_version(&mkb), Some(0x0102_0304));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mkb_type_category_c_20_is_uhd() {
|
||||
// Type 0x10 record, BE24 length 0x0C (12). MKBType field (body
|
||||
// offset 0 = pos+4) = MKB_20_CATEGORY_C (0x48141003).
|
||||
let mkb = [
|
||||
0x10, 0x00, 0x00, 0x0C, 0x48, 0x14, 0x10, 0x03, 0x00, 0x00, 0x00, 0x01,
|
||||
];
|
||||
assert_eq!(mkb_type_raw(&mkb), Some(MKB_20_CATEGORY_C));
|
||||
assert_eq!(mkb_type(&mkb), Some(MkbType::CategoryC20));
|
||||
assert_eq!(mkb_is_uhd(&mkb), Some(true));
|
||||
assert!(MkbType::CategoryC20.is_uhd());
|
||||
assert_eq!(MkbType::CategoryC20.generation(), AacsVersion::V20);
|
||||
// Sanity on the 2.1 sibling.
|
||||
assert_eq!(MkbType::from_raw(MKB_21_CATEGORY_C), MkbType::CategoryC21);
|
||||
assert_eq!(MkbType::CategoryC21.generation(), AacsVersion::V21);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mkb_type_prerecorded_is_bluray_v10() {
|
||||
// Type 0x10 record with MKB_TYPE_4_PRERECORDED (0x00041003) — a
|
||||
// standard Blu-ray (AACS 1.0) block, not UHD.
|
||||
let mkb = [
|
||||
0x10, 0x00, 0x00, 0x0C, 0x00, 0x04, 0x10, 0x03, 0x00, 0x00, 0x00, 0x01,
|
||||
];
|
||||
assert_eq!(mkb_type(&mkb), Some(MkbType::Prerecorded));
|
||||
assert_eq!(mkb_is_uhd(&mkb), Some(false));
|
||||
assert!(!MkbType::Prerecorded.is_uhd());
|
||||
assert_eq!(MkbType::Prerecorded.generation(), AacsVersion::V10);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mkb_type_none_when_no_0x10_record() {
|
||||
// A buffer whose only record is a 0x81 (verify-media-key) record and
|
||||
// no 0x10 Type-and-Version record → mkb_type_raw returns None.
|
||||
let mkb = [0x81, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00];
|
||||
assert_eq!(mkb_type_raw(&mkb), None);
|
||||
assert_eq!(mkb_type(&mkb), None);
|
||||
assert_eq!(mkb_is_uhd(&mkb), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mkb_find_mk_dv_skips_short_verify_record() {
|
||||
// A 0x81 record with rec_len < 20 carries no full mk_dv; the finder
|
||||
|
||||
+11
-1
@@ -542,7 +542,7 @@ mod tests {
|
||||
let vmg = build_vmg(&[(1, 1, 1)]);
|
||||
let vts = build_vts(
|
||||
0,
|
||||
crate::ifo::v_atr_byte(crate::ifo::VIDEO_FORMAT_PAL, crate::ifo::ASPECT_4X3),
|
||||
crate::ifo::v_atr_byte(crate::ifo::VIDEO_FORMAT_PAL, crate::ifo::ASPECT_16X9),
|
||||
&[],
|
||||
&[],
|
||||
&[(0, 9)],
|
||||
@@ -582,6 +582,11 @@ mod tests {
|
||||
ColorSpace::Bt470bg,
|
||||
"PAL DVD is SD BT.470BG, not BT.709"
|
||||
);
|
||||
assert_eq!(
|
||||
v.display_aspect,
|
||||
Some((16, 9)),
|
||||
"ASPECT_16X9 IFO byte must map to a 16:9 display aspect"
|
||||
);
|
||||
}
|
||||
|
||||
/// NTSC DVD video is SD SMPTE-170M colorimetry (not BT.709). Mirror of the
|
||||
@@ -631,6 +636,11 @@ mod tests {
|
||||
ColorSpace::Smpte170m,
|
||||
"NTSC DVD is SD SMPTE-170M, not BT.709"
|
||||
);
|
||||
assert_eq!(
|
||||
v.display_aspect,
|
||||
Some((4, 3)),
|
||||
"ASPECT_4X3 IFO byte must map to a 4:3 display aspect"
|
||||
);
|
||||
}
|
||||
|
||||
/// AC-3 audio gets sub_stream_id 0x80 → PID routed via dvd_audio_pid
|
||||
|
||||
+7
-3
@@ -150,7 +150,11 @@ fn finish_with_grace<R: Send + 'static>(
|
||||
// observes it the moment its wedged syscall returns: it then skips
|
||||
// any further `apply` and skips `close()`, rather than running on to
|
||||
// finalise the abandoned output file.
|
||||
abandoned.store(true, Ordering::Relaxed);
|
||||
// `Release` here pairs with the `Acquire` loads in the consumer loop so
|
||||
// the leaked consumer reliably observes the flag the moment its wedged
|
||||
// syscall returns, even on weak memory models (ARM64/POWER) where
|
||||
// `Relaxed` gives no cross-thread visibility guarantee.
|
||||
abandoned.store(true, Ordering::Release);
|
||||
tracing::warn!(
|
||||
target: "freemkv::pipeline",
|
||||
phase = "finish_with_halt_grace_expired",
|
||||
@@ -299,7 +303,7 @@ impl<I: Send + 'static, R: Send + 'static> Pipeline<I, R> {
|
||||
// dead receiver, but we touch the output no further. The
|
||||
// final post-loop abandonment check returns the error
|
||||
// and skips `close()`.
|
||||
if abandoned_consumer.load(Ordering::Relaxed) {
|
||||
if abandoned_consumer.load(Ordering::Acquire) {
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -359,7 +363,7 @@ impl<I: Send + 'static, R: Send + 'static> Pipeline<I, R> {
|
||||
// MKV Cues + patching the segment header) on a file the
|
||||
// caller already reported as failed is exactly the
|
||||
// write race we must not run.
|
||||
if abandoned_consumer.load(Ordering::Relaxed) {
|
||||
if abandoned_consumer.load(Ordering::Acquire) {
|
||||
return Err(Error::Halted);
|
||||
}
|
||||
|
||||
|
||||
+17
-11
@@ -43,17 +43,6 @@ fn read_capped_to_string<R: Read>(reader: R) -> Result<String> {
|
||||
String::from_utf8(buf).map_err(|_| Error::KeydbParse)
|
||||
}
|
||||
|
||||
/// Standard keydb storage path — the canonical location to write the keydb to.
|
||||
///
|
||||
/// On Windows this is the idiomatic per-user roaming dir
|
||||
/// `%APPDATA%\freemkv\keydb.cfg`, falling back to the legacy
|
||||
/// `%USERPROFILE%\.config\freemkv\keydb.cfg` only if `APPDATA` is unset. On
|
||||
/// Linux/macOS it stays the long-standing `$HOME/.config/freemkv/keydb.cfg`.
|
||||
///
|
||||
/// The CLI's read-side search (first existing of several locations) lives in
|
||||
/// `freemkv-keysources::keydb_search_paths`; this function is the single
|
||||
/// *write* default used by `save`/`update`, kept in lock-step with that crate's
|
||||
/// `default_keydb_path` for the same OS.
|
||||
/// Build the error returned when no home directory can be determined
|
||||
/// (`HOME`/`USERPROFILE` unset). This is an *environment* failure — the
|
||||
/// process has no home dir, which typically signals a stripped container
|
||||
@@ -66,6 +55,23 @@ fn no_home_dir() -> Error {
|
||||
}
|
||||
}
|
||||
|
||||
/// Standard keydb storage path — the canonical location to write the keydb to.
|
||||
///
|
||||
/// On Windows this is the idiomatic per-user roaming dir
|
||||
/// `%APPDATA%\freemkv\keydb.cfg`, falling back to the legacy
|
||||
/// `%USERPROFILE%\.config\freemkv\keydb.cfg` only if `APPDATA` is unset. On
|
||||
/// Linux/macOS it stays the long-standing `$HOME/.config/freemkv/keydb.cfg`.
|
||||
///
|
||||
/// The CLI's read-side search (first existing of several locations) lives in
|
||||
/// `freemkv-keysources::keydb_search_paths`; this function is the single
|
||||
/// *write* default used by `save`/`update`. On Windows the two agree. On
|
||||
/// Linux they can diverge: this write path always uses
|
||||
/// `$HOME/.config/freemkv/keydb.cfg` and ignores `XDG_CONFIG_HOME`, whereas
|
||||
/// the read-side search additionally checks `$XDG_CONFIG_HOME/freemkv` first.
|
||||
/// A user who sets `XDG_CONFIG_HOME` to a non-`$HOME/.config` location will
|
||||
/// therefore have `update-keys` write to the `$HOME` path while the read-side
|
||||
/// search may prefer the `XDG_CONFIG_HOME` location — the `$HOME` path is still
|
||||
/// in the search list, so the freshly-written keydb is found, just not first.
|
||||
pub fn default_path() -> Result<PathBuf> {
|
||||
#[cfg(windows)]
|
||||
{
|
||||
|
||||
+17
-10
@@ -108,6 +108,11 @@ pub struct DiscStream {
|
||||
/// inline `decrypt::decrypt_sectors` step. `DecryptKeys::None`
|
||||
/// (raw / unencrypted disc) makes the decorator a pass-through.
|
||||
reader: DecryptingSectorSource<Box<dyn SectorSource>>,
|
||||
/// Shared decrypt-loss counter, cloned once at construction from
|
||||
/// `reader.decrypt_loss()`. `lost_bytes()` loads it directly so the
|
||||
/// per-frame hot path performs no per-call `Arc::clone` (matching the
|
||||
/// `PipelinedPesStream` pattern).
|
||||
decrypt_loss: std::sync::Arc<std::sync::atomic::AtomicU64>,
|
||||
title: DiscTitle,
|
||||
/// Mirror of the keys handed in at construction. The decorator
|
||||
/// owns the cryptographic state; this field is kept for
|
||||
@@ -253,12 +258,17 @@ impl DiscStream {
|
||||
_ => 1,
|
||||
};
|
||||
|
||||
// Wrap the input reader in DecryptingSectorSource so the internal
|
||||
// fill_extents path sees plaintext bytes. For DecryptKeys::None
|
||||
// (unencrypted / raw / test fixtures) the decorator is a pass-through.
|
||||
let reader = DecryptingSectorSource::new(reader, decrypt_keys.clone());
|
||||
// Clone the shared loss counter once here so `lost_bytes()` never
|
||||
// clones an Arc per frame on the mux hot path.
|
||||
let decrypt_loss = reader.decrypt_loss();
|
||||
|
||||
Self {
|
||||
// Wrap the input reader in DecryptingSectorSource so the
|
||||
// internal fill_extents path sees plaintext bytes. For
|
||||
// DecryptKeys::None (unencrypted / raw / test fixtures)
|
||||
// the decorator is a pass-through.
|
||||
reader: DecryptingSectorSource::new(reader, decrypt_keys.clone()),
|
||||
reader,
|
||||
decrypt_loss,
|
||||
title,
|
||||
decrypt_keys,
|
||||
unit_align,
|
||||
@@ -802,11 +812,8 @@ impl crate::pes::Stream for DiscStream {
|
||||
// them). Both are real missing content the abort gate must see; without
|
||||
// the decrypt term a partial key failure reports lost_bytes=0 and a rip
|
||||
// missing segments passes even under abort_on_lost_secs=0.
|
||||
self.lost_bytes.saturating_add(
|
||||
self.reader
|
||||
.decrypt_loss()
|
||||
.load(std::sync::atomic::Ordering::Relaxed),
|
||||
)
|
||||
self.lost_bytes
|
||||
.saturating_add(self.decrypt_loss.load(std::sync::atomic::Ordering::Relaxed))
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+25
-1
@@ -249,6 +249,17 @@ fn css_key_missing(raw: bool, has_css: bool, keys: &crate::decrypt::DecryptKeys)
|
||||
!raw && has_css && matches!(keys, crate::decrypt::DecryptKeys::None)
|
||||
}
|
||||
|
||||
/// Scrambled-but-uncracked CSS guard (Fix 6). Returns `true` when decryption
|
||||
/// is requested (`!raw`) and the scan recorded a hard CSS error
|
||||
/// (`has_css_error` — `disc.css_error.is_some()`), meaning the content is
|
||||
/// scrambled but no title key was recovered (so `disc.css` is `None`). Muxing
|
||||
/// that case would pass scrambled MPEG through as plaintext, so the caller
|
||||
/// fails fast with [`Error::CssKeyMissing`]. `--raw` is exempt (skips
|
||||
/// decryption), so it always returns `false`.
|
||||
fn css_error_aborts(raw: bool, has_css_error: bool) -> bool {
|
||||
!raw && has_css_error
|
||||
}
|
||||
|
||||
/// Open a PES input stream (produces PES frames).
|
||||
pub fn input(url: &str, opts: &InputOptions) -> io::Result<Box<dyn crate::pes::Stream>> {
|
||||
let parsed = parse_url(url);
|
||||
@@ -290,7 +301,7 @@ pub fn input(url: &str, opts: &InputOptions) -> io::Result<Box<dyn crate::pes::S
|
||||
// as "unencrypted" and the scrambled MPEG would mux as plaintext
|
||||
// garbage at exit 0. Surface the recorded hard error instead.
|
||||
// `--raw` skips decryption, so it is exempt.
|
||||
if !opts.raw && disc.css_error.is_some() {
|
||||
if css_error_aborts(opts.raw, disc.css_error.is_some()) {
|
||||
return Err(crate::error::Error::CssKeyMissing.into());
|
||||
}
|
||||
// No-key guard: if decryption is requested (not --raw) and the disc
|
||||
@@ -658,6 +669,7 @@ fn build_m2ts_pipeline<R: std::io::Read + Send + 'static>(
|
||||
mod tests {
|
||||
use super::StreamUrl;
|
||||
use super::aacs_key_missing;
|
||||
use super::css_error_aborts;
|
||||
use super::css_key_missing;
|
||||
use super::parse_url;
|
||||
use super::validate_network_addr;
|
||||
@@ -779,6 +791,18 @@ mod tests {
|
||||
assert!(!css_key_missing(false, false, &DecryptKeys::None));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn css_error_field_aborts_unless_raw() {
|
||||
// Fix 6: a scrambled-but-uncracked DVD records a hard error in
|
||||
// `disc.css_error` (css is None). With decryption requested the
|
||||
// input() guard must abort with CssKeyMissing.
|
||||
assert!(css_error_aborts(false, true));
|
||||
// --raw skips decryption → never aborts on the css_error field.
|
||||
assert!(!css_error_aborts(true, true));
|
||||
// No recorded css_error → the guard does not fire.
|
||||
assert!(!css_error_aborts(false, false));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn raw_never_aborts() {
|
||||
// --raw skips decryption — must never hit the no-key abort, even on an
|
||||
|
||||
Reference in New Issue
Block a user