diff options
author | Bryan C. Mills <bcmills@google.com> | 2019-10-25 15:37:00 -0400 |
---|---|---|
committer | Dmitri Shuralyov <dmitshur@golang.org> | 2020-03-31 23:05:42 +0000 |
commit | f353662952b48af110dc878adfa533745e168716 (patch) | |
tree | 47f2b110886b33c43fbabd8fac757b499cc3c60d | |
parent | 7261619113b1f35fc9260a44b66bc1f9a13f142f (diff) | |
download | go-f353662952b48af110dc878adfa533745e168716.tar.gz go-f353662952b48af110dc878adfa533745e168716.zip |
[release-branch.go1.13] os: use an actual RemoveAll failure in TestRemoveAllWithMoreErrorThanReqSize
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>
-rw-r--r-- | src/os/export_test.go | 1 | ||||
-rw-r--r-- | src/os/path.go | 3 | ||||
-rw-r--r-- | src/os/removeall_at.go | 1 | ||||
-rw-r--r-- | src/os/removeall_noat.go | 1 | ||||
-rw-r--r-- | src/os/removeall_test.go | 50 |
5 files changed, 34 insertions, 22 deletions
diff --git a/src/os/export_test.go b/src/os/export_test.go index d17d5e6230..812432cee4 100644 --- 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 9d7ecad792..ba43ea3525 100644 --- 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 bc632f5a75..e619851f9c 100644 --- 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 a0694fa4ce..32673c0ab0 100644 --- 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 4d556f977e..050635f533 100644 --- 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)) } } |