From de727cb97e8fed21f77f2d11cc61aa1d4286b74d Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Tue, 3 Nov 2026 08:12:39 -0800 Subject: [PATCH] font-store: preserve Language packs when removal cannot commit --- crates/font-store/README.md | 8 + crates/font-store/src/language_packs.rs | 91 +++++++++-- crates/font-store/src/language_packs/tests.rs | 142 ++++++++++++++++++ crates/font-store/src/store.rs | 48 ++++-- 4 files changed, 270 insertions(+), 19 deletions(-) diff --git a/crates/font-store/README.md b/crates/font-store/README.md index 520b0e0b..7751eefd 100644 --- a/crates/font-store/README.md +++ b/crates/font-store/README.md @@ -138,6 +138,14 @@ let installed = store.installed_language_packs(); | `installed_language_packs()` | The recorded packs whose folder is there. | | `language_packs_dir()` | `/language-packs`, for `craft_proof::packs::set_packs_dir`. | +Removal restores the original folder if the installed-record update fails. If restoration also +fails, the error names a preserved backup under `language-packs/.removing/`. That recovery +folder is outside the ordinary download staging sweep; cleanup does not delete the backup. +Committed removals record their backup IDs in the same atomic record update. The next locked +store operation retries cleanup of those backups; failed rollback backups have no receipt and +remain available for recovery. Receipts stay until cleanup succeeds and cannot be reused for a +new removal while they remain recorded. + **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. **Developer override (testing only — bypasses the pin):** `CRAFT_LANGUAGE_PACK_INDEX=` replaces the pinned index URL with an `http://` URL on a loopback host (`127.0.0.1`, `localhost`, `[::1]`), an `https://` URL or a local file path (`file://` URLs are not taken), and **skips the index's sha256 pin**. Archives are still checked against the sha256 and size in that index, and may then also be loopback `http://` URLs or local paths. Off unless the variable is set; it works in release builds too (for end-to-end tests of a real build), a warning is logged whenever it is used, and `language_pack_index_overridden() -> Option` returns the value so apps can show that the pin is bypassed. Never set it for users. diff --git a/crates/font-store/src/language_packs.rs b/crates/font-store/src/language_packs.rs index 282d5909..b8cc3d03 100644 --- a/crates/font-store/src/language_packs.rs +++ b/crates/font-store/src/language_packs.rs @@ -125,6 +125,9 @@ pub struct InstalledLanguagePack { struct Installed { version: u32, packs: Vec, + /// Committed backup IDs, recorded atomically with removing their pack entries. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + committed_removals: Vec, } impl PackIndex { @@ -442,20 +445,63 @@ impl FontStore { if !is_locale(locale) { return Err(Error::NotInstalled(locale.chars().take(40).collect())); } - let stage = Stage::new(self.dir())?; let _lock = self.lock()?; let dest = self.language_packs_dir().join(locale); let mut installed = self.read_installed()?; let known = installed.packs.iter().any(|p| p.locale == locale); - if !known && std::fs::symlink_metadata(&dest).is_err() { + let has_folder = std::fs::symlink_metadata(&dest).is_ok(); + if !known && !has_folder { return Err(Error::NotInstalled(locale.to_string())); } - if std::fs::symlink_metadata(&dest).is_ok() { - std::fs::rename(&dest, stage.dir.join(locale)).map_err(io(format!("removing the {locale} pack")))?; + let stage = self.language_pack_removal_stage(&installed.committed_removals)?; + let backup = stage.dir.join(locale); + if has_folder { + let id = stage + .dir + .file_name() + .and_then(|name| name.to_str()) + .filter(|name| Stage::valid_name(name)) + .ok_or_else(|| io("recording a Language pack removal")(std::io::Error::other("invalid backup name")))? + .to_string(); + std::fs::rename(&dest, &backup).map_err(io(format!("removing the {locale} pack")))?; + installed.committed_removals.push(id); } installed.packs.retain(|p| p.locale != locale); - self.write_installed(&installed) - // `stage` deletes the old folder when dropped. + finish_pack_removal(stage, &backup, &dest, has_folder, self.write_installed(&installed)) + } + + fn language_pack_removal_stage(&self, excluded: &[String]) -> Result { + // A failed rollback or interrupted removal must not lose its backup to the download + // sweep. Locale scans ignore this hidden directory. + Stage::in_dir_excluding(&self.pack_removal_dir()?, excluded) + } + + fn pack_removal_dir(&self) -> Result { + let parent = self.language_packs_dir().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"))); + } + Err(error) if error.kind() != std::io::ErrorKind::NotFound => return Err(io("reading the Language pack recovery folder")(error)), + _ => {} + } + Ok(parent) + } + + /// Called only after acquiring the store lock: a snapshot must not delete a newly reused ID. + pub(crate) fn sweep_committed_pack_removals(&self) { + let Some(installed) = self.read_record() else { return }; + if installed.version != 1 { + return; + } + let Ok(parent) = self.pack_removal_dir() else { return }; + for id in installed.committed_removals { + if let Err(error) = std::fs::remove_dir_all(parent.join(&id)) + && error.kind() != std::io::ErrorKind::NotFound + { + log::warn!("cannot clean up committed Language pack removal {id}: {error}"); + } + } } /// The record of installed packs. One that can't be read is copied aside as @@ -482,7 +528,8 @@ impl FontStore { /// The record as written, if there is one that can be read. fn read_record(&self) -> Option { - read_limited(&self.dir().join(INSTALLED), MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice::(&b).ok()) + 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) } /// The installed packs as their folders say: each folder named by a locale, with the name @@ -512,16 +559,42 @@ impl FontStore { }) .collect(); packs.sort_by(|a, b| a.locale.cmp(&b.locale)); - Installed { version: 1, packs } + Installed { version: 1, packs, committed_removals: Vec::new() } } fn write_installed(&self, installed: &Installed) -> Result<(), Error> { - let bytes = serde_json::to_vec_pretty(&Installed { version: 1, packs: installed.packs.clone() }) + let parent = self.language_packs_dir().join(".removing"); + // Keep receipts until their backup has actually disappeared. A future locked sweep + // retries failed cleanup, and stage creation excludes IDs whose receipts remain. + let committed_removals = installed + .committed_removals + .iter() + .filter(|id| { + Stage::valid_name(id) + && !matches!(std::fs::symlink_metadata(parent.join(id)), Err(error) if error.kind() == std::io::ErrorKind::NotFound) + }) + .cloned() + .collect(); + let bytes = serde_json::to_vec_pretty(&Installed { version: 1, packs: installed.packs.clone(), committed_removals }) .map_err(|e| io("saving the Language pack list")(std::io::Error::other(e)))?; self.write_file(INSTALLED, &bytes) } } +fn finish_pack_removal(stage: Stage, backup: &Path, dest: &Path, moved: bool, saved: Result<(), Error>) -> Result<(), Error> { + if let Err(error) = saved { + if moved && let Err(restore) = std::fs::rename(backup, dest) { + stage.keep_for_recovery(); + return Err(io(format!("saving the Language pack list failed ({error}); the original pack is preserved at {}", backup.display()))( + restore, + )); + } + return Err(error); + } + // The record committed the removal; dropping the stage cleans up its old files. + Ok(()) +} + /// The zstd frames of `archive` one after another, as one stream (`zstd` writes one frame, but /// a concatenation of frames is a valid `.zst` too, and must not stop after the first). struct Frames<'a> { diff --git a/crates/font-store/src/language_packs/tests.rs b/crates/font-store/src/language_packs/tests.rs index 813ee1c1..d6b0d0ae 100644 --- a/crates/font-store/src/language_packs/tests.rs +++ b/crates/font-store/src/language_packs/tests.rs @@ -171,6 +171,148 @@ fn install_list_replace_and_remove_a_pack() { assert!(matches!(store.remove_language_pack("../x"), Err(Error::NotInstalled(_)))); } +#[cfg(unix)] +#[test] +fn a_failed_pack_removal_preserves_the_files_and_installed_record() { + use std::os::unix::fs::PermissionsExt; + for record_failure in [true, false] { + let (store, _server, pin, _) = setup(if record_failure { "remove-record-failure" } else { "remove-rename-failure" }); + let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap(); + let got = store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap(); + let dictionary = store.language_packs_dir().join("nl/spelling.dic"); + let before_dictionary = std::fs::read(&dictionary).unwrap(); + let before_record = std::fs::read(store.dir().join(INSTALLED)).unwrap(); + drop(store.language_pack_removal_stage(&[]).unwrap()); + // The staging and pack subdirectories remain writable when only the store root + // rejects the atomic record replacement. The other case rejects the pack rename. + let protected = if record_failure { store.dir().to_path_buf() } else { store.language_packs_dir() }; + let permissions = std::fs::metadata(&protected).unwrap().permissions(); + std::fs::set_permissions(&protected, std::fs::Permissions::from_mode(0o555)).unwrap(); + let probe = protected.join("write-probe"); + if std::fs::File::create(&probe).is_ok() { + std::fs::set_permissions(&protected, permissions).unwrap(); + std::fs::remove_dir_all(store.dir()).unwrap(); + continue; // root ignores permissions + } + let result = store.remove_language_pack("nl"); + std::fs::set_permissions(&protected, permissions).unwrap(); + assert!(matches!(result, Err(Error::Io { .. })), "{result:?}"); + assert_eq!(std::fs::read(&dictionary).unwrap(), before_dictionary, "record failure: {record_failure}"); + assert_eq!(std::fs::read(store.dir().join(INSTALLED)).unwrap(), before_record); + assert_eq!(store.installed_language_packs(), vec![got]); + assert!(std::fs::read_dir(store.dir().join(".staging")).unwrap().next().is_none()); + assert!(std::fs::read_dir(store.language_packs_dir().join(".removing")).unwrap().next().is_none()); + std::fs::remove_dir_all(store.dir()).unwrap(); + } +} + +#[test] +fn a_failed_removal_rollback_keeps_a_recoverable_pack_outside_download_cleanup() { + let (store, _server, pin, _) = setup("remove-rollback-failure"); + let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap(); + let got = store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap(); + let before_record = std::fs::read(store.dir().join(INSTALLED)).unwrap(); + let dest = store.language_packs_dir().join("nl"); + let before_dictionary = std::fs::read(dest.join("spelling.dic")).unwrap(); + let stage = store.language_pack_removal_stage(&[]).unwrap(); + let stage_dir = stage.dir.clone(); + let backup = stage_dir.join("nl"); + std::fs::rename(&dest, &backup).unwrap(); + // A replacement appearing at the original path makes restoration fail. Keep it too. + std::fs::write(&dest, b"replacement").unwrap(); + let failed = io("saving the Language pack list")(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "injected record failure")); + let result = finish_pack_removal(stage, &backup, &dest, true, Err(failed)); + assert!(matches!(result, Err(Error::Io { .. }))); + assert!(result.unwrap_err().to_string().contains(&backup.display().to_string())); + #[cfg(unix)] + { + let old = std::time::SystemTime::now() - Duration::from_secs(7200); + std::fs::File::open(&stage_dir).unwrap().set_times(std::fs::FileTimes::new().set_modified(old)).unwrap(); + } + store.sweep_staging(); + drop(store.lock().unwrap()); + assert_eq!(std::fs::read(backup.join("spelling.dic")).unwrap(), before_dictionary); + assert_eq!(std::fs::read(&dest).unwrap(), b"replacement"); + assert_eq!(std::fs::read(store.dir().join(INSTALLED)).unwrap(), before_record); + std::fs::remove_file(&dest).unwrap(); + std::fs::rename(&backup, &dest).unwrap(); + assert_eq!(store.installed_language_packs(), vec![got]); + std::fs::remove_dir_all(store.dir()).unwrap(); +} + +#[test] +fn committed_removal_cleanup_survives_interruption_and_does_not_reuse_a_recorded_id() { + let (store, _server, pin, _) = setup("remove-committed-cleanup"); + 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 mut installed = store.read_installed().unwrap(); + let stage = store.language_pack_removal_stage(&installed.committed_removals).unwrap(); + let id = stage.dir.file_name().unwrap().to_str().unwrap().to_string(); + let backup = stage.dir.join("nl"); + std::fs::rename(store.language_packs_dir().join("nl"), &backup).unwrap(); + installed.packs.clear(); + installed.committed_removals.push(id.clone()); + store.write_installed(&installed).unwrap(); + stage.keep_for_recovery(); // Simulate exiting after the atomic record commit, before Drop. + assert!(backup.join("spelling.dic").is_file()); + let _lock = store.lock().unwrap(); + assert!(!backup.exists(), "a committed leftover is cleaned at the next locked store operation"); + let installed = store.read_installed().unwrap(); + assert_eq!(installed.committed_removals.as_slice(), std::slice::from_ref(&id)); + // A restarted PID/counter could propose the old name after cleanup removed its directory. + // Its still-recorded receipt must exclude it until the next record update prunes it. + let parent = store.pack_removal_dir().unwrap(); + let fresh = Stage::create_in(&parent, &installed.committed_removals, [id.clone(), "0-0".into()].into_iter()).unwrap(); + assert_eq!(fresh.dir.file_name().unwrap(), "0-0"); + drop(fresh); + store.write_installed(&installed).unwrap(); + assert!(store.read_record().unwrap().committed_removals.is_empty()); + assert!(store.installed_language_packs().is_empty()); + drop(_lock); + std::fs::remove_dir_all(store.dir()).unwrap(); +} + +#[test] +fn removal_receipts_cannot_name_paths_outside_the_recovery_folder() { + let (store, _server, pin, _) = setup("remove-invalid-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 outside = temp("remove-outside"); + std::fs::write(outside.join("keep"), b"untouched").unwrap(); + let before = std::fs::read(store.dir().join(INSTALLED)).unwrap(); + for bad in ["../outside".to_string(), outside.display().to_string(), "1-2/../../outside".to_string()] { + let mut record: serde_json::Value = serde_json::from_slice(&before).unwrap(); + record["committed_removals"] = serde_json::json!([bad]); + std::fs::write(store.dir().join(INSTALLED), serde_json::to_vec(&record).unwrap()).unwrap(); + drop(store.lock().unwrap()); + assert_eq!(std::fs::read(outside.join("keep")).unwrap(), b"untouched"); + } + std::fs::remove_dir_all(outside).unwrap(); + std::fs::remove_dir_all(store.dir()).unwrap(); +} + +#[cfg(unix)] +#[test] +fn recovery_cleanup_does_not_follow_a_replaced_parent_symlink() { + let (store, _server, pin, _) = setup("remove-symlink-parent"); + 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 outside = temp("remove-symlink-outside"); + std::fs::create_dir(outside.join("11-12")).unwrap(); + std::fs::write(outside.join("11-12/keep"), b"untouched").unwrap(); + std::os::unix::fs::symlink(&outside, store.language_packs_dir().join(".removing")).unwrap(); + let mut record = store.read_installed().unwrap(); + record.committed_removals.push("11-12".into()); + store.write_installed(&record).unwrap(); + drop(store.lock().unwrap()); + assert!(matches!(store.remove_language_pack("nl"), Err(Error::Io { .. }))); + assert_eq!(std::fs::read(outside.join("11-12/keep")).unwrap(), b"untouched"); + assert!(store.language_packs_dir().join("nl/spelling.dic").is_file()); + std::fs::remove_file(store.language_packs_dir().join(".removing")).unwrap(); + std::fs::remove_dir_all(outside).unwrap(); + std::fs::remove_dir_all(store.dir()).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 4dabaad8..4e9506bb 100644 --- a/crates/font-store/src/store.rs +++ b/crates/font-store/src/store.rs @@ -718,6 +718,7 @@ impl FontStore { std::fs::create_dir_all(&self.dir).map_err(io("creating the font store folder"))?; let file = File::options().create(true).truncate(false).write(true).open(self.dir.join(LOCK)).map_err(io("locking the font store"))?; file.lock().map_err(io("locking the font store"))?; + self.sweep_committed_pack_removals(); Ok(file) } @@ -831,29 +832,54 @@ fn host(url: &str) -> &str { url.split("://").nth(1).and_then(|r| r.split('/').next()).unwrap_or(url) } -/// A folder of downloads under `.staging`, deleted when dropped. +/// A temporary folder, deleted when dropped unless kept for recovery. pub(crate) struct Stage { pub(crate) dir: PathBuf, + remove_on_drop: bool, } impl Stage { pub(crate) fn new(store: &Path) -> Result { + Self::in_dir(&store.join(STAGING)) + } + + pub(crate) fn in_dir(staging: &Path) -> Result { + Self::in_dir_excluding(staging, &[]) + } + + pub(crate) fn in_dir_excluding(staging: &Path, excluded: &[String]) -> Result { static N: AtomicU64 = AtomicU64::new(0); - let staging = store.join(STAGING); - std::fs::create_dir_all(&staging).map_err(io("creating a download folder"))?; + Self::create_in(staging, excluded, std::iter::repeat_with(|| format!("{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed)))) + } + + pub(crate) fn valid_name(name: &str) -> bool { + 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. + 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 - // (stale ones go in `sweep_staging`). + // (`sweep_staging` handles ordinary download staging, not recovery folders). let mut last = None; - for _ in 0..1000 { - let dir = staging.join(format!("{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed))); + for name in names.take(1000) { + if !Self::valid_name(&name) || excluded.contains(&name) { + continue; + } + let dir = staging.join(name); match std::fs::create_dir(&dir) { - Ok(()) => return Ok(Self { dir }), + Ok(()) => return Ok(Self { dir, remove_on_drop: true }), Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => last = Some(e), - Err(e) => return Err(io("creating a download folder")(e)), + Err(e) => return Err(io("creating a temporary folder")(e)), } } - Err(io("creating a download folder")(last.unwrap_or_else(|| std::io::Error::other("no free name")))) + Err(io("creating a temporary folder")(last.unwrap_or_else(|| std::io::Error::other("no free name")))) + } + + /// The caller places recovery data outside the ordinary download staging sweep. + pub(crate) fn keep_for_recovery(mut self) { + self.remove_on_drop = false; } /// Writes a verified download as `.part`, which no font scan picks up. @@ -871,7 +897,9 @@ impl Stage { impl Drop for Stage { fn drop(&mut self) { - let _ = std::fs::remove_dir_all(&self.dir); + if self.remove_on_drop { + let _ = std::fs::remove_dir_all(&self.dir); + } } }