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

@@ -87,6 +87,12 @@ pub fn from_file(flat: &[String], hierarchical: &[String]) -> Vec<String> {
 /// 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<Vec<(String, KeywordInfo)>> {
     let mut synonyms: std::collections::HashMap<usize, std::collections::HashSet<String>> = 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<Vec<(String, KeywordInfo)>> {
             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<String>, more: impl IntoIterator<Item = String>) {
+    let mut seen: std::collections::HashSet<String> = 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<String>) {
     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<String>, 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<String>, seen: &mut std::collections::HashSet<String>, 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<String> = 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 });

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::<String>::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<String> = (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());
+}