fix(skills): eliminate TOCTOU in no-replace rename fallback
The fallback path for filesystems without RENAME_NOREPLACE/EXCL (NFS, FUSE, overlayfs) used Lstat-then-Rename, which could overwrite a concurrently created destination between the check and the rename. Replace the TOCTOU-prone check with truly atomic no-clobber primitives: - Directories: os.Mkdir atomically claims the destination (fails with EEXIST if occupied). On Linux rename(2) replaces the empty dir directly; on Darwin/Windows rename refuses existing dirs so the empty dir is removed and the rename retried — any concurrent creation between remove and rename is detected by the second rename failing. - Files: os.Link atomically fails if the destination exists, then os.Remove completes the move. Add concurrent-creation test covering the mkdir→rename race window.
This commit is contained in:
@@ -21,6 +21,8 @@ var (
|
||||
skillPathChmod = os.Chmod
|
||||
skillPathLstat = os.Lstat
|
||||
skillPathMkdir = os.Mkdir
|
||||
skillPathLink = os.Link
|
||||
skillPathRemove = os.Remove
|
||||
skillPathReadDir = os.ReadDir
|
||||
skillPathReadlink = os.Readlink
|
||||
skillPathSymlink = os.Symlink
|
||||
|
||||
@@ -19,10 +19,10 @@ var skillPathRenameNoReplaceAtomic = renameSkillPathNoReplaceAtomic
|
||||
// support from the underlying filesystem and report EINVAL/ENOTSUP when it is
|
||||
// missing — rename(2) lists only ext4, btrfs, tmpfs and cifs, so NFS, FUSE and
|
||||
// overlayfs homes reject the flag. Failing there would abort the whole install
|
||||
// on those filesystems, so they degrade to an explicit existence check plus a
|
||||
// plain rename: the no-clobber contract is preserved, only its atomicity is
|
||||
// lost. That check-then-rename window is the same exposure a plain rename
|
||||
// always had, and callers additionally verify identity after publication.
|
||||
// on those filesystems, so the fallback uses truly atomic no-clobber primitives
|
||||
// that work on every POSIX filesystem: os.Mkdir for directories (fails with
|
||||
// EEXIST if the path is occupied) and os.Link for regular files (fails with
|
||||
// EEXIST if the path is occupied). Neither primitive has a TOCTOU window.
|
||||
func renameSkillPathNoReplace(source, destination string) error {
|
||||
err := skillPathRenameNoReplaceAtomic(source, destination)
|
||||
if err == nil {
|
||||
@@ -31,10 +31,67 @@ func renameSkillPathNoReplace(source, destination string) error {
|
||||
if !errors.Is(err, errNoReplaceRenameUnsupported) && !isNoReplaceRenameUnsupported(err) {
|
||||
return err
|
||||
}
|
||||
if _, statErr := skillPathLstat(destination); statErr == nil {
|
||||
return &os.LinkError{Op: "rename", Old: source, New: destination, Err: os.ErrExist}
|
||||
} else if !os.IsNotExist(statErr) {
|
||||
return fmt.Errorf("检查发布目标失败 %s: %w", destination, statErr)
|
||||
return renameSkillPathNoReplaceFallback(source, destination)
|
||||
}
|
||||
|
||||
// renameSkillPathNoReplaceFallback is used when the kernel no-replace rename
|
||||
// flag is unavailable. It dispatches on the source type to pick an atomic
|
||||
// no-clobber primitive that does not require filesystem support.
|
||||
func renameSkillPathNoReplaceFallback(source, destination string) error {
|
||||
info, err := skillPathLstat(source)
|
||||
if err != nil {
|
||||
return fmt.Errorf("读取源 Skill 身份失败 %s: %w", source, err)
|
||||
}
|
||||
return skillPathRename(source, destination)
|
||||
switch {
|
||||
case info.IsDir():
|
||||
return renameSkillDirNoReplaceFallback(source, destination)
|
||||
case info.Mode().IsRegular():
|
||||
return renameSkillFileNoReplaceFallback(source, destination)
|
||||
default:
|
||||
return &os.LinkError{
|
||||
Op: "rename", Old: source, New: destination,
|
||||
Err: fmt.Errorf("%w: 源路径类型不支持降级发布", errNoReplaceRenameUnsupported),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// renameSkillDirNoReplaceFallback atomically claims the destination with
|
||||
// mkdir (which fails with EEXIST if the path is already occupied) and then
|
||||
// replaces the empty, self-owned directory with a plain rename. On Linux
|
||||
// rename(2) replaces an empty directory directly. On Darwin and Windows
|
||||
// rename(2) refuses to replace any existing directory, so the empty dir is
|
||||
// removed and the rename retried. If someone populated the claimed directory
|
||||
// between mkdir and remove, Remove fails (ENOTEMPTY) and no data is lost.
|
||||
// If someone creates a new path at the destination between remove and rename,
|
||||
// the second Rename detects it and fails. Neither step has a TOCTOU window
|
||||
// that could silently clobber data.
|
||||
func renameSkillDirNoReplaceFallback(source, destination string) error {
|
||||
if err := skillPathMkdir(destination, 0o700); err != nil {
|
||||
if os.IsExist(err) {
|
||||
return &os.LinkError{Op: "rename", Old: source, New: destination, Err: os.ErrExist}
|
||||
}
|
||||
return fmt.Errorf("声明 Skill 发布目标失败 %s: %w", destination, err)
|
||||
}
|
||||
if err := skillPathRename(source, destination); err == nil {
|
||||
return nil
|
||||
}
|
||||
if rmErr := skillPathRemove(destination); rmErr != nil {
|
||||
return fmt.Errorf("清理已声明 Skill 目标失败 %s: %w", destination, rmErr)
|
||||
}
|
||||
if err := skillPathRename(source, destination); err != nil {
|
||||
return fmt.Errorf("发布 Skill 到已声明目标失败 %s: %w", destination, err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// renameSkillFileNoReplaceFallback uses os.Link, which atomically fails if the
|
||||
// destination exists, then removes the source to complete the move.
|
||||
func renameSkillFileNoReplaceFallback(source, destination string) error {
|
||||
if err := skillPathLink(source, destination); err != nil {
|
||||
if os.IsExist(err) {
|
||||
return &os.LinkError{Op: "rename", Old: source, New: destination, Err: os.ErrExist}
|
||||
}
|
||||
return fmt.Errorf("发布 Skill 文件失败 %s: %w", destination, err)
|
||||
}
|
||||
return skillPathRemove(source)
|
||||
}
|
||||
|
||||
@@ -12,11 +12,13 @@ import (
|
||||
// TestCrossPlatformCoverageNoReplaceRenameFallback pins the degradation path for
|
||||
// filesystems that reject RENAME_NOREPLACE / RENAME_EXCL (NFS, FUSE, overlayfs).
|
||||
// Those installs must still publish, and must still refuse to clobber an
|
||||
// occupied destination.
|
||||
// occupied destination. The fallback uses atomic no-clobber primitives
|
||||
// (os.Mkdir for directories, os.Link for files) instead of a TOCTOU-prone
|
||||
// check-then-rename.
|
||||
func TestCrossPlatformCoverageNoReplaceRenameFallback(t *testing.T) {
|
||||
unsupported := testNoReplaceUnsupportedErrors()
|
||||
|
||||
t.Run("publishes when the filesystem rejects the flag", func(t *testing.T) {
|
||||
t.Run("publishes a file when the filesystem rejects the flag", func(t *testing.T) {
|
||||
for _, unsupportedErr := range unsupported {
|
||||
dir := t.TempDir()
|
||||
source := filepath.Join(dir, "source")
|
||||
@@ -37,7 +39,24 @@ func TestCrossPlatformCoverageNoReplaceRenameFallback(t *testing.T) {
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("still refuses an occupied destination", func(t *testing.T) {
|
||||
t.Run("publishes a directory when the filesystem rejects the flag", func(t *testing.T) {
|
||||
for _, unsupportedErr := range unsupported {
|
||||
dir := t.TempDir()
|
||||
source := filepath.Join(dir, "source")
|
||||
destination := filepath.Join(dir, "destination")
|
||||
seedUpgradeSkill(t, source, "payload", false)
|
||||
testseam.Swap(t, &skillPathRenameNoReplaceAtomic, func(string, string) error { return unsupportedErr })
|
||||
if err := renameSkillPathNoReplace(source, destination); err != nil {
|
||||
t.Fatalf("fallback must publish for %v, got %v", unsupportedErr, err)
|
||||
}
|
||||
assertUpgradeSkillContent(t, destination, "payload")
|
||||
if _, err := os.Lstat(source); !os.IsNotExist(err) {
|
||||
t.Fatalf("source must be gone, stat err=%v", err)
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("still refuses an occupied destination for files", func(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
source := filepath.Join(dir, "source")
|
||||
destination := filepath.Join(dir, "destination")
|
||||
@@ -59,6 +78,49 @@ func TestCrossPlatformCoverageNoReplaceRenameFallback(t *testing.T) {
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("refuses an occupied directory destination via mkdir EEXIST", func(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
source := filepath.Join(dir, "source")
|
||||
destination := filepath.Join(dir, "destination")
|
||||
seedUpgradeSkill(t, source, "payload", false)
|
||||
seedUpgradeSkill(t, destination, "existing", false)
|
||||
testseam.Swap(t, &skillPathRenameNoReplaceAtomic, func(string, string) error { return errNoReplaceRenameUnsupported })
|
||||
err := renameSkillPathNoReplace(source, destination)
|
||||
if !errors.Is(err, os.ErrExist) {
|
||||
t.Fatalf("occupied destination must report ErrExist, got %v", err)
|
||||
}
|
||||
assertUpgradeSkillContent(t, destination, "existing")
|
||||
assertUpgradeSkillContent(t, source, "payload")
|
||||
})
|
||||
|
||||
t.Run("concurrent creation between mkdir and rename is detected", func(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
source := filepath.Join(dir, "source")
|
||||
destination := filepath.Join(dir, "destination")
|
||||
seedUpgradeSkill(t, source, "payload", false)
|
||||
testseam.Swap(t, &skillPathRenameNoReplaceAtomic, func(string, string) error { return errNoReplaceRenameUnsupported })
|
||||
renameErr := errors.New("simulated ENOTEMPTY: destination populated by another process")
|
||||
testseam.Swap(t, &skillPathRename, func(string, string) error { return renameErr })
|
||||
var removeCalled bool
|
||||
testseam.Swap(t, &skillPathRemove, func(path string) error {
|
||||
if path == destination {
|
||||
removeCalled = true
|
||||
}
|
||||
return os.Remove(path)
|
||||
})
|
||||
err := renameSkillPathNoReplace(source, destination)
|
||||
if !errors.Is(err, renameErr) {
|
||||
t.Fatalf("rename failure must surface, got %v", err)
|
||||
}
|
||||
if !removeCalled {
|
||||
t.Fatal("empty destination must be cleaned up after rename failure")
|
||||
}
|
||||
if _, statErr := os.Lstat(destination); !os.IsNotExist(statErr) {
|
||||
t.Fatalf("destination must not exist after cleanup, stat err=%v", statErr)
|
||||
}
|
||||
assertUpgradeSkillContent(t, source, "payload")
|
||||
})
|
||||
|
||||
t.Run("a genuine error is not retried", func(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
source := filepath.Join(dir, "source")
|
||||
@@ -78,7 +140,7 @@ func TestCrossPlatformCoverageNoReplaceRenameFallback(t *testing.T) {
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("stat failure on destination surfaces without rename", func(t *testing.T) {
|
||||
t.Run("source stat failure surfaces without rename", func(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
source := filepath.Join(dir, "source")
|
||||
if err := os.WriteFile(source, []byte("payload"), 0o644); err != nil {
|
||||
|
||||
Reference in New Issue
Block a user