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.

Updates #52745.

Change-Id: Ic1111d688eebc8804a87d39d3261c2a6eb33f176
Reviewed-on: https://go-review.googlesource.com/c/go/+/588495
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>

diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index 8ecbffa81f7b6a5ded1645f362d967d3c01055b0..37bfaa1a72dfaebbd5a97454e46538ced9c7dd49 100644 (file)
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -152,7 +152,7 @@ const (
        kindSock
        // 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 and openDirNolog for directories.
+       // Used by openDirAt and openDirNolog for directories.
        kindNoPoll
 )
 
@@ -260,7 +260,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 and openDirNolog, if relevant.
+// Changes here should be reflected in openDirAt and openDirNolog, 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 87c4d805c3d8f8206877e062d09a9790f11d12f9..2a12add7a25e6bd703bad94974f05114b58d3417 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