Make the network nonce store's reader lock and corruption handling fail closed - #952
Conversation
Walkthrough
ChangesNonce store hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@keep-frost-net/src/nonce_store.rs`:
- Line 47: Update FileNonceStore::new to reject nonce-store paths whose
extension is .lock before constructing the lock path, preventing
path.with_extension("lock") from colliding with the data file. Add a regression
test covering a path ending in .lock and verify construction fails without
changing existing valid-path behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 964fadba-e3f0-48dc-86d4-54484e29aca9
📒 Files selected for processing (1)
keep-frost-net/src/nonce_store.rs
1 participant
Summary
Three problems in the same file, found while reviewing the equivalent fix in the CLI's store.
The reader locked the wrong file. Loading the consumed set took an exclusive lock on the store, while the append path and the rewrite path both lock a sibling. The reader and the writers therefore locked different inodes and never contended, so that lock excluded nothing: the set could be loaded while the file was being appended to or replaced underneath it, and the guard would begin life having missed entries. The existence check had the same shape of hole and is now inside the critical section, so a store created between the check and the load can no longer be skipped entirely.
A truncated entry was skipped with a warning. That is the fail-open direction for a replay guard, and the record it drops is the worst one to lose: a short entry is a partial append, so the missing record is the most recently consumed session, and skipping it returns that session id to the available set. The recovery path itself produced the reuse the store exists to prevent, and the only trace was a warning nobody reads after a crash. Loading now refuses.
Worth noting this was already inconsistent rather than merely permissive. A truncation leaving an odd number of hex characters fails the decode and errors hard a few lines above; only an even-length one was skipped. The same corruption had two opposite answers depending on where the write happened to stop, so this aligns them rather than introducing a new failure mode.
The unlock targeted a handle that was never locked. Moving the lock onto the sibling left the paired unlock on the store handle. On Unix that is a silent no-op, so nothing here observes it. On Windows
UnlockFileon an unheld region reports an error, which would have made every load of an existing store fail, on a target whose build job does not run for pull requests. The explicit unlock is gone in favor of the drop order the error paths already rely on.A store named like its own lock was erased by its own writes. The lock and temp siblings are derived by replacing the extension, so a store called
*.lockderives a lock path equal to itself, and both writer paths open the lock withtruncate(true). Recording into such a store zeroed it and left only the entry that had just arrived, so every previously consumed id was gone from disk and the guard came back empty on the next start. Confirmed by observation rather than inspection: with the check removed, recording two ids and reloading finds the first one missing. Such a path is now refused,.tmpincluded, since it collides the same way.Scope
The guard decision still reads an in-memory set built once at construction, so two processes sharing a file each hold their own stale view. Locking the load correctly makes the read atomic; it does not make the store multi-process safe, and this change should not be read as claiming that. Whether that is a supported deployment is a question about how this is run rather than about this code, and the answer changes what the fix should be, so it is filed rather than guessed at.
Also filed rather than fixed here: eviction above the entry ceiling silently returns old session ids to the available set, and the corruption refusal names no path and no remedy, which matters because the obvious operator response to a node that will not start is to delete the store, forfeiting all replay protection rather than the one lost entry.
Test plan
Four tests. A truncated entry refuses to load, with the refusal saying why. A session recorded before a truncated entry does not come back as available, which is the consequence that matters rather than the error itself. And loading blocks while another handle holds the writers' lock, then succeeds and returns the entry once it is released.
The lock test asserts the lock is held rather than that the lock file exists. An existence assertion passes even with the
lock_exclusivecall deleted outright, since the file is created by the open that precedes it; the contended version fails against that mutant.A fourth covers the colliding name, including a mixed-case spelling and a near-miss (
nonces.locked) that must still be accepted.Falsified: restoring the skip fails the truncation tests, and deleting
lock_exclusivefails the lock test, and removing the name check fails the collision test. The unlock fix has no test, deliberately and not for lack of trying: it is a no-op on Unix, so nothing running in this pipeline can observe it. All 459 library tests pass, workspace builds, formatter and clippy clean.