diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go index fe8b1faf2b3f22..7f2d5922ae03c3 100644 --- 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 0f7dce078a6b08..9dab0d4bb1079a 100644 --- 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) + } + } + } +}