os: consistently return PathError from RemoveAll

Fixes #30491

Change-Id: If4070e5d39d8649643d7e90f6f3eb499642e25ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/164720
Run-TryBot: Baokun Lee <nototon@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>

src/os/path.go

@@ -62,6 +62,7 @@ func MkdirAll(path string, perm FileMode) error {
 // It removes everything it can but returns the first error
 // it encounters. If the path does not exist, RemoveAll
 // returns nil (no error).
+// If there is an error, it will be of type *PathError.
 func RemoveAll(path string) error {
 	return removeAll(path)
 }

src/os/path_unix.go

@@ -51,7 +51,7 @@ func splitPath(path string) (string, string) {
 	// Remove leading directory path
 	for i--; i >= 0; i-- {
 		if path[i] == '/' {
-			dirname = path[:i+1]
+			dirname = path[:i]
 			basename = path[i+1:]
 			break
 		}

src/os/removeall_at.go

@@ -46,13 +46,20 @@ func removeAll(path string) error {
 	}
 	defer parent.Close()
 
-	return removeAllFrom(parent, base)
+	if err := removeAllFrom(parent, base); err != nil {
+		if pathErr, ok := err.(*PathError); ok {
+			pathErr.Path = parentDir + string(PathSeparator) + pathErr.Path
+			err = pathErr
+		}
+		return err
+	}
+	return nil
 }
 
-func removeAllFrom(parent *File, path string) error {
+func removeAllFrom(parent *File, base string) error {
 	parentFd := int(parent.Fd())
 	// Simple case: if Unlink (aka remove) works, we're done.
-	err := unix.Unlinkat(parentFd, path, 0)
+	err := unix.Unlinkat(parentFd, base, 0)
 	if err == nil || IsNotExist(err) {
 		return nil
 	}
@@ -64,21 +71,21 @@ func removeAllFrom(parent *File, path string) error {
 	// whose contents need to be removed.
 	// Otherwise just return the error.
 	if err != syscall.EISDIR && err != syscall.EPERM && err != syscall.EACCES {
-		return err
+		return &PathError{"unlinkat", base, err}
 	}
 
 	// Is this a directory we need to recurse into?
 	var statInfo syscall.Stat_t
-	statErr := unix.Fstatat(parentFd, path, &statInfo, unix.AT_SYMLINK_NOFOLLOW)
+	statErr := unix.Fstatat(parentFd, base, &statInfo, unix.AT_SYMLINK_NOFOLLOW)
 	if statErr != nil {
 		if IsNotExist(statErr) {
 			return nil
 		}
-		return statErr
+		return &PathError{"fstatat", base, statErr}
 	}
 	if statInfo.Mode&syscall.S_IFMT != syscall.S_IFDIR {
-		// Not a directory; return the error from the Remove.
-		return err
+		// Not a directory; return the error from the unix.Unlinkat.
+		return &PathError{"unlinkat", base, err}
 	}
 
 	// Remove the directory's entries.
@@ -87,12 +94,12 @@ func removeAllFrom(parent *File, path string) error {
 		const request = 1024
 
 		// Open the directory to recurse into
-		file, err := openFdAt(parentFd, path)
+		file, err := openFdAt(parentFd, base)
 		if err != nil {
 			if IsNotExist(err) {
 				return nil
 			}
-			recurseErr = err
+			recurseErr = &PathError{"openfdat", base, err}
 			break
 		}
 
@@ -103,12 +110,15 @@ func removeAllFrom(parent *File, path string) error {
 			if IsNotExist(readErr) {
 				return nil
 			}
-			return readErr
+			return &PathError{"readdirnames", base, readErr}
 		}
 
 		for _, name := range names {
 			err := removeAllFrom(file, name)
 			if err != nil {
+				if pathErr, ok := err.(*PathError); ok {
+					pathErr.Path = base + string(PathSeparator) + pathErr.Path
+				}
 				recurseErr = err
 			}
 		}
@@ -127,7 +137,7 @@ func removeAllFrom(parent *File, path string) error {
 	}
 
 	// Remove the directory itself.
-	unlinkError := unix.Unlinkat(parentFd, path, unix.AT_REMOVEDIR)
+	unlinkError := unix.Unlinkat(parentFd, base, unix.AT_REMOVEDIR)
 	if unlinkError == nil || IsNotExist(unlinkError) {
 		return nil
 	}
@@ -135,7 +145,7 @@ func removeAllFrom(parent *File, path string) error {
 	if recurseErr != nil {
 		return recurseErr
 	}
-	return unlinkError
+	return &PathError{"unlinkat", base, unlinkError}
 }
 
 // openFdAt opens path relative to the directory in fd.
@@ -157,7 +167,7 @@ func openFdAt(dirfd int, name string) (*File, error) {
 			continue
 		}
 
-		return nil, &PathError{"openat", name, e}
+		return nil, e
 	}
 
 	if !supportsCloseOnExec {

src/os/removeall_test.go

@@ -286,7 +286,7 @@ func TestRemoveReadOnlyDir(t *testing.T) {
 }
 
 // Issue #29983.
-func TestRemoveAllButReadOnly(t *testing.T) {
+func TestRemoveAllButReadOnlyAndPathError(t *testing.T) {
 	switch runtime.GOOS {
 	case "nacl", "js", "windows":
 		t.Skipf("skipping test on %s", runtime.GOOS)
@@ -347,10 +347,21 @@ func TestRemoveAllButReadOnly(t *testing.T) {
 		defer Chmod(d, 0777)
 	}
 
-	if err := RemoveAll(tempDir); err == nil {
+	err = RemoveAll(tempDir)
+	if err == nil {
 		t.Fatal("RemoveAll succeeded unexpectedly")
 	}
 
+	// The error should be of type *PathError.
+	// see issue 30491 for details.
+	if pathErr, ok := err.(*PathError); ok {
+		if g, w := pathErr.Path, filepath.Join(tempDir, "b", "y"); g != w {
+			t.Errorf("got %q, expected pathErr.path %q", g, w)
+		}
+	} else {
+		t.Errorf("got %T, expected *os.PathError", err)
+	}
+
 	for _, dir := range dirs {
 		_, err := Stat(filepath.Join(tempDir, dir))
 		if inReadonly(dir) {