Lock a file the nonce store's own rename cannot unlink - #951
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The deterministic assertion is a better test than the one I wrote, and it covers the half mine could not. The concurrency test only ever exercised the stale read. The invariant underneath is simpler and directly observable: a claim replaces the store, so the store's inode changes, and it must not touch the lock's. Locking the store made those the same inode, which is the whole bug stated in one line. That now fails immediately on the old shape, needs no threads, and takes microseconds. It also fails if someone later deletes the lock file between calls as a cleanup, which is the realistic way this regresses. The swallowed error is a fair hit. Mapping every Err to "not granted" meant an implementation that failed under contention for seven of eight callers still showed one winner and went green, which is the shape of test that reports success for the wrong reason. Every thread's result is now required to be Ok before counting winners. Also removed the no-op filter, and the test's own doc now says it discriminates on the read rather than implying it proves both halves. Not taken here: guarding against the lock file being unlinked while held. The recovery is a stat-and-retry loop, and it is defending against something outside the process rather than the bug this change is about. The call-site comment says the file must never be replaced, and the new inode assertion now enforces that for anything the code itself does, which covers the realistic regression. Filed for the external case. The findings in the sibling network store are the more interesting result and are filed rather than fixed here: its reader locks the store file while its writers lock a sibling, so those two never contend and the reader's lock excludes nothing; a malformed trailing entry is skipped with a warning, which drops the most recently consumed session and is the fail-open direction for a replay guard; and the guard decision reads an in-memory set populated once at construction, so it never consults the file the lock protects. The comment in this change cites that store as having got the locking right, which is true of its writers and not of its reader; worth correcting when those are addressed. |
1 participant
Summary
The signer's nonce store took its exclusive lock on the store file, and the write path renamed a new file over that same path. Renaming unlinks the inode the lock is held on, so the lock stopped protecting the thing it was taken for.
Two failures follow, and they are not the same failure.
A caller that opened the store before another replaced it waits on the old inode, is handed the lock once the holder finishes, and then reads that unlinked file: the state from before the claim it just waited for. It sees no claim and hands out the same round-1 commitment. Two signature shares over one nonce under different challenges recover the signer's key share, which is the outcome this store exists to prevent.
Separately, once the store has been replaced, one caller can hold a lock on the unlinked inode while another holds a lock on the file that replaced it. Both are inside the critical section at the same time and exclusion has quietly stopped meaning anything, whatever either of them reads.
The lock now lives on a sibling that is never renamed, which is what the network crate's equivalent store already does, and the state is read inside the critical section rather than through the locked handle. The first change makes exclusion real; the second makes what is read current.
Pre-existing rather than introduced recently, but it arrived with the change that made writes atomic: replacing an in-place write with write-temp-then-rename is exactly what turned the locked file into a file that gets unlinked.
Test plan
Eight handles race to claim one commitment, repeated over rounds, asserting exactly one is granted. On the fixed code exactly one winner is guaranteed, so this cannot fail spuriously; on the broken code it needs the threads to overlap, and in practice it fails on the first round.
Falsified, and the first attempt at falsifying was wrong in a way worth recording. Reverting only the lock left the test green, because reading by path still resolves to the current file: the fix has two halves and I had reverted one. Reverting both fails immediately. Testing the halves separately then showed the read is what this test discriminates on, and the exclusion half is not covered by any test here, since reproducing it needs a three-way interleaving. The comment at the call site says which case is covered and which is reasoned, so the next reader does not assume the test proves both.
The existing sequential test is kept. It claims and then claims again, so the second call opens the store after the first replaced it and sees the claim naturally; it cannot reach this interleaving, and it reads as though it can.
All nonce-store tests pass, workspace builds, formatter and clippy clean.