From fa887ccd26a2d4e3ea5e9e7580c7735fc62e7fc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Mon, 17 Aug 2026 13:11:15 +0800 Subject: [PATCH] fix(skills): eliminate TOCTOU in no-replace rename fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/upgrade/skill_path_move.go | 2 + internal/upgrade/skill_publish_rename.go | 75 ++++++++++++++++--- internal/upgrade/skill_publish_rename_test.go | 70 ++++++++++++++++- 3 files changed, 134 insertions(+), 13 deletions(-) diff --git a/internal/upgrade/skill_path_move.go b/internal/upgrade/skill_path_move.go index e9f9539c..b1bb95f7 100644 --- a/internal/upgrade/skill_path_move.go +++ b/internal/upgrade/skill_path_move.go @@ -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 diff --git a/internal/upgrade/skill_publish_rename.go b/internal/upgrade/skill_publish_rename.go index 490f0f9a..ba6cb925 100644 --- a/internal/upgrade/skill_publish_rename.go +++ b/internal/upgrade/skill_publish_rename.go @@ -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) } diff --git a/internal/upgrade/skill_publish_rename_test.go b/internal/upgrade/skill_publish_rename_test.go index a420eed5..17acec77 100644 --- a/internal/upgrade/skill_publish_rename_test.go +++ b/internal/upgrade/skill_publish_rename_test.go @@ -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 {