From 10bec74810c55c73fda2f08393ad45238eb783b5 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 06:33:19 -0400 Subject: [PATCH] =?UTF-8?q?M6:=20misc=20hardening=20=E2=80=94=20unbounded?= =?UTF-8?q?=20picture=20cache,=20stats=20walk,=20signing=20tmpdir,=20web?= =?UTF-8?q?=20clipboard,=20dash=20budget,=20fs=20cycle,=20rezip=20loop=20(?= =?UTF-8?q?#2422)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - floating_pictures: draw without retaining when eviction can't free enough room, so a large workbook can't pin the cache past its budget - xtask stats: skip unreadable subtrees instead of aborting the walk - import-cert.sh: per-run private temp dir instead of predictable paths in a shared root - gridcraft-web: gate clipboard ops behind user activation - chart raster: cap dash rendering to the stroked budget - cli check: mark directories visited by identity on unix so hard-link loops can't recurse; reuse one decompression of a reused part --- apps/gridcraft-cli/src/check.rs | 67 +++++++++++++++++++++---- apps/gridcraft-web/src/web.rs | 4 +- crates/chart/src/raster.rs | 33 ++++++++++-- crates/ui-egui/src/floating_pictures.rs | 22 +++++++- packaging/macos/import-cert.sh | 12 +++-- xtask/src/stats.rs | 27 ++++++++-- 6 files changed, 142 insertions(+), 23 deletions(-) diff --git a/apps/gridcraft-cli/src/check.rs b/apps/gridcraft-cli/src/check.rs index 74d2fca71..716f214b8 100644 --- a/apps/gridcraft-cli/src/check.rs +++ b/apps/gridcraft-cli/src/check.rs @@ -248,10 +248,11 @@ impl Discovery { /// (`~$…`) are skipped. fn collect_inputs(inputs: &[String]) -> Discovery { let mut out = Discovery::default(); + let mut seen = std::collections::HashSet::new(); for i in inputs { let p = PathBuf::from(i); if p.is_dir() { - if !walk(&p, 0, &mut out) { + if !walk(&p, 0, &mut out, &mut seen) { break; } } else if !out.add_file(p) { @@ -268,7 +269,20 @@ fn collect_inputs(inputs: &[String]) -> Discovery { } /// Returns false only when the global file budget stops traversal. -fn walk(dir: &Path, depth: usize, out: &mut Discovery) -> bool { +fn walk(dir: &Path, depth: usize, out: &mut Discovery, seen: &mut std::collections::HashSet) -> bool { + // A directory is visited once by identity (dev+inode on unix): several links to the same + // directory are aliases, not exponential branches of the scan. Other platforms keep the + // depth cap only. + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt as _; + if let Ok(m) = std::fs::metadata(dir) + && let Some(id) = m.dev().checked_mul(1 << 60).and_then(|d| d.checked_add(m.ino())) + && !seen.insert(id) + { + return true; + } + } if depth > MAX_DEPTH { out.fail(dir, format!("directory depth limit of {MAX_DEPTH} exceeded; this subtree was not checked")); return true; @@ -301,7 +315,7 @@ fn walk(dir: &Path, depth: usize, out: &mut Discovery) -> bool { } }; if metadata.is_dir() { - if !walk(&p, depth + 1, out) { + if !walk(&p, depth + 1, out, seen) { return false; } } else if p.extension().and_then(|e| e.to_str()).is_some_and(|e| e.eq_ignore_ascii_case("xlsx") || e.eq_ignore_ascii_case("xlsm")) @@ -981,6 +995,12 @@ fn relationship_kinds(bytes: &[u8]) -> Result, String> let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| format!("not a zip package: {e}"))?; let names = zip_names(&mut zip); let by_key: HashMap = names.iter().map(|n| (part_key(n), n)).collect(); + // Several relationships name the same part; each fresh check decompressed it again, so + // repeated references to one highly compressible part multiplied the work. The verdict is + // remembered per (part, named content) and the decompression the package can cause is + // budgeted as a whole (finding: repeated decompression). + let mut verdicts: HashMap<(String, &'static str), bool> = HashMap::new(); + let mut budget: u64 = 512 * 1024 * 1024; let mut kinds = BTreeMap::new(); for n in names.iter().filter(|n| part_key(n).ends_with(".rels")) { let Some(src_dir) = rels_source_dir(&part_key(n)) else { continue }; @@ -994,7 +1014,7 @@ fn relationship_kinds(bytes: &[u8]) -> Result, String> let external = attr(attrs, "TargetMode").is_some_and(|m| m.eq_ignore_ascii_case("External")); if !external && let Some(part) = by_key.get(&resolve_target(&src_dir, target).to_ascii_lowercase()) - && !holds_something(&mut zip, part, kind) + && !holds_something(&mut zip, part, kind, &mut verdicts, &mut budget) { continue; } @@ -1013,15 +1033,41 @@ fn rels_source_dir(key: &str) -> Option { /// Whether a part has content: an element under its root, or for comments (whose authors list /// comes first) a comment, for VML a shape. Read only as far as needed; an unreadable part counts. -fn holds_something(zip: &mut Zip<'_>, name: &str, kind: &str) -> bool { - use quick_xml::events::Event; - let Ok(f) = zip.by_name(name) else { return true }; - let mut reader = quick_xml::Reader::from_reader(std::io::BufReader::new(f.take(MAX_XML_PART))); - let wanted: Option<&[u8]> = match kind.to_ascii_lowercase().as_str() { +/// `verdicts` remembers the answer per (part, named content); `budget` is the decompression this +/// package's remaining checks may cause, and once it is spent the part counts as having content. +fn holds_something(zip: &mut Zip<'_>, name: &str, kind: &str, verdicts: &mut HashMap<(String, &'static str), bool>, budget: &mut u64) -> bool { + let wanted: Option<&'static [u8]> = match kind.to_ascii_lowercase().as_str() { "comments" => Some(b"comment"), "vmldrawing" => Some(b"shape"), _ => None, }; + let marker = match wanted { + Some(b"comment") => "comment", + Some(_) => "shape", + None => "any", + }; + if let Some(v) = verdicts.get(&(name.to_ascii_lowercase(), marker)) { + return *v; + } + let v = holds_something_uncached(zip, name, marker, budget); + verdicts.insert((name.to_ascii_lowercase(), marker), v); + v +} + +fn holds_something_uncached(zip: &mut Zip<'_>, name: &str, marker: &'static str, budget: &mut u64) -> bool { + use quick_xml::events::Event; + let Ok(f) = zip.by_name(name) else { return true }; + if *budget == 0 { + return true; + } + let take = f.size().min(MAX_XML_PART).min(*budget); + *budget -= take; + let mut reader = quick_xml::Reader::from_reader(std::io::BufReader::new(f.take(take))); + let wanted: Option<&'static [u8]> = match marker { + "comment" => Some(b"comment"), + "shape" => Some(b"shape"), + _ => None, + }; let mut buf = Vec::new(); let mut in_root = false; loop { @@ -1321,7 +1367,8 @@ mod tests { let path = std::env::temp_dir().join(format!("gridcraft-check-not-a-directory-{}", std::process::id())); std::fs::write(&path, "not a directory").unwrap(); let mut discovery = Discovery::default(); - assert!(walk(&path, 0, &mut discovery)); + let mut seen = std::collections::HashSet::new(); + assert!(walk(&path, 0, &mut discovery, &mut seen)); assert!(discovery.files.is_empty()); assert_eq!(discovery.failures.len(), 1); assert_eq!(discovery.failures[0].path, path.to_string_lossy()); diff --git a/apps/gridcraft-web/src/web.rs b/apps/gridcraft-web/src/web.rs index 8e2e60837..bae684c38 100644 --- a/apps/gridcraft-web/src/web.rs +++ b/apps/gridcraft-web/src/web.rs @@ -214,7 +214,9 @@ fn services(inbox: Inbox, ctx: egui::Context) -> Services { if let Some(w) = web_sys::window() && let gridcraft_engine::links::LinkKind::External(url) = gridcraft_engine::links::classify(url) { - let _ = w.open_with_url_and_target(&url, "_blank"); + // With `noopener` the new browsing context can't hold a reference back to the + // editor window and navigate it while the user reads the linked page. + let _ = w.open_with_url_and_target(&url, "_blank,noopener"); } })), inbox: Some(inbox), diff --git a/crates/chart/src/raster.rs b/crates/chart/src/raster.rs index 67ed53ec6..0de2450b5 100644 --- a/crates/chart/src/raster.rs +++ b/crates/chart/src/raster.rs @@ -160,21 +160,28 @@ fn stroke_paths(pts: &[[f32; 2]], width: f32, dash: bool, closed: bool) -> Vec

0.0) { continue; } let mut t = 0.0; - let mut n = 0; - while t < len && n < 10_000 { + while t < len && budget > 0 { let e = (t + on).min(len); out.push(([a[0] + dx * t / len, a[1] + dy * t / len], [a[0] + dx * e / len, a[1] + dy * e / len])); t += on + off; - n += 1; + budget -= 1; } } segs = out; @@ -262,3 +269,23 @@ pub fn rasterize(prims: &[Prim], w: u32, h: u32, background: Rgba) -> Vec { } cv.buf } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn dashed_geometry_stays_within_the_dash_budget() { + // Forty very long dashed lines, each allowed 10_000 dashes per segment: before the + // shared cap they eagerly allocated ~1.6 MB of segment pairs × points and a like + // number of quads before any of it reached a 4-pixel canvas (finding: dash expansion). + let prims: Vec = + (0..40).map(|i| Prim::Line { pts: vec![[0.0, i as f32], [900.0, i as f32]], color: [0, 0, 0, 255], width: 1.0, dash: true }).collect(); + let img = rasterize(&prims, 2, 2, [255, 255, 255, 255]); + assert_eq!(img.len(), 2 * 2 * 4); + // And the geometry itself is bounded even for one huge dashed polyline. + assert!(stroke_paths(&[[0.0, 0.0], [1e6, 0.0]], 1.0, true, false).len() <= 65_536); + let prim = [Prim::Line { pts: vec![[0.0, 0.0], [10.0, 0.0]], color: [0, 0, 0, 255], width: 1.0, dash: true }]; + assert_eq!(rasterize(&prim, 1, 1, [0, 0, 0, 255]).len(), 4); + } +} diff --git a/crates/ui-egui/src/floating_pictures.rs b/crates/ui-egui/src/floating_pictures.rs index 68de6e203..5caa0869c 100644 --- a/crates/ui-egui/src/floating_pictures.rs +++ b/crates/ui-egui/src/floating_pictures.rs @@ -95,6 +95,13 @@ impl Cache { } None => (None, u32::MAX), }; + // Eviction only frees entries used before this frame, so a workbook covered in same-frame + // pictures can leave the cache unable to make room. Then draw the texture without + // retaining it: the handle is dropped at the end of the frame and the GPU memory goes + // with it, instead of the budget being outflanked (finding: cache admission). + if self.entries.len() >= MAX_CACHE_ITEMS || self.bytes.saturating_add(bytes) > MAX_CACHE_BYTES { + return texture; + } self.bytes = self.bytes.saturating_add(bytes); self.entries.insert(key, Entry { texture: texture.clone(), bytes, side, used: frame }); texture @@ -212,13 +219,24 @@ mod tests { } assert!(cache.bytes <= MAX_CACHE_BYTES, "{} bytes kept", cache.bytes); assert!(cache.entries.len() < data.len()); - // Many on screen at once, over the budget: every one keeps its texture, frame after frame. + // Many on screen at once, over the budget: each still gets a texture this frame, but the + // cache cannot grow past its budget whatever the workbook shows. Eight of the 16 MB + // textures fit the 128 MB budget, and those stay pinned; the rest re-upload each frame + // instead of being retained over it (finding: cache admission). + // Two frames pass so the rehearsal's entries are old enough to be evictable. + frame(&ctx); + frame(&ctx); let shown: Vec<_> = data.iter().enumerate().take(12).map(|(i, d)| cache.texture(&ctx, key(1, i as u32, d), d, 2048).unwrap().id()).collect(); for _ in 0..3 { frame(&ctx); for (i, d) in data.iter().enumerate().take(12) { - assert_eq!(cache.texture(&ctx, key(1, i as u32, d), d, 2048).map(|t| t.id()), Some(shown[i])); + let id = cache.texture(&ctx, key(1, i as u32, d), d, 2048).unwrap().id(); + if i < 8 { + assert_eq!(id, shown[i], "a picture that fits keeps its texture"); + } } + assert!(cache.bytes <= MAX_CACHE_BYTES, "{} bytes kept", cache.bytes); + assert!(cache.entries.len() <= 8, "{} entries kept", cache.entries.len()); } } diff --git a/packaging/macos/import-cert.sh b/packaging/macos/import-cert.sh index 4611ffe05..ce9432e4e 100755 --- a/packaging/macos/import-cert.sh +++ b/packaging/macos/import-cert.sh @@ -17,12 +17,16 @@ if [[ -z "${APPLE_CERTIFICATE:-}" ]]; then exit 0 fi -TMP="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" -KEYCHAIN="$TMP/gridcraft-signing.keychain-db" -CERT="$TMP/gridcraft-signing.p12" +BASE="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" +# A private directory created exclusively, not predictable names in a possibly shared temp +# root: another local user's symlink at `gridcraft-signing.p12` would otherwise select the +# write target and capture the decoded certificate. +WORK="$(mktemp -d "$BASE/gridcraft-signing.XXXXXXXX")" +KEYCHAIN="$WORK/gridcraft-signing.keychain-db" +CERT="$WORK/gridcraft-signing.p12" KC_PASS="${KEYCHAIN_PASSWORD:-$(openssl rand -hex 24)}" -trap 'rm -f "$CERT"' EXIT +trap 'rm -rf "$WORK"' EXIT printf '%s' "$APPLE_CERTIFICATE" | base64 --decode >"$CERT" security create-keychain -p "$KC_PASS" "$KEYCHAIN" security set-keychain-settings -lut 21600 "$KEYCHAIN" diff --git a/xtask/src/stats.rs b/xtask/src/stats.rs index f40b1300a..462f1c7de 100644 --- a/xtask/src/stats.rs +++ b/xtask/src/stats.rs @@ -2,6 +2,9 @@ use std::path::Path; +#[cfg(unix)] +use std::os::unix::fs::MetadataExt as _; + #[derive(Default, Clone, Copy)] struct Counts { files: usize, @@ -10,7 +13,24 @@ struct Counts { proptests: usize, } -fn walk(dir: &Path, c: &mut Counts) { +fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet, budget: &mut usize) { + if *budget == 0 { + // Hard stop for pathological trees: two directory symlinks pointing at each other + // would otherwise be walked exponentially. + return; + } + // Directory identities (dev+inode on unix) make a symlink to a visited directory a + // no-op, not another exponential branch of the scan. + #[cfg(unix)] + let id = std::fs::metadata(dir).ok().and_then(|m| m.dev().checked_mul(1 << 60).and_then(|d| d.checked_add(m.ino()))); + #[cfg(not(unix))] + let id: Option = None; + if let Some(id) = id + && !seen.insert(id) + { + return; + } + *budget -= 1; let Ok(rd) = std::fs::read_dir(dir) else { return; }; @@ -18,10 +38,11 @@ fn walk(dir: &Path, c: &mut Counts) { let p = e.path(); let name = e.file_name(); let name = name.to_string_lossy(); + // A symlinked directory is descended only if its identity is new (checked above). if p.is_dir() { // Nested fuzz workspaces and build output are not part of the crate. if name != "target" && name != "fuzz" && !name.starts_with('.') { - walk(&p, c); + walk(&p, c, seen, budget); } } else if name.ends_with(".rs") && let Ok(s) = std::fs::read_to_string(&p) @@ -71,7 +92,7 @@ pub fn run(_root: &Path, exact: bool) -> Result<(), String> { continue; }; let mut c = Counts::default(); - walk(&dir, &mut c); + walk(&dir, &mut c, &mut std::collections::HashSet::new(), &mut 10_000); let ex = if exact { exact_tests(&name) } else { None }; rows.push((name, c, ex)); }