diff --git a/.github/workflows/font-store.yml b/.github/workflows/font-store.yml index 8b547ea8c..5a9d2d731 100644 --- a/.github/workflows/font-store.yml +++ b/.github/workflows/font-store.yml @@ -21,7 +21,7 @@ jobs: os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - uses: dtolnay/rust-toolchain@b725eda525362e7cf34af9a7243cac1e3f01a7ec # stable with: components: rustfmt, clippy diff --git a/crates/font-store/README.md b/crates/font-store/README.md index fcbade7fd..520b0e0b7 100644 --- a/crates/font-store/README.md +++ b/crates/font-store/README.md @@ -138,7 +138,7 @@ 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`. | -**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. +**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 c1b4e8dfa..282d5909f 100644 --- a/crates/font-store/src/language_packs.rs +++ b/crates/font-store/src/language_packs.rs @@ -418,7 +418,10 @@ impl FontStore { /// The installed Language packs whose folder is there, by locale. pub fn installed_language_packs(&self) -> Vec { let dir = self.language_packs_dir(); - let installed = self.read_installed().unwrap_or_else(|_| self.installed_from_disk()); + // Read only: without the store lock this never writes the record (an install or removal + // may be under way); a record that can't be read is listed from the folders and repaired + // by the next install or removal, which hold the lock. + let installed = self.read_record().unwrap_or_else(|| self.installed_from_disk()); installed .packs .into_iter() @@ -462,7 +465,7 @@ impl FontStore { fn read_installed(&self) -> Result { let path = self.dir().join(INSTALLED); let there = std::fs::symlink_metadata(&path).is_ok(); - if there && let Some(i) = read_limited(&path, MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice::(&b).ok()) { + if let Some(i) = self.read_record() { return Ok(i); } let rebuilt = self.installed_from_disk(); @@ -477,6 +480,11 @@ impl FontStore { Ok(rebuilt) } + /// 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()) + } + /// The installed packs as their folders say: each folder named by a locale, with the name /// and licence from its `pack.toml` (release, version and sha256 unknown). fn installed_from_disk(&self) -> Installed { diff --git a/crates/font-store/src/language_packs/tests.rs b/crates/font-store/src/language_packs/tests.rs index 5b7d13edb..813ee1c14 100644 --- a/crates/font-store/src/language_packs/tests.rs +++ b/crates/font-store/src/language_packs/tests.rs @@ -387,6 +387,10 @@ fn a_corrupt_record_is_rebuilt_from_the_folders() { // Still listed, with the name from its pack.toml. let listed = store.installed_language_packs(); assert_eq!(listed.iter().map(|p| (p.locale.as_str(), p.name.as_str())).collect::>(), [("nl", "Test")]); + // craft-libs#486 review: listing holds no store lock, so it never writes the record (it + // could race an install); the next install or removal repairs it. + assert_eq!(std::fs::read(&record).unwrap(), b"{ not json"); + assert!(!store.dir().join("language-packs.json.bad").exists()); // Installing another keeps it. std::fs::write(&record, b"{ not json").unwrap(); let de = pack_archive("de", "zzde"); @@ -401,8 +405,8 @@ fn a_corrupt_record_is_rebuilt_from_the_folders() { // A missing record with pack folders is rebuilt too. std::fs::remove_file(&record).unwrap(); assert_eq!(store.installed_language_packs().len(), 2); + assert!(!record.exists(), "listing writes nothing"); let nl_again = PackEntry { url: format!("{RELEASE}nl.tar.zst"), ..nl.clone() }; - std::fs::remove_file(&record).unwrap(); store.install_pack_from(false, &index, &nl_again, &Cancel::new(), &mut |_| {}).unwrap(); assert_eq!(store.installed_language_packs().len(), 2, "de kept when the record was missing"); // Removing works on a rebuilt record too.