From 896097000912761dbd31cead2bec99f17534f521 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 28 Mar 2025 16:40:34 -0700 Subject: [PATCH] os: add Root.RemoveAll For #67002 Change-Id: If59dab4fd934a115d8ff383826525330de750b54 Reviewed-on: https://go-review.googlesource.com/c/go/+/661595 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI Auto-Submit: Damien Neil --- api/next/67002.txt | 1 + doc/next/6-stdlib/99-minor/os/67002.md | 1 + src/os/removeall_at.go | 8 +-- src/os/root.go | 6 ++ src/os/root_noopenat.go | 20 ++++++ src/os/root_openat.go | 22 ++++++ src/os/root_test.go | 92 ++++++++++++++++++++++++++ src/os/root_unix.go | 3 + src/os/root_windows.go | 3 + 9 files changed, 151 insertions(+), 5 deletions(-) diff --git a/api/next/67002.txt b/api/next/67002.txt index 112f477e8e..274f200538 100644 --- a/api/next/67002.txt +++ b/api/next/67002.txt @@ -4,5 +4,6 @@ pkg os, method (*Root) Chtimes(string, time.Time, time.Time) error #67002 pkg os, method (*Root) Lchown(string, int, int) error #67002 pkg os, method (*Root) Link(string, string) error #67002 pkg os, method (*Root) Readlink(string) (string, error) #67002 +pkg os, method (*Root) RemoveAll(string) error #67002 pkg os, method (*Root) Rename(string, string) error #67002 pkg os, method (*Root) Symlink(string, string) error #67002 diff --git a/doc/next/6-stdlib/99-minor/os/67002.md b/doc/next/6-stdlib/99-minor/os/67002.md index 84661c6c40..62f1b36054 100644 --- a/doc/next/6-stdlib/99-minor/os/67002.md +++ b/doc/next/6-stdlib/99-minor/os/67002.md @@ -6,5 +6,6 @@ The [os.Root] type supports the following additional methods: * [os.Root.Lchown] * [os.Root.Link] * [os.Root.Readlink] + * [os.Root.RemoveAll] * [os.Root.Rename] * [os.Root.Symlink] diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go index 0d9ebd2e4f..a613aeeb91 100644 --- a/src/os/removeall_at.go +++ b/src/os/removeall_at.go @@ -44,7 +44,7 @@ func removeAll(path string) error { } defer parent.Close() - if err := removeAllFrom(parent, base); err != nil { + if err := removeAllFrom(sysfdType(parent.Fd()), base); err != nil { if pathErr, ok := err.(*PathError); ok { pathErr.Path = parentDir + string(PathSeparator) + pathErr.Path err = pathErr @@ -54,9 +54,7 @@ func removeAll(path string) error { return nil } -func removeAllFrom(parent *File, base string) error { - parentFd := sysfdType(parent.Fd()) - +func removeAllFrom(parentFd sysfdType, base string) error { // Simple case: if Unlink (aka remove) works, we're done. err := removefileat(parentFd, base) if err == nil || IsNotExist(err) { @@ -109,7 +107,7 @@ func removeAllFrom(parent *File, base string) error { respSize = len(names) for _, name := range names { - err := removeAllFrom(file, name) + err := removeAllFrom(sysfdType(file.Fd()), name) if err != nil { if pathErr, ok := err.(*PathError); ok { pathErr.Path = base + string(PathSeparator) + pathErr.Path diff --git a/src/os/root.go b/src/os/root.go index fb2bb8350a..9b9deaecc4 100644 --- a/src/os/root.go +++ b/src/os/root.go @@ -177,6 +177,12 @@ func (r *Root) Remove(name string) error { return rootRemove(r, name) } +// RemoveAll removes the named file or directory and any children that it contains. +// See [RemoveAll] for more details. +func (r *Root) RemoveAll(name string) error { + return rootRemoveAll(r, name) +} + // Stat returns a [FileInfo] describing the named file in the root. // See [Stat] for more details. func (r *Root) Stat(name string) (FileInfo, error) { diff --git a/src/os/root_noopenat.go b/src/os/root_noopenat.go index 47d6ebfa82..b34416284f 100644 --- a/src/os/root_noopenat.go +++ b/src/os/root_noopenat.go @@ -9,6 +9,7 @@ package os import ( "errors" "sync/atomic" + "syscall" "time" ) @@ -156,6 +157,25 @@ func rootRemove(r *Root, name string) error { return nil } +func rootRemoveAll(r *Root, name string) error { + if endsWithDot(name) { + // Consistency with os.RemoveAll: Return EINVAL when trying to remove . + return &PathError{Op: "RemoveAll", Path: name, Err: syscall.EINVAL} + } + if err := checkPathEscapesLstat(r, name); err != nil { + if err == syscall.ENOTDIR { + // Some intermediate path component is not a directory. + // RemoveAll treats this as success (since the target doesn't exist). + return nil + } + return &PathError{Op: "RemoveAll", Path: name, Err: err} + } + if err := RemoveAll(joinPath(r.root.name, name)); err != nil { + return &PathError{Op: "RemoveAll", Path: name, Err: underlyingError(err)} + } + return nil +} + func rootReadlink(r *Root, name string) (string, error) { if err := checkPathEscapesLstat(r, name); err != nil { return "", &PathError{Op: "readlinkat", Path: name, Err: err} diff --git a/src/os/root_openat.go b/src/os/root_openat.go index db2ae2295f..b57506a2eb 100644 --- a/src/os/root_openat.go +++ b/src/os/root_openat.go @@ -138,6 +138,28 @@ func rootRemove(r *Root, name string) error { return nil } +func rootRemoveAll(r *Root, name string) error { + // Consistency with os.RemoveAll: Strip trailing /s from the name, + // so RemoveAll("not_a_directory/") succeeds. + for len(name) > 0 && IsPathSeparator(name[len(name)-1]) { + name = name[:len(name)-1] + } + if endsWithDot(name) { + // Consistency with os.RemoveAll: Return EINVAL when trying to remove . + return &PathError{Op: "RemoveAll", Path: name, Err: syscall.EINVAL} + } + _, err := doInRoot(r, name, func(parent sysfdType, name string) (struct{}, error) { + return struct{}{}, removeAllFrom(parent, name) + }) + if IsNotExist(err) { + return nil + } + if err != nil { + return &PathError{Op: "RemoveAll", Path: name, Err: underlyingError(err)} + } + return err +} + func rootRename(r *Root, oldname, newname string) error { _, err := doInRoot(r, oldname, func(oldparent sysfdType, oldname string) (struct{}, error) { _, err := doInRoot(r, newname, func(newparent sysfdType, newname string) (struct{}, error) { diff --git a/src/os/root_test.go b/src/os/root_test.go index 530d5abda6..c75a094730 100644 --- a/src/os/root_test.go +++ b/src/os/root_test.go @@ -630,6 +630,46 @@ func TestRootRemoveDirectory(t *testing.T) { } } +func TestRootRemoveAll(t *testing.T) { + for _, test := range rootTestCases { + test.run(t, func(t *testing.T, target string, root *os.Root) { + wantError := test.wantError + if test.ltarget != "" { + // Remove doesn't follow symlinks in the final path component, + // so it will successfully remove ltarget. + wantError = false + target = filepath.Join(root.Name(), test.ltarget) + } else if target != "" { + if err := os.Mkdir(target, 0o777); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(target, "file"), nil, 0o666); err != nil { + t.Fatal(err) + } + } + targetExists := true + if _, err := root.Lstat(test.open); errors.Is(err, os.ErrNotExist) { + // If the target doesn't exist, RemoveAll succeeds rather + // than returning ErrNotExist. + targetExists = false + wantError = false + } + + err := root.RemoveAll(test.open) + if errEndsTest(t, err, wantError, "root.RemoveAll(%q)", test.open) { + return + } + if !targetExists { + return + } + _, err = os.Lstat(target) + if !errors.Is(err, os.ErrNotExist) { + t.Fatalf(`stat file removed with Root.Remove(%q): %v, want ErrNotExist`, test.open, err) + } + }) + } +} + func TestRootOpenFileAsRoot(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "target") @@ -958,6 +998,9 @@ type rootConsistencyTest struct { // detailedErrorMismatch indicates that os.Root and the corresponding non-Root // function return different errors for this test. detailedErrorMismatch func(t *testing.T) bool + + // check is called before the test starts, and may t.Skip if necessary. + check func(t *testing.T) } var rootConsistencyTestCases = []rootConsistencyTest{{ @@ -1115,6 +1158,16 @@ var rootConsistencyTestCases = []rootConsistencyTest{{ // and os.Open returns "The file cannot be accessed by the system.". return runtime.GOOS == "windows" }, + check: func(t *testing.T) { + if runtime.GOOS == "windows" && strings.HasPrefix(t.Name(), "TestRootConsistencyRemoveAll/") { + // Root.RemoveAll notices that a/ is not a directory, + // and returns success. + // os.RemoveAll tries to open a/ and fails because + // it is not a regular file. + // The inconsistency here isn't worth fixing, so just skip this test. + t.Skip("known inconsistency on windows") + } + }, }, { name: "question mark", open: "?", @@ -1156,6 +1209,10 @@ func (test rootConsistencyTest) run(t *testing.T, f func(t *testing.T, path stri } t.Run(test.name, func(t *testing.T) { + if test.check != nil { + test.check(t) + } + dir1 := makefs(t, test.fs) dir2 := makefs(t, test.fs) if test.fsFunc != nil { @@ -1321,6 +1378,23 @@ func TestRootConsistencyRemove(t *testing.T) { } } +func TestRootConsistencyRemoveAll(t *testing.T) { + for _, test := range rootConsistencyTestCases { + if test.open == "." || test.open == "./" { + continue // can't remove the root itself + } + test.run(t, func(t *testing.T, path string, r *os.Root) (string, error) { + var err error + if r == nil { + err = os.RemoveAll(path) + } else { + err = r.RemoveAll(path) + } + return "", err + }) + } +} + func TestRootConsistencyStat(t *testing.T) { for _, test := range rootConsistencyTestCases { test.run(t, func(t *testing.T, path string, r *os.Root) (string, error) { @@ -1759,3 +1833,21 @@ func TestOpenInRoot(t *testing.T) { } } } + +func TestRootRemoveDot(t *testing.T) { + dir := t.TempDir() + root, err := os.OpenRoot(dir) + if err != nil { + t.Fatal(err) + } + defer root.Close() + if err := root.Remove("."); err == nil { + t.Errorf(`root.Remove(".") = %v, want error`, err) + } + if err := root.RemoveAll("."); err == nil { + t.Errorf(`root.RemoveAll(".") = %v, want error`, err) + } + if _, err := os.Stat(dir); err != nil { + t.Error(`root.Remove(All)?(".") removed the root`) + } +} diff --git a/src/os/root_unix.go b/src/os/root_unix.go index 74320451d5..45462c9e10 100644 --- a/src/os/root_unix.go +++ b/src/os/root_unix.go @@ -14,6 +14,9 @@ import ( "time" ) +// sysfdType is the native type of a file handle +// (int on Unix, syscall.Handle on Windows), +// permitting helper functions to be written portably. type sysfdType = int // openRootNolog is OpenRoot. diff --git a/src/os/root_windows.go b/src/os/root_windows.go index b096d78b68..3d3db1916e 100644 --- a/src/os/root_windows.go +++ b/src/os/root_windows.go @@ -77,6 +77,9 @@ func rootCleanPath(s string, prefix, suffix []string) (string, error) { return s, nil } +// sysfdType is the native type of a file handle +// (int on Unix, syscall.Handle on Windows), +// permitting helper functions to be written portably. type sysfdType = syscall.Handle // openRootNolog is OpenRoot. -- 2.50.0