From e412b14fa3c2fa24d18c827f54039820864766b0 Mon Sep 17 00:00:00 2001 From: ljkx <177671075+ljkx@users.noreply.github.com> Date: Mon, 2 Nov 2026 18:43:43 +0100 Subject: [PATCH 1/2] update: one asset per name in SHA256SUMS.txt (#478) A line repeated as is counts once. A name listed with two different hashes is dropped, with a warning, so the result no longer depends on line order: the file has no published checksum and its install is refused, as for any file the release doesn't vouch for. --- crates/update/src/release.rs | 53 ++++++++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/crates/update/src/release.rs b/crates/update/src/release.rs index ca5419e4..3fbf785a 100644 --- a/crates/update/src/release.rs +++ b/crates/update/src/release.rs @@ -247,15 +247,32 @@ pub fn tag_from_redirect(location: &str, repo: &str) -> Option { (!rest.is_empty() && rest.len() <= 64 && rest.bytes().all(|b| b.is_ascii_alphanumeric() || b"._-+".contains(&b))).then(|| rest.to_string()) } -/// Reads `sha256sum` output: ` ` (or ` *`) per line. +/// Reads `sha256sum` output: ` ` (or ` *`) per line, one entry per name. +/// A line repeated as is counts once; a name listed with two different hashes is dropped, so the +/// file has no checksum and isn't installed, whichever line comes first. pub fn parse_sha256sums(text: &str) -> Vec<(String, String)> { - text.lines() - .filter_map(|line| { - let (hash, rest) = line.trim().split_once(char::is_whitespace)?; - let name = rest.trim_start().trim_start_matches('*'); - (is_hex64(hash) && valid_file_name(name)).then(|| (hash.to_ascii_lowercase(), name.to_string())) - }) - .collect() + let mut sums: Vec<(String, String)> = Vec::new(); + let mut conflicting: Vec = Vec::new(); + for line in text.lines() { + let Some((hash, rest)) = line.trim().split_once(char::is_whitespace) else { continue }; + let name = rest.trim_start().trim_start_matches('*'); + if !is_hex64(hash) || !valid_file_name(name) { + continue; + } + let hash = hash.to_ascii_lowercase(); + match sums.iter().find(|(_, n)| n == name) { + None => sums.push((hash, name.to_string())), + Some((h, _)) if *h == hash => {} + Some(_) => { + if !conflicting.iter().any(|n| n == name) { + log::warn!("SHA256SUMS.txt lists {name} with different checksums; ignoring it"); + conflicting.push(name.to_string()); + } + } + } + } + sums.retain(|(_, n)| !conflicting.contains(n)); + sums } /// A bare file name we'd be willing to save: no paths, no control characters. @@ -428,6 +445,26 @@ mod tests { assert_eq!(r.version, Some(Version::new(0, 5, 0))); } + #[test] + fn a_name_listed_twice_gives_one_asset_or_none() { + let other = "b".repeat(64); + // The same line twice (and the same hash in upper case) is one entry. + let sums = parse_sha256sums(&format!("{HASH} a.zip\n{HASH} *a.zip\n{} a.zip\n", HASH.to_ascii_uppercase())); + assert_eq!(sums, [(HASH.to_string(), "a.zip".to_string())]); + let r = release_from_sums(REPO, "v0.5.0", &sums); + assert_eq!(r.assets.iter().filter(|a| a.name == "a.zip").count(), 1); + // Two hashes for one name: the name is dropped in either line order, and the other + // files keep theirs. + for text in [format!("{HASH} a.zip\n{other} a.zip\n{HASH} b.zip\n"), format!("{other} a.zip\n{HASH} b.zip\n{HASH} a.zip\n")] { + let sums = parse_sha256sums(&text); + assert_eq!(sums, [(HASH.to_string(), "b.zip".to_string())], "{text}"); + let r = release_from_sums(REPO, "v0.5.0", &sums); + assert!(r.assets.iter().all(|a| a.name != "a.zip"), "{text}"); + } + // A third, matching line doesn't bring the name back. + assert!(parse_sha256sums(&format!("{HASH} a.zip\n{other} a.zip\n{HASH} a.zip\n")).is_empty()); + } + #[test] fn file_names_cannot_escape() { for bad in ["", "..", "a/b", "a\\b", ".hidden", "c:x", "x\u{0}"] { From 240a005494b3a47f8cb4f590f26313b0c20242a5 Mon Sep 17 00:00:00 2001 From: ljkx <177671075+ljkx@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:56:02 +0100 Subject: [PATCH 2/2] update: look SHA256SUMS.txt names up in a map, not by scanning A 4 MiB list holds some 60,000 names, and searching the earlier entries for each one made parsing quadratic: about 8 s for a full-size list in a release build. A map from name to position and a set of conflicting names make it linear again, with the same entries in the same order. --- crates/update/src/release.rs | 39 +++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/crates/update/src/release.rs b/crates/update/src/release.rs index 3fbf785a..ef725f48 100644 --- a/crates/update/src/release.rs +++ b/crates/update/src/release.rs @@ -3,6 +3,8 @@ //! Everything here is untrusted input. Sizes are bounded by the caller, download links must point //! at the app's own release downloads, and checksums must look like checksums. +use std::collections::{HashMap, HashSet}; + use serde::{Deserialize, Serialize}; use crate::version::{self, Version}; @@ -248,11 +250,14 @@ pub fn tag_from_redirect(location: &str, repo: &str) -> Option { } /// Reads `sha256sum` output: ` ` (or ` *`) per line, one entry per name. -/// A line repeated as is counts once; a name listed with two different hashes is dropped, so the -/// file has no checksum and isn't installed, whichever line comes first. +/// A line repeated as is counts once; a name listed with two different hashes is dropped, +/// whichever line comes first, so the list vouches for neither (a file nothing else vouches for +/// isn't installed). pub fn parse_sha256sums(text: &str) -> Vec<(String, String)> { let mut sums: Vec<(String, String)> = Vec::new(); - let mut conflicting: Vec = Vec::new(); + // Where each name is in `sums`: a 4 MiB list holds some 60,000 names, too many to search. + let mut index: HashMap<&str, usize> = HashMap::new(); + let mut conflicting: HashSet<&str> = HashSet::new(); for line in text.lines() { let Some((hash, rest)) = line.trim().split_once(char::is_whitespace) else { continue }; let name = rest.trim_start().trim_start_matches('*'); @@ -260,18 +265,19 @@ pub fn parse_sha256sums(text: &str) -> Vec<(String, String)> { continue; } let hash = hash.to_ascii_lowercase(); - match sums.iter().find(|(_, n)| n == name) { - None => sums.push((hash, name.to_string())), - Some((h, _)) if *h == hash => {} - Some(_) => { - if !conflicting.iter().any(|n| n == name) { + match index.get(name) { + None => { + index.insert(name, sums.len()); + sums.push((hash, name.to_string())); + } + Some(&i) => { + if sums.get(i).is_some_and(|(h, _)| *h != hash) && conflicting.insert(name) { log::warn!("SHA256SUMS.txt lists {name} with different checksums; ignoring it"); - conflicting.push(name.to_string()); } } } } - sums.retain(|(_, n)| !conflicting.contains(n)); + sums.retain(|(_, n)| !conflicting.contains(n.as_str())); sums } @@ -465,6 +471,19 @@ mod tests { assert!(parse_sha256sums(&format!("{HASH} a.zip\n{other} a.zip\n{HASH} a.zip\n")).is_empty()); } + #[test] + fn a_full_size_list_keeps_its_order_and_finds_a_conflict_across_it() { + // About as many names as fit in the 4 MiB a list may be, and a conflict between the + // first and last lines. + let other = "b".repeat(64); + let mut text: String = (0..57_000).map(|i| format!("{HASH} f{i}\n")).collect(); + text.push_str(&format!("{other} f0\n")); + assert!(text.len() <= 4 << 20); + let sums = parse_sha256sums(&text); + assert_eq!(sums.len(), 56_999); + assert!(sums.iter().enumerate().all(|(i, (h, n))| *h == HASH && *n == format!("f{}", i + 1))); + } + #[test] fn file_names_cannot_escape() { for bad in ["", "..", "a/b", "a\\b", ".hidden", "c:x", "x\u{0}"] {