[release-branch.go1.13] os: use an actual RemoveAll failure in TestRemoveAllWithMoreE...

Previously we injected an error, and the injection points were
(empirically) not realistic on some platforms.

Instead, we now make the directory read-only, which (on most
platforms) suffices to prevent the removal of its files.

Also remove unused test hook, as was done in CL 204060.

For #35117.
For #29921.
Fixes #37895.

Change-Id: Ica4e2818566f8c14df3eed7c3b8de5c0abeb6963
Reviewed-on: https://go-review.googlesource.com/c/go/+/203502
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 06bdd52f7540eca9e3ade6e78234d00703f3ee23)
Reviewed-on: https://go-review.googlesource.com/c/go/+/223700
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>

diff --git a/src/os/export_test.go b/src/os/export_test.go
index d17d5e62308a7ba36b05290421e9567421a1fb67..812432cee4869c0de1424105afb3f840d0cc67dc 100644 (file)
--- a/src/os/export_test.go
+++ b/src/os/export_test.go
@@ -9,4 +9,3 @@ package os
 var Atime = atime
 var LstatP = &lstat
 var ErrWriteAtInAppendMode = errWriteAtInAppendMode
-var RemoveAllTestHook = &removeAllTestHook

diff --git a/src/os/path.go b/src/os/path.go
index 9d7ecad79298ad06f57b4876d3b76280fee01f03..ba43ea352547b9e4018df73c55b2b8d9a6f9bc90 100644 (file)
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -58,9 +58,6 @@ func MkdirAll(path string, perm FileMode) error {
        return nil
 }
 
-// removeAllTestHook is a hook for testing.
-var removeAllTestHook = func(err error) error { return err }
-
 // RemoveAll removes path and any children it contains.
 // It removes everything it can but returns the first error
 // it encounters. If the path does not exist, RemoveAll

diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go
index bc632f5a751bd118f7090eae12efdb466535b7ba..e619851f9c8276b43614949471e9ec6efbf08557 100644 (file)
--- a/src/os/removeall_at.go
+++ b/src/os/removeall_at.go
@@ -153,7 +153,6 @@ func removeAllFrom(parent *File, base string) error {
 
        // Remove the directory itself.
        unlinkError := unix.Unlinkat(parentFd, base, unix.AT_REMOVEDIR)
-       unlinkError = removeAllTestHook(unlinkError)
        if unlinkError == nil || IsNotExist(unlinkError) {
                return nil
        }

diff --git a/src/os/removeall_noat.go b/src/os/removeall_noat.go
index a0694fa4ce03b1dc555f1a3ae34193c8784c7639..32673c0ab003a70a416762107c766c874cba55d3 100644 (file)
--- a/src/os/removeall_noat.go
+++ b/src/os/removeall_noat.go
@@ -124,7 +124,6 @@ func removeAll(path string) error {
 
        // Remove directory.
        err1 := Remove(path)
-       err1 = removeAllTestHook(err1)
        if err1 == nil || IsNotExist(err1) {
                return nil
        }

diff --git a/src/os/removeall_test.go b/src/os/removeall_test.go
index 4d556f977e4239b9024348651a743b732fb79b96..050635f5332576330fdaf7a48debe053bfcbb0a5 100644 (file)
--- a/src/os/removeall_test.go
+++ b/src/os/removeall_test.go
@@ -5,7 +5,6 @@
 package os_test
 
 import (
-       "errors"
        "fmt"
        "io/ioutil"
        . "os"
@@ -413,14 +412,6 @@ func TestRemoveAllWithMoreErrorThanReqSize(t *testing.T) {
                t.Skip("skipping in short mode")
        }
 
-       defer func(oldHook func(error) error) {
-               *RemoveAllTestHook = oldHook
-       }(*RemoveAllTestHook)
-
-       *RemoveAllTestHook = func(err error) error {
-               return errors.New("error from RemoveAllTestHook")
-       }
-
        tmpDir, err := ioutil.TempDir("", "TestRemoveAll-")
        if err != nil {
                t.Fatal(err)
@@ -429,7 +420,7 @@ func TestRemoveAllWithMoreErrorThanReqSize(t *testing.T) {
 
        path := filepath.Join(tmpDir, "_TestRemoveAllWithMoreErrorThanReqSize_")
 
-       // Make directory with 1025 files and remove.
+       // Make directory with 1025 read-only files.
        if err := MkdirAll(path, 0777); err != nil {
                t.Fatalf("MkdirAll %q: %s", path, err)
        }
@@ -442,13 +433,40 @@ func TestRemoveAllWithMoreErrorThanReqSize(t *testing.T) {
                fd.Close()
        }
 
-       // This call should not hang
-       if err := RemoveAll(path); err == nil {
-               t.Fatal("Want error from RemoveAllTestHook, got nil")
+       // Make the parent directory read-only. On some platforms, this is what
+       // prevents os.Remove from removing the files within that directory.
+       if err := Chmod(path, 0555); err != nil {
+               t.Fatal(err)
        }
+       defer Chmod(path, 0755)
 
-       // We hook to inject error, but the actual files must be deleted
-       if _, err := Lstat(path); err == nil {
-               t.Fatal("directory must be deleted even with removeAllTetHook run")
+       // This call should not hang, even on a platform that disallows file deletion
+       // from read-only directories.
+       err = RemoveAll(path)
+
+       if Getuid() == 0 {
+               // On many platforms, root can remove files from read-only directories.
+               return
+       }
+       if err == nil {
+               t.Fatal("RemoveAll(<read-only directory>) = nil; want error")
+       }
+
+       dir, err := Open(path)
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer dir.Close()
+
+       if runtime.GOOS == "windows" {
+               // Marking a directory in Windows does not prevent the os package from
+               // creating or removing files within it.
+               // (See https://golang.org/issue/35042.)
+               return
+       }
+
+       names, _ := dir.Readdirnames(1025)
+       if len(names) < 1025 {
+               t.Fatalf("RemoveAll(<read-only directory>) unexpectedly removed %d read-only files from that directory", 1025-len(names))
        }
 }