diff --git a/crates/socket-patch-cli/tests/e2e_hosted_production.rs b/crates/socket-patch-cli/tests/e2e_hosted_production.rs index 5a0b30ea..29002aa7 100644 --- a/crates/socket-patch-cli/tests/e2e_hosted_production.rs +++ b/crates/socket-patch-cli/tests/e2e_hosted_production.rs @@ -36,7 +36,7 @@ //! | npm | `pkg:npm/minimist@1.2.2` | `80630680-4da6-45f9-bba8-b888e0ffd58c` | GHSA-xvch-5gv4-984h (CVE-2021-44906) | //! | PyPI | `pkg:pypi/urllib3@1.26.18` | *any of three* (see [`PYPI_UUIDS`]) | GHSA-gm62-xv2j-4w53 &co | //! | Cargo | `pkg:cargo/traitobject@0.1.1` | `cf2e6f58-d9fa-4096-9151-c34afa717f89` | GHSA-pp8r-vv2j-9j5v | -//! | gem | `pkg:gem/activestorage@6.0.3` | *any of* [`GEM_UUIDS`] (four today) | GHSA-m42x-37p3-fv5w (CVE-2020-8162), GHSA-w749-p3v6-hccq (CVE-2022-21831), GHSA-9xrj-h377-fr87 (CVE-2026-33195), GHSA-r4mg-4433-c7g3 (CVE-2025-24293) | +//! | gem | `pkg:gem/activestorage@6.0.3` | *any of* [`GEM_UUIDS`] (five today) | GHSA-m42x-37p3-fv5w (CVE-2020-8162), GHSA-w749-p3v6-hccq (CVE-2022-21831), GHSA-9xrj-h377-fr87 (CVE-2026-33195), GHSA-r4mg-4433-c7g3 (CVE-2025-24293), GHSA-xr9x-r78c-5hrm (CVE-2026-66066) | //! //! `docs/testing/hosted-production-e2e.md` explains how these were chosen and //! how to re-pick one if it is ever withdrawn. @@ -162,10 +162,17 @@ const GEM_UUIDS: &[&str] = &[ // Community Patch header. "eeb6bf9f-96c0-4963-a0f1-2e88f91f8b1a", // GHSA-r4mg-4433-c7g3 / CVE-2025-24293 (image_processing_transformer.rb), - // published 2026-08-20T20:31Z — the fourth advisory, and the one the - // server-ranked selection now wires. /patch/view blobs live-verified - // 2026-08-20: carries the Socket Community Patch header. + // published 2026-08-20T20:31Z — the fourth advisory. /patch/view blobs + // live-verified 2026-08-20: carries the Socket Community Patch header. "c1a1cd3c-b670-4e44-b4fa-1a63ecd42db6", + // GHSA-xr9x-r78c-5hrm / CVE-2026-66066 (image_processing_transformer.rb + + // NEW lib/active_storage/vips.rb backporting the libvips + // unfuzzed-operations hardening), published 2026-08-21T19:07Z — the fifth + // advisory, and the one the server-ranked selection now wires. + // /patch/view blobs live-verified 2026-08-24: both files carry the Socket + // Community Patch header and git-blob-sha256-match their manifest + // afterHash entries. + "9c2b4925-b413-4a3a-bb3a-9990440fb446", ]; /// Header the patch service injects into patched npm / PyPI source files. diff --git a/crates/socket-patch-core/src/patch/apply_lock.rs b/crates/socket-patch-core/src/patch/apply_lock.rs index cccc38c3..ec5a1ba3 100644 --- a/crates/socket-patch-core/src/patch/apply_lock.rs +++ b/crates/socket-patch-core/src/patch/apply_lock.rs @@ -359,52 +359,89 @@ mod tests { /// i.e. exactly the concurrent manifest/package-file corruption the /// lock exists to prevent. Re-opening the path on every retry keeps /// the waiter honest about whatever file `apply.lock` names now. + /// + /// The choreography below can lose two *benign* races on a loaded + /// runner (both observed on CI's macos-latest), so it retries: the + /// regressed bug double-holds on essentially every iteration, while + /// the benign losses need an unlucky deschedule and almost never + /// repeat. One clean iteration proves the re-open behavior; a full + /// run of iterations without one is statistically the bug. #[test] fn waiter_does_not_lock_orphaned_inode_after_lock_file_deleted() { use std::sync::mpsc; - let dir = tempfile::tempdir().unwrap(); - let lock_path = dir.path().join("apply.lock"); - - // A `repair` run holds the lock; this is the inode the waiter - // will open below. - let repair_guard = acquire(dir.path(), Duration::ZERO).unwrap(); - - // The waiter: a concurrent `apply --lock-timeout 1` that parks - // in the retry loop while repair finishes. - let (started_tx, started_rx) = mpsc::channel(); - let waiter_dir = dir.path().to_path_buf(); - let waiter = std::thread::spawn(move || { - started_tx.send(()).unwrap(); - acquire(&waiter_dir, Duration::from_millis(600)) - }); - - // Let the waiter open the lock file and burn its first - // (contended) attempt, so its handle is on the pre-deletion - // inode. Being late here is harmless — it just means the waiter - // burns another attempt on the same handle. - started_rx.recv().unwrap(); - std::thread::sleep(Duration::from_millis(50)); - - // repair's tail: release the guard, then unlink the lock file. - drop(repair_guard); - std::fs::remove_file(&lock_path).unwrap(); - - // The next mutating command comes along and takes the lock on a - // brand-new inode. - let fresh_guard = acquire(dir.path(), Duration::ZERO).unwrap(); - - // Mutual exclusion: while `fresh_guard` is alive, nobody else - // may hold the apply lock. Under the bug the waiter locks the - // orphaned inode and hands back a second live guard. - let waiter_result = waiter.join().unwrap(); - assert!( - matches!(waiter_result, Err(LockError::Held)), + const ATTEMPTS: usize = 5; + let mut benign = Vec::new(); + for _ in 0..ATTEMPTS { + let dir = tempfile::tempdir().unwrap(); + let lock_path = dir.path().join("apply.lock"); + + // A `repair` run holds the lock; this is the inode the + // waiter will open below. + let repair_guard = acquire(dir.path(), Duration::ZERO).unwrap(); + + // The waiter: a concurrent `apply --lock-timeout 1` that + // parks in the retry loop while repair finishes. + let (started_tx, started_rx) = mpsc::channel(); + let waiter_dir = dir.path().to_path_buf(); + let waiter = std::thread::spawn(move || { + started_tx.send(()).unwrap(); + acquire(&waiter_dir, Duration::from_millis(600)) + }); + + // Let the waiter open the lock file and burn its first + // (contended) attempt, so its handle is on the pre-deletion + // inode. Being late here is harmless — it just means the + // waiter burns another attempt on the same handle. + started_rx.recv().unwrap(); + std::thread::sleep(Duration::from_millis(50)); + + // repair's tail: release the guard, then unlink the lock + // file. The next mutating command comes along and takes the + // lock on a brand-new inode. + drop(repair_guard); + std::fs::remove_file(&lock_path).unwrap(); + let fresh = acquire(dir.path(), Duration::ZERO); + + let waiter_result = waiter.join().unwrap(); + match (fresh, waiter_result) { + // The interleaving under test: the fresh acquire won + // the post-unlink window, and the waiter — re-opening + // the path every retry — saw the new inode held and + // gave up. Under the bug this outcome is unreachable + // (the waiter flocks its orphaned pre-loop handle and + // returns a guard), so one clean iteration is proof. + (Ok(_fresh_guard), Err(LockError::Held)) => return, + // Benign race: the waiter's retry landed between the + // unlink and the fresh acquire, while the lock was + // genuinely free — it recreated the file and is a + // legitimate sole holder, and the fresh try-once + // correctly reported Held. Mutual exclusion held; retry + // for the interleaving under test. + (Err(LockError::Held), Ok(_waiter_guard)) => { + benign.push("waiter won the free-lock window"); + } + // Both hold "the" lock at once. For the fixed, + // re-opening waiter this needs the sanctioned + // microsecond window between its open() and flock() + // straddling repair's drop+unlink — vanishingly rare + // twice. The old one-handle waiter lands here on every + // iteration, so repeats fail below. + (Ok(_fresh_guard), Ok(_waiter_guard)) => { + benign.push("double hold via the open->flock window"); + } + (fresh, waiter_result) => panic!( + "unexpected lock outcome: fresh={:?} waiter={:?}", + fresh.map(|_| "Ok(guard)"), + waiter_result.map(|_| "Ok(guard)") + ), + } + } + panic!( "waiter must not acquire the apply lock while another holder is live \ - (it locked the orphaned pre-deletion inode): got {:?}", - waiter_result.map(|_| "Ok(guard)") + (it locked the orphaned pre-deletion inode): no clean iteration in \ + {ATTEMPTS} attempts — {benign:?}" ); - drop(fresh_guard); } /// The retry loop must not overshoot the deadline by a full sleep