[release-branch.go1.21] os: RemoveAll: fix symlink race for unix

Since all the platforms now support O_DIRECTORY flag for open, it can be
used to (together with O_NOFOLLOW) to ensure we open a directory, thus
eliminating the need to call stat before open. This fixes the symlink race,
when a directory is replaced by a symlink in between stat and open calls.

While at it, rename openFdAt to openDirAt, because this function is (and was)
meant for directories only.

NOTE Solaris supports O_DIRECTORY since before Solaris 11 (which is the
only version Go supports since supported version now), and Illumos
always had it. The only missing piece was O_DIRECTORY flag value, which
is taken from golang.org/x/sys/unix.

Fixes #67695.

Change-Id: Ic1111d688eebc8804a87d39d3261c2a6eb33f176
Reviewed-on: https://go-review.googlesource.com/c/go/+/589057
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>

diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index 533a48404b9d090e390823222f2566348e8478e0..f4709a4a295c12ffe82f0fdb84ea6ad9cda3b3ce 100644 (file)
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -157,7 +157,7 @@ const (
        kindNonBlock
        // kindNoPoll means that we should not put the descriptor into
        // non-blocking mode, because we know it is not a pipe or FIFO.
-       // Used by openFdAt for directories.
+       // Used by openDirAt for directories.
        kindNoPoll
 )
 
@@ -256,7 +256,7 @@ func epipecheck(file *File, e error) {
 const DevNull = "/dev/null"
 
 // openFileNolog is the Unix implementation of OpenFile.
-// Changes here should be reflected in openFdAt, if relevant.
+// Changes here should be reflected in openDirAt, if relevant.
 func openFileNolog(name string, flag int, perm FileMode) (*File, error) {
        setSticky := false
        if !supportsCreateWithStickyBit && flag&O_CREATE != 0 && perm&ModeSticky != 0 {

diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go
index 8ea5df411740fd7dfbb5025b11b478502ca1ff71..774ca15823bbc141c8c5293de729889065e0902d 100644 (file)
--- a/src/os/removeall_at.go
+++ b/src/os/removeall_at.go
@@ -74,22 +74,7 @@ func removeAllFrom(parent *File, base string) error {
        if err != syscall.EISDIR && err != syscall.EPERM && err != syscall.EACCES {
                return &PathError{Op: "unlinkat", Path: base, Err: err}
        }
-
-       // Is this a directory we need to recurse into?
-       var statInfo syscall.Stat_t
-       statErr := ignoringEINTR(func() error {
-               return unix.Fstatat(parentFd, base, &statInfo, unix.AT_SYMLINK_NOFOLLOW)
-       })
-       if statErr != nil {
-               if IsNotExist(statErr) {
-                       return nil
-               }
-               return &PathError{Op: "fstatat", Path: base, Err: statErr}
-       }
-       if statInfo.Mode&syscall.S_IFMT != syscall.S_IFDIR {
-               // Not a directory; return the error from the unix.Unlinkat.
-               return &PathError{Op: "unlinkat", Path: base, Err: err}
-       }
+       uErr := err
 
        // Remove the directory's entries.
        var recurseErr error
@@ -98,11 +83,15 @@ func removeAllFrom(parent *File, base string) error {
                var respSize int
 
                // Open the directory to recurse into
-               file, err := openFdAt(parentFd, base)
+               file, err := openDirAt(parentFd, base)
                if err != nil {
                        if IsNotExist(err) {
                                return nil
                        }
+                       if err == syscall.ENOTDIR {
+                               // Not a directory; return the error from the unix.Unlinkat.
+                               return &PathError{Op: "unlinkat", Path: base, Err: uErr}
+                       }
                        recurseErr = &PathError{Op: "openfdat", Path: base, Err: err}
                        break
                }
@@ -168,16 +157,19 @@ func removeAllFrom(parent *File, base string) error {
        return &PathError{Op: "unlinkat", Path: base, Err: unlinkError}
 }
 
-// openFdAt opens path relative to the directory in fd.
-// Other than that this should act like openFileNolog.
+// openDirAt opens a directory name relative to the directory referred to by
+// the file descriptor dirfd. If name is anything but a directory (this
+// includes a symlink to one), it should return an error. Other than that this
+// should act like openFileNolog.
+//
 // This acts like openFileNolog rather than OpenFile because
 // we are going to (try to) remove the file.
 // The contents of this file are not relevant for test caching.
-func openFdAt(dirfd int, name string) (*File, error) {
+func openDirAt(dirfd int, name string) (*File, error) {
        var r int
        for {
                var e error
-               r, e = unix.Openat(dirfd, name, O_RDONLY|syscall.O_CLOEXEC, 0)
+               r, e = unix.Openat(dirfd, name, O_RDONLY|syscall.O_CLOEXEC|syscall.O_DIRECTORY|syscall.O_NOFOLLOW, 0)
                if e == nil {
                        break
                }

diff --git a/src/syscall/zerrors_solaris_amd64.go b/src/syscall/zerrors_solaris_amd64.go
index 4a1d9c3d26669f9cdb410868b6ff1b626a6a4fb1..b2c81d9a51f97a2fa589400c1d9b370fd571c254 100644 (file)
--- a/src/syscall/zerrors_solaris_amd64.go
+++ b/src/syscall/zerrors_solaris_amd64.go
@@ -634,6 +634,7 @@ const (
        O_APPEND                      = 0x8
        O_CLOEXEC                     = 0x800000
        O_CREAT                       = 0x100
+       O_DIRECTORY                   = 0x1000000
        O_DSYNC                       = 0x40
        O_EXCL                        = 0x400
        O_EXEC                        = 0x400000