fix(store): eliminate TOCTOU race in write_atomic#16
Merged
mark-pro merged 3 commits intooneirosoft:mainfrom Mar 31, 2026
Merged
fix(store): eliminate TOCTOU race in write_atomic#16mark-pro merged 3 commits intooneirosoft:mainfrom
mark-pro merged 3 commits intooneirosoft:mainfrom
Conversation
On Unix, fs::rename atomically overwrites the target, so the prior remove_file call was unnecessary and introduced a window where a crash between remove and rename would lose the state file entirely. Split the implementation with cfg(unix) / cfg(windows): - Unix: single rename call (fully atomic) - Windows: remove then rename with temp-file cleanup on error Also adds unit tests for write_atomic and tempfile dev-dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the store layer’s atomic write helper to avoid a Unix TOCTOU window by relying on rename(2) overwrite semantics, and introduces unit tests around write_atomic.
Changes:
- Switch Unix implementation to a single
fs::rename(atomic overwrite) instead ofremove_file+rename. - Add a Windows-specific path that removes the destination before renaming and cleans up the temp file on rename error.
- Add unit tests for
write_atomicand addtempfileas a dev-dependency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/core/store/fs.rs |
Adjusts write_atomic platform behavior and adds unit tests covering create/overwrite/parent-dir creation. |
Cargo.toml |
Adds tempfile under [dev-dependencies] for the new tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add fallback block for non-Unix/non-Windows targets - Clean up temp file on rename error in Unix path - Handle remove_file errors explicitly in Windows path (allow NotFound) - Clarify comment wording about atomicity guarantees Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mark-pro
approved these changes
Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
write_atomicwhereremove_filefollowed byrenamecould lose the state file on a crash between the two calls.rename(2)atomically overwrites the target, so theremove_filecall was unnecessary and harmful. Now uses a singlefs::renamecall.renamecannot overwrite, so usesremove_file+renamewith temp-file cleanup on error.write_atomicandtempfiledev-dependency.Addresses item 2 in #10.
Test plan
write_atomic_creates_new_file— writing to a non-existent path succeedswrite_atomic_overwrites_existing_file— overwriting produces correct contentwrite_atomic_creates_parent_dirs— nested parent directories are created🤖 Generated with Claude Code