diff --git a/internal/cleanup/cleanup_test.go b/internal/cleanup/cleanup_test.go index 5effada..5e0e27e 100644 --- a/internal/cleanup/cleanup_test.go +++ b/internal/cleanup/cleanup_test.go @@ -780,3 +780,29 @@ func TestWhyOmitsMainWorktree(t *testing.T) { } } } + +// A trash name taken between the free-name check and the rename is retried +// with the next name instead of failing the removal. +func TestTrashNameTakenRetries(t *testing.T) { + e, _, _, _ := fixture(t) + path := scratch(t, e, "taken") + c := listed(t, e, path) + var tried []string + e.Rename = func(from, to string) error { + tried = append(tried, to) + if len(tried) == 1 { + return syscall.EEXIST + } + return os.Rename(from, to) + } + r, err := Apply(context.Background(), e, c, ApplyOpts{}) + if err != nil { + t.Fatal(err) + } + if len(tried) != 2 || tried[0] == tried[1] || r.TrashPath != tried[1] { + t.Fatal(tried, r.TrashPath) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatal("scratch still in place", err) + } +} diff --git a/internal/cleanup/trash.go b/internal/cleanup/trash.go index dde446a..1070035 100644 --- a/internal/cleanup/trash.go +++ b/internal/cleanup/trash.go @@ -4,10 +4,12 @@ package cleanup import ( "context" + "errors" "fmt" "github.com/labstack/driga/internal/secfile" "os" "path/filepath" + "syscall" ) func trash(ctx context.Context, e Env, c Candidate, opts ApplyOpts, r Result) (Result, error) { @@ -35,27 +37,35 @@ func trash(ctx context.Context, e Env, c Candidate, opts ApplyOpts, r Result) (R return r, err } name := filepath.Base(c.Path) - for i := 0; ; i++ { - candidate := name - if i > 0 { - candidate = fmt.Sprintf("%s.%d.%d", name, e.now().UnixNano(), i) - } - r.TrashPath = filepath.Join(files, candidate) - _, err := os.Lstat(r.TrashPath) - if os.IsNotExist(err) { - if infoDir != "" { - if _, err := os.Lstat(filepath.Join(infoDir, candidate+".trashinfo")); err == nil { - continue - } else if !os.IsNotExist(err) { - return r, err + // pick sets r.TrashPath to the first free trash name at index i or later + // and returns its index. + pick := func(i int) (int, error) { + for ; ; i++ { + candidate := name + if i > 0 { + candidate = fmt.Sprintf("%s.%d.%d", name, e.now().UnixNano(), i) + } + r.TrashPath = filepath.Join(files, candidate) + _, err := os.Lstat(r.TrashPath) + if os.IsNotExist(err) { + if infoDir != "" { + if _, err := os.Lstat(filepath.Join(infoDir, candidate+".trashinfo")); err == nil { + continue + } else if !os.IsNotExist(err) { + return i, err + } } + return i, nil + } + if err != nil { + return i, err } - break - } - if err != nil { - return r, err } } + index, err := pick(0) + if err != nil { + return r, err + } r.Commands = [][]string{{"rename", c.Path, r.TrashPath}} if opts.DryRun { return r, nil @@ -81,22 +91,35 @@ func trash(ctx context.Context, e Env, c Candidate, opts ApplyOpts, r Result) (R if infoDir != "" { canonicalInfo = filepath.Join(canonicalHome, mustRelative(e.Home, infoDir)) } - finish, err := trashInfo(canonicalInfo, filepath.Base(r.TrashPath), c.Path, e.now()) - if err != nil { - return r, err - } - var moveErr error - if e.Rename != nil { - moveErr = e.Rename(c.Path, r.TrashPath) - } else { - moveErr = renameScratch(c, filepath.Join(canonicalFiles, filepath.Base(r.TrashPath))) - } - if err := moveErr; err != nil { + // The rename never replaces an item; a name taken since pick is retried + // with the next one. + for attempt := 0; ; attempt++ { + finish, err := trashInfo(canonicalInfo, filepath.Base(r.TrashPath), c.Path, e.now()) + if err != nil { + return r, err + } + var moveErr error + if e.Rename != nil { + moveErr = e.Rename(c.Path, r.TrashPath) + } else { + moveErr = renameScratch(c, filepath.Join(canonicalFiles, filepath.Base(r.TrashPath))) + } + if moveErr == nil { + finish(true) + return r, nil + } finish(false) - return r, fmt.Errorf("trash rename (same filesystem required): %w", err) + if errors.Is(moveErr, syscall.EEXIST) && attempt < 8 { + if index, err = pick(index + 1); err != nil { + return r, err + } + continue + } + if errors.Is(moveErr, syscall.EXDEV) { + return r, fmt.Errorf("trash rename (same filesystem required): %w", moveErr) + } + return r, fmt.Errorf("trash rename: %w", moveErr) } - finish(true) - return r, nil } // Both arguments were already validated as known HOME-relative OS paths. diff --git a/internal/cleanup/trash_darwin_test.go b/internal/cleanup/trash_darwin_test.go new file mode 100644 index 0000000..2adaf1c --- /dev/null +++ b/internal/cleanup/trash_darwin_test.go @@ -0,0 +1,67 @@ +package cleanup + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +// macOS refuses to open ~/.Trash to a process without Full Disk Access, as +// the Driga service is, but lets it rename into it. A trash directory that +// cannot be read stands in for that here: scratch still moves into it, and an +// item already there keeps its name. +func TestScratchTrashUnreadable(t *testing.T) { + e, _, _, _ := fixture(t) + trash := filepath.Join(e.Home, ".Trash") + put(t, filepath.Join(trash, "unreadable", "kept"), "keep") + if err := os.Chmod(trash, 0o300); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(trash, 0o700) }) + if _, err := os.ReadDir(trash); err == nil { + t.Skip("trash still readable; running as root?") + } + path := scratch(t, e, "unreadable") + r, err := Apply(context.Background(), e, listed(t, e, path), ApplyOpts{}) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatal("scratch still in place", err) + } + if filepath.Dir(r.TrashPath) != trash || filepath.Base(r.TrashPath) == "unreadable" { + t.Fatal(r.TrashPath) + } + if _, err := os.Stat(filepath.Join(r.TrashPath, "file")); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(trash, "unreadable", "kept")); err != nil { + t.Fatal("existing trash item replaced", err) + } +} + +// A .Trash swapped for a symlink after the checks is refused by the rename +// itself, so scratch never lands in the symlink's target. +func TestRenameIntoTrashRefusesSymlink(t *testing.T) { + home := t.TempDir() + elsewhere := t.TempDir() + if err := os.Symlink(elsewhere, filepath.Join(home, ".Trash")); err != nil { + t.Fatal(err) + } + src := t.TempDir() + if err := os.Mkdir(filepath.Join(src, "item"), 0o700); err != nil { + t.Fatal(err) + } + source, err := openDir(resolved(src)) + if err != nil { + t.Fatal(err) + } + defer source.Close() + if err := renameIntoTrash(source, "item", nil, filepath.Join(resolved(home), ".Trash", "item")); err == nil { + t.Fatal("rename followed a .Trash symlink") + } + if _, err := os.Stat(filepath.Join(elsewhere, "item")); !os.IsNotExist(err) { + t.Fatal("scratch moved through the symlink", err) + } +} diff --git a/internal/cleanup/trash_move_unix.go b/internal/cleanup/trash_move_unix.go index 6b89107..079619b 100644 --- a/internal/cleanup/trash_move_unix.go +++ b/internal/cleanup/trash_move_unix.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" "strings" - "syscall" "golang.org/x/sys/unix" ) @@ -62,17 +61,5 @@ func renameScratch(c Candidate, destination string) error { if !os.SameFile(info, c.identity.info) { return fmt.Errorf("scratch identity changed") } - target, err := openDir(filepath.Dir(destination)) - if err != nil { - return err - } - defer target.Close() - targetInfo, err := target.Stat() - if err != nil { - return err - } - if !sameDevice(rootInfo.Sys().(*syscall.Stat_t), targetInfo.Sys().(*syscall.Stat_t)) { - return syscall.EXDEV - } - return unix.Renameat(int(source.Fd()), name, int(target.Fd()), filepath.Base(destination)) + return renameIntoTrash(source, name, rootInfo, destination) } diff --git a/internal/cleanup/trash_rename_darwin.go b/internal/cleanup/trash_rename_darwin.go new file mode 100644 index 0000000..76b2766 --- /dev/null +++ b/internal/cleanup/trash_rename_darwin.go @@ -0,0 +1,24 @@ +package cleanup + +import ( + "os" + "path/filepath" + + "golang.org/x/sys/unix" +) + +// renameIntoTrash moves name from source into ~/.Trash. macOS privacy +// protection refuses to open ~/.Trash to a process without Full Disk Access, +// as the Driga service is, but lets it rename an item into it. So the move +// goes through a handle on the trash's parent, refuses a symlink anywhere in +// the destination path, and never replaces an existing item. The kernel +// refuses a cross-device move with EXDEV. +func renameIntoTrash(source *os.File, name string, _ os.FileInfo, destination string) error { + trashDir := filepath.Dir(destination) + parent, err := openDir(filepath.Dir(trashDir)) + if err != nil { + return err + } + defer parent.Close() + return unix.RenameatxNp(int(source.Fd()), name, int(parent.Fd()), filepath.Join(filepath.Base(trashDir), filepath.Base(destination)), unix.RENAME_EXCL|unix.RENAME_NOFOLLOW_ANY) +} diff --git a/internal/cleanup/trash_rename_linux.go b/internal/cleanup/trash_rename_linux.go new file mode 100644 index 0000000..4160347 --- /dev/null +++ b/internal/cleanup/trash_rename_linux.go @@ -0,0 +1,35 @@ +package cleanup + +import ( + "errors" + "os" + "path/filepath" + "syscall" + + "golang.org/x/sys/unix" +) + +// renameIntoTrash moves name from source into the trash directory through a +// handle on that directory, so no component of it can be swapped for a +// symlink in between, and never replaces an existing item. +func renameIntoTrash(source *os.File, name string, rootInfo os.FileInfo, destination string) error { + target, err := openDir(filepath.Dir(destination)) + if err != nil { + return err + } + defer target.Close() + targetInfo, err := target.Stat() + if err != nil { + return err + } + if !sameDevice(rootInfo.Sys().(*syscall.Stat_t), targetInfo.Sys().(*syscall.Stat_t)) { + return syscall.EXDEV + } + // Never replace an existing item. A filesystem without RENAME_NOREPLACE + // falls back to the plain rename the free-name check guards. + err = unix.Renameat2(int(source.Fd()), name, int(target.Fd()), filepath.Base(destination), unix.RENAME_NOREPLACE) + if errors.Is(err, unix.EINVAL) || errors.Is(err, unix.ENOSYS) { + err = unix.Renameat(int(source.Fd()), name, int(target.Fd()), filepath.Base(destination)) + } + return err +}