From 00b020cf2fa1d71c1b5e2f35ec805be0ad8e8404 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:51:11 -0500 Subject: [PATCH] catalog: bound keyword list path expansion and index synonym merges (#3641) * catalog: bound keyword list path expansion and index synonym merges * address review: hold keyword list writer to the path budget the reader enforces * address review: charge the emitted parent path in the keyword list writer --- crates/catalog/src/keywords.rs | 82 ++++++++++++++++++++-------- crates/catalog/src/keywords/tests.rs | 55 +++++++++++++++++++ 2 files changed, 115 insertions(+), 22 deletions(-) diff --git a/crates/catalog/src/keywords.rs b/crates/catalog/src/keywords.rs index d36ab11cd6..5fb095767b 100644 --- a/crates/catalog/src/keywords.rs +++ b/crates/catalog/src/keywords.rs @@ -87,6 +87,12 @@ pub fn from_file(flat: &[String], hierarchical: &[String]) -> Vec { /// The deepest a keyword list file may nest keywords: a file is input, so its nesting is bounded. pub const MAX_LEVELS: usize = 64; +/// The most bytes of full keyword paths a keyword list file may spell out. Every line names its +/// keyword by its whole path (`parent|child`), so a file of one long keyword with many short ones +/// inside it would otherwise grow into far more memory than its own size (CWE-400). Lightroom +/// Classic's own lists of tens of thousands of keywords come to a few MiB. +pub const MAX_LIST_PATH_BYTES: usize = 64 << 20; // 64 MiB + /// A keyword list as written by [`Catalog::keyword_list_text`]: the file's text, the keywords in /// it (paths), and what the format can't hold and so is left out (a keyword, with those inside it, /// as its path; a synonym as `path {synonym}`). @@ -114,6 +120,8 @@ pub fn parse_keyword_list(text: &str) -> Result> { let mut synonyms: std::collections::HashMap> = std::collections::HashMap::new(); // the keyword at each level above the line: its path and where it is in `out` let mut above: Vec<(String, usize)> = Vec::new(); + // bytes of full paths spelled out so far, checked against `MAX_LIST_PATH_BYTES` before each + let mut path_bytes = 0usize; for (i, line) in text.split('\n').enumerate() { let n = i + 1; if line.trim().is_empty() { @@ -162,6 +170,11 @@ pub fn parse_keyword_list(text: &str) -> Result> { return Err(bad(n, "“|” separates a keyword's levels: a name can't hold one")); } above.truncate(depth); + let parent_len = above.last().map_or(0, |(parent, _)| parent.len().saturating_add(1)); + path_bytes = path_bytes.saturating_add(parent_len.saturating_add(name.len())); + if path_bytes > MAX_LIST_PATH_BYTES { + return Err(bad(n, &format!("the keywords' full paths come to more than {} MiB", MAX_LIST_PATH_BYTES >> 20))); + } let path = match above.last() { Some((parent, _)) => format!("{parent}{SEP}{name}"), None => name.to_string(), @@ -189,6 +202,14 @@ pub fn closest<'a>(k: &str, from: &'a [String]) -> Option<&'a String> { from.iter().filter(|f| is_under(k, f)).max_by_key(|f| f.split(SEP).count()) } +/// Add each of `more` to `synonyms` that isn't there in any case, in order. One lowered set of +/// what is there, not a search of the growing list per synonym: an imported list can give a +/// keyword many synonyms, and the search was quadratic in them (CWE-407). +fn merge_synonyms(synonyms: &mut Vec, more: impl IntoIterator) { + let mut seen: std::collections::HashSet = synonyms.iter().map(|s| s.to_lowercase()).collect(); + synonyms.extend(more.into_iter().filter(|s| seen.insert(s.to_lowercase()))); +} + /// Keep the first of case-insensitively equal keywords. fn dedupe(v: &mut Vec) { let mut seen = std::collections::HashSet::new(); @@ -402,11 +423,7 @@ impl Catalog { Some(there) => { there.info.new_keywords_inside |= l.info.new_keywords_inside; there.info.keyword_shortcut |= l.info.keyword_shortcut; - for s in l.info.synonyms { - if !there.info.synonyms.iter().any(|x| same(x, &s)) { - there.info.synonyms.push(s); - } - } + merge_synonyms(&mut there.info.synonyms, l.info.synonyms); } None => { list.insert(path.to_lowercase(), ListedKeyword { path, info: l.info }); @@ -467,8 +484,11 @@ impl Catalog { let defaults = KeywordInfo::default(); let info = |path: &str| self.keyword_info(path).unwrap_or(&defaults); let mut out = ExportKeywords::default(); - let push = |v: &mut Vec, s: &str| { - if !s.is_empty() && !v.iter().any(|x| same(x, s)) { + // what each list holds, lowered: a search of the list per name was quadratic in a keyword's + // synonyms, which an imported list sets (CWE-407) + let (mut flat_seen, mut paths_seen) = (std::collections::HashSet::new(), std::collections::HashSet::new()); + let push = |v: &mut Vec, seen: &mut std::collections::HashSet, s: &str| { + if !s.is_empty() && seen.insert(s.to_lowercase()) { v.push(s.to_string()); } }; @@ -481,11 +501,11 @@ impl Catalog { let levels: Vec<&str> = path.split(SEP).collect(); let mut names = |at: usize, i: &KeywordInfo| { if let Some(name) = levels.get(at) { - push(&mut out.flat, name); + push(&mut out.flat, &mut flat_seen, name); } if i.export_synonyms { for s in &i.synonyms { - push(&mut out.flat, s.trim()); + push(&mut out.flat, &mut flat_seen, s.trim()); } } }; @@ -499,7 +519,7 @@ impl Catalog { } } } - push(&mut out.hierarchical, &path); + push(&mut out.hierarchical, &mut paths_seen, &path); } out } @@ -509,6 +529,13 @@ impl Catalog { /// keyword, a keyword left out of export in square brackets; by name. What the format can't hold /// is left out and named, so the file always reads back. pub fn keyword_list_text(&self) -> KeywordListFile { + self.keyword_list_text_within(MAX_LIST_PATH_BYTES) + } + + /// [`keyword_list_text`](Self::keyword_list_text) with the full paths written held to + /// `max_path_bytes` as [`parse_keyword_list`] holds them: a keyword past it is named + /// unwritable, with its subtree, so the file reads back. + fn keyword_list_text_within(&self, max_path_bytes: usize) -> KeywordListFile { // a name the list would read as something else, or not at all fn unwritable(name: &str) -> bool { name.trim().is_empty() @@ -519,14 +546,27 @@ impl Catalog { || (name.starts_with('[') && name.ends_with(']')) || (name.starts_with('{') && name.ends_with('}')) } - fn write(c: &Catalog, nodes: &[KeywordNode], depth: usize, file: &mut KeywordListFile) { + fn write( + c: &Catalog, + nodes: &[KeywordNode], + depth: usize, + file: &mut KeywordListFile, + path_bytes: &mut usize, + max_path_bytes: usize, + parent_len: usize, + ) { let tabs = "\t".repeat(depth); for n in nodes { // the keywords inside go with it: under its parent they'd be other keywords - if depth >= MAX_LEVELS || unwritable(&n.name) { + // (a line's full path is what the reader counts against its path budget) + // the path the reader rebuilds from the lines written: the parent as emitted, a + // separator, the name (a child's `n.path` can spell its parent in another case) + let emitted = if depth == 0 { n.name.len() } else { parent_len + SEP.len_utf8() + n.name.len() }; + if depth >= MAX_LEVELS || unwritable(&n.name) || path_bytes.saturating_add(emitted) > max_path_bytes { file.unwritable.push(n.path.clone()); continue; } + *path_bytes += emitted; let info = c.keyword_info(&n.path).cloned().unwrap_or_default(); if info.include_on_export { file.text.push_str(&format!("{tabs}{}\n", n.name)); @@ -536,19 +576,21 @@ impl Catalog { file.written.push(n.path.clone()); // the reader trims synonyms and takes one per spelling: an empty one, or another's // in a different case, is no loss; a line break or a “|” is - let mut synonyms: Vec<&str> = Vec::new(); + // (one lowered set, not a search of those written so far per synonym: quadratic + // in a keyword's synonyms, which an imported list sets — CWE-407) + let mut synonyms: std::collections::HashSet = std::collections::HashSet::new(); for s in info.synonyms.iter().map(|s| s.trim()) { - if s.is_empty() || synonyms.iter().any(|x| same(x, s)) { + if s.is_empty() || synonyms.contains(&s.to_lowercase()) { continue; } if s.chars().any(char::is_control) || s.contains(SEP) { file.unwritable.push(format!("{} {{{s}}}", n.path)); } else { - synonyms.push(s); + synonyms.insert(s.to_lowercase()); file.text.push_str(&format!("{tabs}\t{{{s}}}\n")); } } - write(c, &n.children, depth + 1, file); + write(c, &n.children, depth + 1, file, path_bytes, max_path_bytes, emitted); } } let mut file = KeywordListFile::default(); @@ -568,7 +610,7 @@ impl Catalog { } } } - write(self, &self.keyword_tree(), 0, &mut file); + write(self, &self.keyword_tree(), 0, &mut file, &mut 0, max_path_bytes, 0); file.unwritable.extend(over_deep); file } @@ -632,11 +674,7 @@ impl Catalog { // one the library has: the list's synonyms join its own (listed only if it gains some) let mut info_now = list.get(&key).map(|l| l.info.clone()).unwrap_or_default(); let before = info_now.synonyms.len(); - for s in &info.synonyms { - if !info_now.synonyms.iter().any(|x| same(x, s)) { - info_now.synonyms.push(s.clone()); - } - } + merge_synonyms(&mut info_now.synonyms, info.synonyms.iter().cloned()); if info_now.synonyms.len() > before { let path = list.get(&key).map_or(path, |l| l.path.clone()); list.insert(key, ListedKeyword { path, info: info_now }); diff --git a/crates/catalog/src/keywords/tests.rs b/crates/catalog/src/keywords/tests.rs index 6f50bd482e..6b9e505c0f 100644 --- a/crates/catalog/src/keywords/tests.rs +++ b/crates/catalog/src/keywords/tests.rs @@ -556,6 +556,17 @@ fn the_keyword_list_is_written_as_tab_indented_text() { assert_eq!(c.keyword_list_text().text, "beach\nEvents\n\tWeddings\n[Places]\n\tLisbon\n\t\t{Lisboa}\n"); } +/// The path budget the reader enforces holds for the writer too: a keyword past it, and its +/// subtree, is named unwritable instead of making a file `keyword.import` rejects. +#[test] +fn the_keyword_list_written_stays_within_the_path_budget() { + let (c, _) = lib(&[&["Places|Lisbon", "beach"], &["Events|Weddings"]]); + let file = c.keyword_list_text_within(17); + assert_eq!(file.text, "beach\nEvents\nPlaces\n"); + assert_eq!(file.unwritable, ["Events|Weddings", "Places|Lisbon"]); + assert_eq!(c.keyword_list_text().unwritable, Vec::::new()); +} + /// Reading a list gives each keyword's path and attributes: what was written comes back, and a /// file from elsewhere (Windows line ends, a byte-order mark, blank lines) reads too. #[test] @@ -765,3 +776,47 @@ fn keyword_tree_caps_a_keyword_at_the_list_depth() { let deepest = tree_paths(&c.keyword_tree()).iter().map(|(p, _)| p.split(SEP).count()).max().unwrap_or(0); assert_eq!(deepest, MAX_LEVELS); } + +/// A keyword list file spells each keyword's whole path, so one long keyword with many short ones +/// inside it expands into far more than its own size: past `MAX_LIST_PATH_BYTES` it is refused +/// with the line where that happened, instead of growing without bound (CWE-400). +#[test] +fn a_list_whose_paths_expand_past_the_budget_is_refused() { + let parent = "p".repeat(1 << 20); + let children = MAX_LIST_PATH_BYTES / parent.len() + 2; + let mut text = format!("{parent}\n"); + for i in 0..children { + text.push_str(&format!("\tc{i}\n")); + } + let err = parse_keyword_list(&text).unwrap_err().to_string(); + assert!(err.contains("full paths come to more than"), "{err}"); + // a list within the budget still reads + let ok = parse_keyword_list(&format!("{parent}\n\tchild\n")).unwrap(); + assert_eq!(ok.len(), 2); + assert_eq!(ok[1].0, format!("{parent}|child")); +} + +/// Many synonyms joining a keyword the library has, its export and its keyword list file each keep +/// every spelling once, in order, without a search of the growing list per synonym (CWE-407). +#[test] +fn many_synonyms_are_gathered_once_each_in_order() { + const N: usize = 40_000; + let mut c = Catalog::new(); + c.apply(Op::SetKeyword { path: "travel".into(), info: Some(with_synonyms(&["Trip"])) }).unwrap(); + let mut incoming: Vec = (0..N).map(|i| format!("syn{i}")).collect(); + incoming.push("TRIP".into()); + incoming.push("SYN7".into()); + let started = std::time::Instant::now(); + let (op, added, updated) = c.import_keywords_ops(&[("travel".into(), KeywordInfo { synonyms: incoming, ..KeywordInfo::default() })]); + c.apply(op).unwrap(); + assert_eq!((added, updated), (0, 1)); + let synonyms = c.keyword_info("travel").unwrap().synonyms.clone(); + assert_eq!(synonyms.len(), N + 1); + assert_eq!(synonyms[0], "Trip"); + assert_eq!(synonyms[8], "syn7"); + let exported = c.export_keywords(&["travel".into()]); + assert_eq!(exported.flat.len(), N + 2); + let file = c.keyword_list_text(); + assert_eq!(file.text.lines().count(), N + 2); + assert!(started.elapsed() < std::time::Duration::from_secs(20), "took {:?}", started.elapsed()); +}