From 40cd57930aedb5d871f94865574ca05edf20ca4a Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Tue, 3 Nov 2026 11:59:16 -0800 Subject: [PATCH] font-store: confine recovery cleanup and preserve valid receipts --- Cargo.lock | 84 +++++++++++++ crates/font-store/Cargo.toml | 1 + crates/font-store/README.md | 4 + crates/font-store/src/language_packs.rs | 51 ++++++-- crates/font-store/src/language_packs/tests.rs | 116 ++++++++++++++++++ crates/font-store/src/store.rs | 13 +- 6 files changed, 257 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fd5e504f..8293bd22 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -63,6 +63,12 @@ version = "0.2.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "683d7910e743518b0e34f1186f92494becacb047c7b6bf616c96772180fef923" +[[package]] +name = "ambient-authority" +version = "0.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e9d4ee0d472d1cd2e28c97dfa124b3d8d992e10eb0a035f33f5d12e3a177ba3b" + [[package]] name = "android-activity" version = "0.6.1" @@ -514,6 +520,24 @@ dependencies = [ "thiserror 1.0.69", ] +[[package]] +name = "cap-primitives" +version = "4.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b5f74729fd2f44701d1a8eb47e906cdb3ccd9ec0f02baad85a744b791940b18" +dependencies = [ + "ambient-authority", + "fs-set-times", + "io-extras", + "io-lifetimes 3.0.1", + "ipnet", + "maybe-owned", + "rustix 1.1.5", + "rustix-linux-procfs", + "windows-sys 0.61.2", + "winx", +] + [[package]] name = "cast" version = "0.3.0" @@ -878,6 +902,7 @@ dependencies = [ name = "craft-font-store" version = "0.1.0" dependencies = [ + "cap-primitives", "craft-atomic", "craft-font-catalog", "flate2", @@ -1944,6 +1969,17 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fs-set-times" +version = "0.20.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "94e7099f6313ecacbe1256e8ff9d617b75d1bcb16a6fddef94866d225a01a14a" +dependencies = [ + "io-lifetimes 2.0.4", + "rustix 1.1.5", + "windows-sys 0.59.0", +] + [[package]] name = "fs_extra" version = "1.3.0" @@ -2825,6 +2861,28 @@ dependencies = [ "hashbrown 0.17.1", ] +[[package]] +name = "io-extras" +version = "0.19.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "20fd6de4ccfcc187e38bc21cfa543cb5a302cb86a8b114eb7f0bf0dc9f8ac00f" +dependencies = [ + "io-lifetimes 3.0.1", + "windows-sys 0.59.0", +] + +[[package]] +name = "io-lifetimes" +version = "2.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "06432fb54d3be7964ecd3649233cddf80db2832f47fec34c01f65b3d9d774983" + +[[package]] +name = "io-lifetimes" +version = "3.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2f0fb0570afe1fed943c5c3d4102d5358592d8625fda6a0007fdbe65a92fba96" + [[package]] name = "ipnet" version = "2.12.2" @@ -3145,6 +3203,12 @@ version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4050469837a6ff301cd14c1f8f24f88549e6d548f24f64e2148eb0f72cebc51f" +[[package]] +name = "maybe-owned" +version = "0.3.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4facc753ae494aeb6e3c22f839b158aebd4f9270f55cd3c79906c45476c47ab4" + [[package]] name = "memchr" version = "2.8.3" @@ -4618,6 +4682,16 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "rustix-linux-procfs" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2fc84bf7e9aa16c4f2c758f27412dc9841341e16aa682d9c7ac308fe3ee12056" +dependencies = [ + "once_cell", + "rustix 1.1.5", +] + [[package]] name = "rustls" version = "0.23.46" @@ -6816,6 +6890,16 @@ version = "0.0.29" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ef0ffc427f045c0cc9ebffd6f4f91153dcd2a1547b5c3c29ee3f541e16e95c6" +[[package]] +name = "winx" +version = "0.36.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f3fd376f71958b862e7afb20cfe5a22830e1963462f3a17f49d82a6c1d1f42d" +dependencies = [ + "bitflags 2.13.2", + "windows-sys 0.59.0", +] + [[package]] name = "wit-bindgen" version = "0.57.1" diff --git a/crates/font-store/Cargo.toml b/crates/font-store/Cargo.toml index 5d78609f..d840ff48 100644 --- a/crates/font-store/Cargo.toml +++ b/crates/font-store/Cargo.toml @@ -23,6 +23,7 @@ thiserror = { workspace = true } # The store and the network are native only; the web keeps fonts elsewhere. [target.'cfg(not(target_arch = "wasm32"))'.dependencies] +cap-primitives = "4.0.3" craft-atomic = { workspace = true } craft-font-catalog = { workspace = true } log = "0.4" diff --git a/crates/font-store/README.md b/crates/font-store/README.md index a84d88db..d54b0a92 100644 --- a/crates/font-store/README.md +++ b/crates/font-store/README.md @@ -152,6 +152,10 @@ Receipts stay until cleanup succeeds and a later record write prunes them; their reused for a new removal while they remain recorded. An installed record rebuilt after corruption has no cleanup receipts; unreceipted backups are retained for manual recovery. +Invalid receipt names are discarded without losing readable pack metadata or other valid receipts. +Cleanup retains open store, Language pack, and recovery-directory handles; it rejects symlinked +recovery ancestors. Unix cleanup resolves receipt names relative to the open recovery directory; +Windows retains ancestor handles that prevent their replacement during cleanup. **Unpacking is hostile-input safe:** pure-Rust zstd (`ruzstd`, MIT; every frame is read, and nothing but tar padding may follow the archive) and `tar` (MIT OR Apache-2.0); regular files and the `LICENCES` folder only (no links or devices). Only the contract's names are allowed: `pack.toml`, `spelling.aff`, `spelling.dic`, `hyphenation.pat` at the root and `LICENCES/` (`[A-Za-z0-9._-]`, no leading or trailing dot, no Windows device names such as `CON` or `nul.txt`). Each file at most 64 MiB, 256 MiB and 256 entries per pack. `pack.toml` is required and must name the entry's locale; the files the index promises (`spelling`, `hyphenation`) must be there. Index entries need a lowercase BCP 47 locale (listed once, not a Windows device name), a sha256 and an `https://` URL; names, licences, versions, classes and the release lose control and bidi/format characters before they are stored or listed; an optional `source` must be an `https://` URL. At most 4096 zstd frames per archive. An unreadable `language-packs.json` is copied to `language-packs.json.bad` and rebuilt from the pack folders (each `pack.toml`) atomically in place, as is a missing one when pack folders are there, so no installed pack drops out of the list. Only an install or removal (under the store lock) rewrites it; listing reads the folders meanwhile and writes nothing. diff --git a/crates/font-store/src/language_packs.rs b/crates/font-store/src/language_packs.rs index dfdb67af..8b41b0f0 100644 --- a/crates/font-store/src/language_packs.rs +++ b/crates/font-store/src/language_packs.rs @@ -478,8 +478,17 @@ impl FontStore { Stage::in_dir_excluding(&self.pack_removal_dir()?, excluded) } + /// Reject redirected or non-directory recovery ancestors before staging a removal. fn pack_removal_dir(&self) -> Result { - let parent = self.language_packs_dir().join(".removing"); + let packs = self.language_packs_dir(); + match std::fs::symlink_metadata(&packs) { + Ok(metadata) if !metadata.is_dir() || metadata.file_type().is_symlink() => { + return Err(io("using the Language pack folder")(std::io::Error::other("not a plain directory"))); + } + Err(error) if error.kind() != std::io::ErrorKind::NotFound => return Err(io("reading the Language pack folder")(error)), + _ => {} + } + let parent = packs.join(".removing"); match std::fs::symlink_metadata(&parent) { Ok(metadata) if !metadata.is_dir() || metadata.file_type().is_symlink() => { return Err(io("using the Language pack recovery folder")(std::io::Error::other("not a plain directory"))); @@ -497,6 +506,7 @@ impl FontStore { return; } let Ok(parent) = self.pack_removal_dir() else { return }; + let Ok(root) = PackRemovalRoot::open(self.dir()) else { return }; let ids = &installed.committed_removals; if ids.is_empty() { return; @@ -517,7 +527,7 @@ impl FontStore { } let id = &ids[next]; next += 1; - if let Err(error) = remove_committed_backup(&parent.join(id)) + if let Err(error) = root.remove(id, &parent) && error.kind() != std::io::ErrorKind::NotFound { log::warn!("cannot clean up committed Language pack removal {id}: {error}"); @@ -549,10 +559,12 @@ impl FontStore { Ok(rebuilt) } - /// The record as written, if there is one that can be read. + /// Preserve readable pack metadata and valid receipts when an optional receipt is malformed. fn read_record(&self) -> Option { - let installed: Installed = read_limited(&self.dir().join(INSTALLED), MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice(&b).ok())?; - installed.committed_removals.iter().all(|name| Stage::valid_name(name)).then_some(installed) + let mut installed: Installed = + read_limited(&self.dir().join(INSTALLED), MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice(&b).ok())?; + installed.committed_removals.retain(|name| Stage::valid_name(name)); + Some(installed) } /// The installed packs as their folders say: each folder named by a locale, with the name @@ -606,10 +618,31 @@ impl FontStore { } } -fn remove_committed_backup(path: &Path) -> std::io::Result<()> { - #[cfg(test)] - tests::before_cleanup(path)?; - std::fs::remove_dir_all(path) +/// Keep recovery ancestors open until cleanup ends. Unix operations use directory-relative +/// handles; Windows directory handles deny deletion and renaming while they remain open. +/// The caller-selected store is the ambient authority boundary, not an untrusted child path. +struct PackRemovalRoot { + _store: std::fs::File, + _packs: std::fs::File, + recovery: std::fs::File, +} + +impl PackRemovalRoot { + /// Open each named child without following a symlink; preserve every ancestor handle. + fn open(store: &Path) -> std::io::Result { + use cap_primitives::fs::{open_ambient_dir, open_dir_nofollow}; + let root = open_ambient_dir(store, cap_primitives::ambient_authority())?; + let packs = open_dir_nofollow(&root, Path::new("language-packs"))?; + let recovery = open_dir_nofollow(&packs, Path::new(".removing"))?; + Ok(Self { _store: root, _packs: packs, recovery }) + } + + /// Resolve only a validated receipt relative to the retained recovery directory. + fn remove(&self, id: &str, _display_parent: &Path) -> std::io::Result<()> { + #[cfg(test)] + tests::before_cleanup(&_display_parent.join(id))?; + cap_primitives::fs::remove_dir_all(&self.recovery, Path::new(id)) + } } fn finish_pack_removal(stage: Stage, backup: &Path, dest: &Path, moved: bool, saved: Result<(), Error>) -> Result<(), Error> { diff --git a/crates/font-store/src/language_packs/tests.rs b/crates/font-store/src/language_packs/tests.rs index 10ff3c7f..31755d18 100644 --- a/crates/font-store/src/language_packs/tests.rs +++ b/crates/font-store/src/language_packs/tests.rs @@ -21,6 +21,8 @@ pub(super) enum IoFailure { thread_local! { static IO_FAILURE: Cell> = const { Cell::new(None) }; static CLEANUP_ATTEMPTS: RefCell> = const { RefCell::new(Vec::new()) }; + #[cfg(unix)] + static REPLACE_CLEANUP_PARENT: RefCell> = const { RefCell::new(None) }; } struct InjectFailure(Option); @@ -45,8 +47,14 @@ pub(super) fn fail_io(failure: IoFailure) -> std::io::Result<()> { } } +/// Record attempts and inject deterministic failures or a single ancestor replacement. pub(super) fn before_cleanup(path: &Path) -> std::io::Result<()> { CLEANUP_ATTEMPTS.with_borrow_mut(|attempts| attempts.push(path.to_path_buf())); + #[cfg(unix)] + if let Some((parent, moved, outside)) = REPLACE_CLEANUP_PARENT.take() { + std::fs::rename(&parent, moved)?; + std::os::unix::fs::symlink(outside, parent)?; + } fail_io(IoFailure::Cleanup) } @@ -468,6 +476,114 @@ fn recovery_cleanup_does_not_follow_a_replaced_parent_symlink() { std::fs::remove_dir_all(store.dir()).unwrap(); } +/// The language-packs ancestor must not redirect a committed cleanup outside the store. +#[cfg(unix)] +#[test] +fn residual_cleanup_rejects_a_symlinked_language_packs_ancestor() { + let (store, _server, _pin, _) = setup("remove-symlink-packs"); + let outside = temp("remove-symlink-packs-outside"); + std::fs::create_dir_all(outside.join(".removing/11-12")).unwrap(); + std::fs::write(outside.join(".removing/11-12/keep"), b"untouched").unwrap(); + std::fs::create_dir_all(store.dir()).unwrap(); + std::os::unix::fs::symlink(&outside, store.language_packs_dir()).unwrap(); + let mut record = Installed::default(); + record.committed_removals.push("11-12".into()); + store.write_installed(&record).unwrap(); + drop(store.lock().unwrap()); + assert_eq!(std::fs::read(outside.join(".removing/11-12/keep")).unwrap(), b"untouched"); + assert!(store.pack_removal_dir().is_err()); + std::fs::remove_file(store.language_packs_dir()).unwrap(); + std::fs::remove_dir_all(outside).unwrap(); + std::fs::remove_dir_all(store.dir()).unwrap(); +} + +/// A replacement after validation must not redirect recursive deletion to the new target. +#[cfg(unix)] +#[test] +fn residual_cleanup_stays_anchored_after_recovery_parent_replacement() { + let (store, _server, _pin, _) = setup("remove-parent-race"); + let parent = store.language_packs_dir().join(".removing"); + let moved = store.language_packs_dir().join(".removing-old"); + let outside = temp("remove-parent-race-outside"); + for base in [&parent, &outside] { + std::fs::create_dir_all(base.join("11-12")).unwrap(); + std::fs::write(base.join("11-12/keep"), b"untouched").unwrap(); + } + let mut record = Installed::default(); + record.committed_removals.push("11-12".into()); + store.write_installed(&record).unwrap(); + REPLACE_CLEANUP_PARENT.set(Some((parent.clone(), moved.clone(), outside.clone()))); + drop(store.lock().unwrap()); + assert_eq!(std::fs::read(outside.join("11-12/keep")).unwrap(), b"untouched"); + assert!(!moved.join("11-12").exists(), "the originally opened recovery folder is cleaned"); + std::fs::remove_file(parent).unwrap(); + std::fs::remove_dir_all(outside).unwrap(); + std::fs::remove_dir_all(store.dir()).unwrap(); +} + +/// Retained Windows handles prevent replacement of every recovery ancestor during cleanup. +#[cfg(windows)] +#[test] +fn recovery_cleanup_locks_all_windows_ancestors_until_removal_finishes() { + let (store, _server, _pin, _) = setup("remove-windows-parent-locks"); + let parent = store.language_packs_dir().join(".removing"); + let outside = temp("remove-windows-parent-outside"); + std::fs::create_dir_all(parent.join("11-12")).unwrap(); + std::fs::create_dir_all(outside.join("11-12")).unwrap(); + std::fs::write(outside.join("11-12/keep"), b"untouched").unwrap(); + let root = PackRemovalRoot::open(store.dir()).unwrap(); + let packs = store.language_packs_dir(); + for ancestor in [parent.as_path(), packs.as_path(), store.dir()] { + assert!( + std::fs::rename(ancestor, ancestor.with_extension("replaced")).is_err(), + "an open ancestor cannot be replaced: {}", + ancestor.display() + ); + } + root.remove("11-12", &parent).unwrap(); + assert!(!parent.join("11-12").exists()); + assert_eq!(std::fs::read(outside.join("11-12/keep")).unwrap(), b"untouched"); + drop(root); + std::fs::remove_dir_all(outside).unwrap(); + std::fs::remove_dir_all(store.dir()).unwrap(); +} + +/// A malformed optional receipt must not discard valid receipts or installed-pack metadata. +#[test] +fn residual_one_bad_receipt_preserves_valid_metadata_and_cleanup() { + let (store, _server, pin, _) = setup("remove-mixed-receipts"); + let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap(); + store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap(); + let original = store.read_record().unwrap(); + let parent = store.language_packs_dir().join(".removing"); + std::fs::create_dir_all(parent.join("11-12")).unwrap(); + std::fs::write(parent.join("11-12/keep"), b"committed").unwrap(); + let mut raw = serde_json::to_value(&original).unwrap(); + raw["committed_removals"] = serde_json::json!(["11-12", "../outside"]); + std::fs::write(store.dir().join(INSTALLED), serde_json::to_vec(&raw).unwrap()).unwrap(); + let record = store.read_record().expect("one invalid receipt must not invalidate the whole record"); + assert_eq!(record.committed_removals, ["11-12"]); + assert_eq!(serde_json::to_value(&record.packs).unwrap(), serde_json::to_value(&original.packs).unwrap()); + drop(store.lock().unwrap()); + assert!(!parent.join("11-12").exists()); + std::fs::remove_dir_all(store.dir()).unwrap(); +} + +/// Exclusions do not consume filesystem candidates; hostile iterator input remains bounded. +#[test] +fn residual_stage_exclusions_leave_the_filesystem_candidate_budget() { + let parent = temp("stage-excluded-budget"); + let excluded: Vec = (0..1000).map(|n| format!("0-{n}")).collect(); + let stage = Stage::create_in(&parent, &excluded, excluded.iter().cloned().chain(std::iter::once("0-1000".into()))) + .expect("a free name after excluded receipts is still considered"); + drop(stage); + assert!(Stage::create_in(&parent, &excluded, std::iter::repeat_with(|| "0-0".into())).is_err()); + assert!(Stage::create_in(&parent, &[], std::iter::repeat_with(|| "../invalid".into())).is_err()); + std::fs::create_dir(parent.join("0-1001")).unwrap(); + assert!(Stage::create_in(&parent, &[], std::iter::repeat_with(|| "0-1001".into())).is_err()); + std::fs::remove_dir_all(parent).unwrap(); +} + #[test] fn network_off_pins_and_checksums_are_enforced() { let (store, server, pin, archive) = setup("pins"); diff --git a/crates/font-store/src/store.rs b/crates/font-store/src/store.rs index 2f5aeccb..f888cf91 100644 --- a/crates/font-store/src/store.rs +++ b/crates/font-store/src/store.rs @@ -865,17 +865,24 @@ impl Stage { name.split_once('-').is_some_and(|(pid, counter)| pid.parse::().is_ok() && counter.parse::().is_ok()) } - /// The candidate iterator also lets tests reproduce a restarted PID/counter sequence. + /// Allow distinct receipt exclusions in addition to 1,000 filesystem candidates. + /// Bound total iterator reads too, including repeated excluded or malformed names. pub(crate) fn create_in(staging: &Path, excluded: &[String], names: impl Iterator) -> Result { std::fs::create_dir_all(staging).map_err(io("creating a temporary folder"))?; // A folder of that name may be another process's (PID namespaces in Flatpak or // containers repeat ids) or a stale one: never touched here, the next name is taken // (`sweep_staging` handles ordinary download staging, not recovery folders). let mut last = None; - for name in names.take(1000) { - if !Self::valid_name(&name) || excluded.contains(&name) { + let mut candidates = 0; + let excluded: std::collections::HashSet<&str> = excluded.iter().map(String::as_str).collect(); + for name in names.take(1000usize.saturating_add(excluded.len())) { + if !Self::valid_name(&name) || excluded.contains(name.as_str()) { continue; } + if candidates == 1000 { + break; + } + candidates += 1; let dir = staging.join(name); match std::fs::create_dir(&dir) { Ok(()) => return Ok(Self { dir, remove_on_drop: true }),