fix: use atomic rename that refuses existing destinations#93
Conversation
Replace Stat-then-Rename with RENAME_NOREPLACE / RENAME_EXCL so concurrent creates cannot be silently overwritten during path renames. Fixes dolph#4
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4105a0167d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if !errors.Is(err, os.ErrNotExist) { | ||
| return fmt.Errorf("stat rename destination %v: %w", newpath, err) | ||
| } | ||
| return os.Rename(oldpath, newpath) |
There was a problem hiding this comment.
On Windows this still has the same TOCTOU clobber window: os.Rename is implemented with replace-existing semantics, so any file that appears at newpath after the os.Stat check is overwritten. Because WalkDir renames siblings concurrently, two sibling names that collapse to the same target, e.g. with find="ab", replace="a", can both pass the pre-check and then one rename replaces the other instead of returning the refusal error this change is meant to guarantee.
Useful? React with 👍 / 👎.
| } else if !errors.Is(err, os.ErrNotExist) { | ||
| return fmt.Errorf("stat rename destination %v: %w", newpath, err) | ||
| } | ||
| return os.Rename(oldpath, newpath) |
There was a problem hiding this comment.
For the !linux && !darwin && !windows build, this helper is still a stat-then-os.Rename, and on Unix targets such as FreeBSD/Solaris rename replaces an existing non-directory destination. If another concurrent rename creates newpath after the check, including sibling files processed by this tool whose names map to the same replacement target, this path can silently overwrite that file rather than preserving the no-replace guarantee.
Useful? React with 👍 / 👎.
1 participant
Fixes #4
Made with Cursor