os: treat EACCES as a permission error in RemoveAll

Fixes #29983

Change-Id: I24077bde991e621c23d00973b2a77bb3a18e4ae7
Reviewed-on: https://go-review.googlesource.com/c/160180
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go
index fe8b1faf2b3f222555803186a7c2537e8ed8408e..7f2d5922ae03c3f5ab387c4af2dbb39986f047ff 100644 (file)
--- a/src/os/removeall_at.go
+++ b/src/os/removeall_at.go
@@ -57,8 +57,13 @@ func removeAllFrom(parent *File, path string) error {
                return nil
        }
 
-       // If not a "is directory" error, we have a problem
-       if err != syscall.EISDIR && err != syscall.EPERM {
+       // EISDIR means that we have a directory, and we need to
+       // remove its contents.
+       // EPERM or EACCES means that we don't have write permission on
+       // the parent directory, but this entry might still be a directory
+       // whose contents need to be removed.
+       // Otherwise just return the error.
+       if err != syscall.EISDIR && err != syscall.EPERM && err != syscall.EACCES {
                return err
        }
 
@@ -69,11 +74,11 @@ func removeAllFrom(parent *File, path string) error {
                return statErr
        }
        if statInfo.Mode&syscall.S_IFMT != syscall.S_IFDIR {
-               // Not a directory; return the error from the Remove
+               // Not a directory; return the error from the Remove.
                return err
        }
 
-       // Remove the directory's entries
+       // Remove the directory's entries.
        var recurseErr error
        for {
                const request = 1024
@@ -88,7 +93,7 @@ func removeAllFrom(parent *File, path string) error {
                }
 
                names, readErr := file.Readdirnames(request)
-               // Errors other than EOF should stop us from continuing
+               // Errors other than EOF should stop us from continuing.
                if readErr != nil && readErr != io.EOF {
                        file.Close()
                        if IsNotExist(readErr) {
@@ -117,7 +122,7 @@ func removeAllFrom(parent *File, path string) error {
                }
        }
 
-       // Remove the directory itself
+       // Remove the directory itself.
        unlinkError := unix.Unlinkat(parentFd, path, unix.AT_REMOVEDIR)
        if unlinkError == nil || IsNotExist(unlinkError) {
                return nil

diff --git a/src/os/removeall_test.go b/src/os/removeall_test.go
index 0f7dce078a6b081769a4db3936b898e9beaaeca3..9dab0d4bb1079a8881e4a7d1437d822aed2696d2 100644 (file)
--- a/src/os/removeall_test.go
+++ b/src/os/removeall_test.go
@@ -292,3 +292,83 @@ func TestRemoveReadOnlyDir(t *testing.T) {
                t.Error("subdirectory was not removed")
        }
 }
+
+// Issue #29983.
+func TestRemoveAllButReadOnly(t *testing.T) {
+       switch runtime.GOOS {
+       case "nacl", "js", "windows":
+               t.Skipf("skipping test on %s", runtime.GOOS)
+       }
+
+       if Getuid() == 0 {
+               t.Skip("skipping test when running as root")
+       }
+
+       t.Parallel()
+
+       tempDir, err := ioutil.TempDir("", "TestRemoveAllButReadOnly-")
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer RemoveAll(tempDir)
+
+       dirs := []string{
+               "a",
+               "a/x",
+               "a/x/1",
+               "b",
+               "b/y",
+               "b/y/2",
+               "c",
+               "c/z",
+               "c/z/3",
+       }
+       readonly := []string{
+               "b",
+       }
+       inReadonly := func(d string) bool {
+               for _, ro := range readonly {
+                       if d == ro {
+                               return true
+                       }
+                       dd, _ := filepath.Split(d)
+                       if filepath.Clean(dd) == ro {
+                               return true
+                       }
+               }
+               return false
+       }
+
+       for _, dir := range dirs {
+               if err := Mkdir(filepath.Join(tempDir, dir), 0777); err != nil {
+                       t.Fatal(err)
+               }
+       }
+       for _, dir := range readonly {
+               d := filepath.Join(tempDir, dir)
+               if err := Chmod(d, 0555); err != nil {
+                       t.Fatal(err)
+               }
+
+               // Defer changing the mode back so that the deferred
+               // RemoveAll(tempDir) can succeed.
+               defer Chmod(d, 0777)
+       }
+
+       if err := RemoveAll(tempDir); err == nil {
+               t.Fatal("RemoveAll succeeded unexpectedly")
+       }
+
+       for _, dir := range dirs {
+               _, err := Stat(filepath.Join(tempDir, dir))
+               if inReadonly(dir) {
+                       if err != nil {
+                               t.Errorf("file %q was deleted but should still exist", dir)
+                       }
+               } else {
+                       if err == nil {
+                               t.Errorf("file %q still exists but should have been deleted", dir)
+                       }
+               }
+       }
+}