CP4.1: craft-libs#486 review — listing Language packs never writes the record without the store lock (an install or removal repairs it); the font-store workflow pins actions/checkout to v7.0.1's commit

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FypgCX8LsYYE8fsj4byBm2

.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

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()` | `<store>/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/<name>` (`[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/<name>` (`[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=<url or path>` 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<String>` returns the value so apps can show that the pin is bypassed. Never set it for users.
 

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<InstalledLanguagePack> {
         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<Installed, Error> {
         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::<Installed>(&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<Installed> {
+        read_limited(&self.dir().join(INSTALLED), MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice::<Installed>(&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 {

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::<Vec<_>>(), [("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.