From 92247bea0075aac1d4c52a804ae950cb795bffce Mon Sep 17 00:00:00 2001 From: Chris Guidry Date: Thu, 30 Jul 2026 13:21:13 -0400 Subject: [PATCH] Fix diff performance, stale dead-code allows, and tar symlink escape The fidelity diff built a full Levenshtein matrix even when the two word streams were identical, and again when they differed by one word in a 10,000-word file: 424 MB for one comparison, 385 MB peak RSS and 13.9 of 14.0 seconds across the whole test suite. first_divergence now returns early on equal streams and trims the common prefix and suffix off both streams before aligning the rest, so the matrix only covers the words that actually differ. cargo test drops from 13.90 s to 0.59 s; srd verify drops from 14.23 s to 0.48 s and 385 MB to 18 MB peak RSS. MonsterFields and SpellFields carried #[allow(dead_code)] with doc comments claiming their fields were validated but never read again. subtitle.rs reads every one of them, checking them against the source's subtitle line; removing the attributes and clippy still passes clean. unpack_stripped called Entry::unpack, which does not validate a symlink entry's target before writing through it. is_safe_entry_path checks an entry's own path but not its type, so a tarball could carry a symlink entry followed by a file entry whose path walks through it, writing outside the destination once the pinned tarball hash changes. Vendored entries are now required to be a regular file or a directory. --- src/srd/fetch.rs | 74 ++++++++++++++++++++++- src/srd/verify/diff.rs | 124 +++++++++++++++++++++++++++++++++++--- src/srd/verify/monster.rs | 8 +-- src/srd/verify/spell.rs | 6 +- 4 files changed, 193 insertions(+), 19 deletions(-) diff --git a/src/srd/fetch.rs b/src/srd/fetch.rs index 19f37bc..17a5d82 100644 --- a/src/srd/fetch.rs +++ b/src/srd/fetch.rs @@ -275,8 +275,15 @@ fn unpack_stripped(tarball: &[u8], destination: &Path) -> Result<(), FetchError> if !should_vendor(&relative_path) { continue; } + let entry_type = entry.header().entry_type(); + if !is_safe_entry_type(entry_type) { + return Err(FetchError::UnsafeEntry(format!( + "{} is a {entry_type:?} entry, not a file or directory", + entry_path.display() + ))); + } let target = destination.join(&relative_path); - if entry.header().entry_type().is_dir() { + if entry_type.is_dir() { fs::create_dir_all(&target)?; } else { let file_parent = target.parent().unwrap_or(Path::new("")); @@ -297,6 +304,16 @@ fn is_safe_entry_path(path: &Path) -> bool { .any(|component| matches!(component, std::path::Component::ParentDir)) } +/// Reports whether a tar entry's type is safe to unpack: a regular file or +/// a directory, the only two shapes this vendored repo actually contains. +/// A symlink entry unpacked through `Entry::unpack` has its target used +/// unvalidated, so a later entry whose path walks through that symlink can +/// write outside `destination`; rejecting the symlink itself closes that +/// off before any such entry is ever written. +fn is_safe_entry_type(entry_type: tar::EntryType) -> bool { + entry_type.is_file() || entry_type.is_dir() +} + /// The upstream repo's top-level files that document the source and its /// licensing, rather than being part of the entry corpus itself. const VENDORED_TOP_LEVEL_FILES: [&str; 3] = ["Legal.md", "README.md", "structure.md"]; @@ -546,6 +563,39 @@ mod tests { encoder.finish().unwrap() } + /// Builds a tarball with a symlink entry (`07_Spells/x`, targeting + /// `/tmp`) followed by a regular file entry whose path walks through + /// that symlink (`07_Spells/x/evil.md`), the way a malicious upstream + /// commit could try to write outside the unpack destination via a + /// symlinked directory rather than a `..` path component. + fn tar_gz_with_a_symlink_escape(top_dir: &str) -> Vec { + let mut builder = tar::Builder::new(Vec::new()); + append_entry(&mut builder, &format!("{top_dir}/"), None); + append_entry(&mut builder, &format!("{top_dir}/07_Spells/"), None); + + let mut symlink_header = tar::Header::new_gnu(); + symlink_header + .set_path(format!("{top_dir}/07_Spells/x")) + .unwrap(); + symlink_header.set_entry_type(tar::EntryType::Symlink); + symlink_header.set_link_name("/tmp").unwrap(); + symlink_header.set_mode(0o777); + symlink_header.set_size(0); + symlink_header.set_cksum(); + builder.append(&symlink_header, io::empty()).unwrap(); + + append_entry( + &mut builder, + &format!("{top_dir}/07_Spells/x/evil.md"), + Some(b"evil"), + ); + let tar_bytes = builder.into_inner().unwrap(); + + let mut encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); + encoder.write_all(&tar_bytes).unwrap(); + encoder.finish().unwrap() + } + fn fixture_meta() -> SrdMeta { SrdMeta { version: "5.2.1".to_string(), @@ -995,6 +1045,28 @@ mod tests { server.join().unwrap(); } + #[test] + fn fetch_text_rejects_a_symlink_entry() { + let body = tar_gz_with_a_symlink_escape("dnd.srd.5.2.1-deadbeef"); + let expected = sha256_hex(&body); + let (url, server) = serve_once(body); + let config = TextFetchConfig { + url, + destination: unique_temp_dir().join("dnd.srd.5.2.1"), + expected_tarball_sha256: expected, + commit: "deadbeef".to_string(), + meta_path: unique_temp_dir().join("meta.yaml"), + }; + + let error = fetch_text(&config, &fixture_meta()).unwrap_err(); + + assert!(matches!(error, FetchError::UnsafeEntry(_))); + assert!(error.to_string().contains("07_Spells/x")); + assert!(error.to_string().contains("Symlink")); + assert!(!Path::new("/tmp/evil.md").exists()); + server.join().unwrap(); + } + #[test] fn fetch_text_rejects_a_tarball_hash_mismatch() { let body = tiny_tar_gz("dnd.srd.5.2.1-deadbeef"); diff --git a/src/srd/verify/diff.rs b/src/srd/verify/diff.rs index ccc188a..6d8cef0 100644 --- a/src/srd/verify/diff.rs +++ b/src/srd/verify/diff.rs @@ -35,18 +35,57 @@ fn template(op: &Op) -> Option { /// Describes the first divergence between `source` and `corpus`, or `None` /// if the two word streams are identical. +/// +/// Equal streams return immediately, without allocating the alignment +/// matrix. On a mismatch, the common prefix and common suffix are trimmed +/// off both streams first, so the matrix only covers the words that +/// actually differ; the trimmed words still supply context when the +/// divergence sits too close to either end of what remains. pub fn first_divergence(source: &[String], corpus: &[String]) -> Option { - let ops = edit_script(source, corpus); + if source == corpus { + return None; + } + + let prefix_len = common_prefix_len(source, corpus); + let suffix_len = common_suffix_len(&source[prefix_len..], &corpus[prefix_len..]); + let prefix = &source[..prefix_len]; + let suffix = &source[source.len() - suffix_len..]; + let source_middle = &source[prefix_len..source.len() - suffix_len]; + let corpus_middle = &corpus[prefix_len..corpus.len() - suffix_len]; + + let ops = edit_script(source_middle, corpus_middle); let (index, message) = ops .iter() .enumerate() .find_map(|(i, op)| template(op).map(|message| (i, message)))?; - let before = context_before(&ops, index); - let after = context_after(&ops, index); + let before = context_before(&ops, index, prefix); + let after = context_after(&ops, index, suffix); let context = format!("{before} ... {after}").trim().to_string(); Some(format!("{message} (context: {context})")) } +/// How many leading words `source` and `corpus` share. +fn common_prefix_len(source: &[String], corpus: &[String]) -> usize { + source + .iter() + .zip(corpus.iter()) + .take_while(|(a, b)| a == b) + .count() +} + +/// How many trailing words `source` and `corpus` share. Comparing from the +/// end and stopping at the first zipped mismatch naturally caps the count +/// at the shorter stream's length, so a stream that is a prefix of the +/// other never double-counts words already claimed by the common prefix. +fn common_suffix_len(source: &[String], corpus: &[String]) -> usize { + source + .iter() + .rev() + .zip(corpus.iter().rev()) + .take_while(|(a, b)| a == b) + .count() +} + /// Aligns `source` against `corpus` word by word, minimizing the number of /// single-word inserts, deletes, and substitutions. fn edit_script(source: &[String], corpus: &[String]) -> Vec { @@ -93,26 +132,38 @@ fn edit_script(source: &[String], corpus: &[String]) -> Vec { ops } -/// The last few equal words before `ops[index]`, in reading order. -fn context_before(ops: &[Op], index: usize) -> String { +/// The last few equal words before `ops[index]`, in reading order. When +/// `ops` itself does not hold enough equal words, the words trimmed off +/// the common prefix before the alignment ran fill in the rest. +fn context_before(ops: &[Op], index: usize, prefix: &[String]) -> String { let mut words: Vec<&str> = ops[..index] .iter() .rev() .filter_map(equal_word) .take(CONTEXT_WORDS) .collect(); + if words.len() < CONTEXT_WORDS { + let remaining = CONTEXT_WORDS - words.len(); + words.extend(prefix.iter().rev().take(remaining).map(String::as_str)); + } words.reverse(); words.join(" ") } -/// The next few equal words after `ops[index]`, in reading order. -fn context_after(ops: &[Op], index: usize) -> String { - ops[index + 1..] +/// The next few equal words after `ops[index]`, in reading order. When +/// `ops` itself does not hold enough equal words, the words trimmed off +/// the common suffix before the alignment ran fill in the rest. +fn context_after(ops: &[Op], index: usize, suffix: &[String]) -> String { + let mut words: Vec<&str> = ops[index + 1..] .iter() .filter_map(equal_word) .take(CONTEXT_WORDS) - .collect::>() - .join(" ") + .collect(); + if words.len() < CONTEXT_WORDS { + let remaining = CONTEXT_WORDS - words.len(); + words.extend(suffix.iter().take(remaining).map(String::as_str)); + } + words.join(" ") } fn equal_word(op: &Op) -> Option<&str> { @@ -130,6 +181,11 @@ mod tests { text.split_whitespace().map(str::to_string).collect() } + #[test] + fn template_is_none_for_an_equal_op() { + assert_eq!(template(&Op::Equal("a".to_string())), None); + } + #[test] fn context_skips_a_second_nearby_divergence() { let message = first_divergence(&words("a b c d e f g"), &words("a b X d Y f g")).unwrap(); @@ -141,6 +197,54 @@ mod tests { assert_eq!(first_divergence(&words("a b c"), &words("a b c")), None); } + #[test] + fn common_prefix_len_counts_leading_shared_words() { + assert_eq!(common_prefix_len(&words("a b c"), &words("a b x")), 2); + } + + #[test] + fn common_prefix_len_is_zero_without_a_shared_first_word() { + assert_eq!(common_prefix_len(&words("a b c"), &words("x b c")), 0); + } + + #[test] + fn common_suffix_len_counts_trailing_shared_words() { + assert_eq!(common_suffix_len(&words("a b c"), &words("x b c")), 2); + } + + #[test] + fn common_suffix_len_caps_at_the_shorter_streams_length() { + assert_eq!(common_suffix_len(&words("a b c"), &words("c")), 1); + } + + #[test] + fn reports_an_extra_trailing_word_when_source_is_a_strict_prefix() { + let message = first_divergence(&words("a b"), &words("a b b")).unwrap(); + assert!(message.contains("adds")); + assert!(message.contains("\"b\"")); + } + + #[test] + fn reports_a_missing_trailing_word_when_corpus_is_a_strict_prefix() { + let message = first_divergence(&words("a b b"), &words("a b")).unwrap(); + assert!(message.contains("drops it")); + assert!(message.contains("\"b\"")); + } + + #[test] + fn a_single_word_change_deep_in_a_long_stream_stays_isolated() { + let mut source_text = "filler ".repeat(500); + source_text.push_str("roar"); + let mut corpus_text = "filler ".repeat(500); + corpus_text.push_str("roars"); + + let message = first_divergence(&words(&source_text), &words(&corpus_text)).unwrap(); + + assert!(message.contains("\"roar\"")); + assert!(message.contains("\"roars\"")); + assert!(message.contains("context: filler filler filler ...")); + } + #[test] fn reports_a_dropped_word() { let message = first_divergence( diff --git a/src/srd/verify/monster.rs b/src/srd/verify/monster.rs index 92a1310..5814cbc 100644 --- a/src/srd/verify/monster.rs +++ b/src/srd/verify/monster.rs @@ -29,11 +29,9 @@ pub struct Ability { /// renders to the same comma-joined text for the stat-block word count. /// /// `size`, `creature_type`, and `alignment` are validated here (present, -/// non-empty) but not read again afterward: their words live in the -/// source's italic type line, outside the stat-block region, so the -/// ordinary body word-stream check is what holds them to the source, not -/// this struct. -#[allow(dead_code)] +/// non-empty), then read again by `subtitle::check_monster`, which checks +/// them against the source's italic type line, outside the stat-block +/// region that the ordinary body word-stream check covers. pub struct MonsterFields { pub size: String, pub creature_type: String, diff --git a/src/srd/verify/spell.rs b/src/srd/verify/spell.rs index a2d1684..d099be5 100644 --- a/src/srd/verify/spell.rs +++ b/src/srd/verify/spell.rs @@ -18,11 +18,11 @@ const SCHOOLS: [&str; 8] = [ ]; /// A spell's mechanical frontmatter, validated. `level`, `school`, and -/// `classes` are validated here (range, enum, lowercase) but not read -/// again afterward; the field-line drift check only re-checks the fields +/// `classes` are validated here (range, enum, lowercase), then read again +/// by `subtitle::check_spell`, which checks them against the source's +/// subtitle line; the field-line drift check only re-checks the fields /// that duplicate a bold line in the body (`casting_time`, `range`, /// `components`, `duration`). -#[allow(dead_code)] pub struct SpellFields { pub level: i64, pub school: String, -- 2.51.2