os: RemoveAll: fix symlink race for unix

kolyshkin · gopherbot · commit a3a584e4abad · 2024-05-29T13:52:34.000Z
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
@@ -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
@@ -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
