tstest/integration: fix ETXTBSY race when copying test binaries

Tests that exec a fresh CopyTo copy of the tailscale/tailscaled
binaries occasionally failed with "text file busy" on GitHub
Actions, previously worked around with the retry loop in
awaitTailscaledRunnable.

The root cause: CopyTo's Linux hardlink fast path only works while
the built binary is still linked somewhere. The kernel refuses to
hardlink an inode whose link count is zero, even via the still-open
FD, so once the building test's TempDir (and every other test's copy)
has been cleaned up, later tests silently fall through to the
byte-copy path. That path writes the new copy from the test process
itself, and if another parallel test forks a child while the write FD
is open, the child holds the inherited FD (O_CLOEXEC only closes at
exec, not at fork) and a subsequent exec of the fresh copy fails with
ETXTBSY. This is golang.org/issue/22315. On macOS and the BSDs,
CopyTo always takes the byte-copy path, so every copy races there.

Fix it the way the syscall package documents: hold ForkLock for
reading across the copy so that no fork overlaps the lifetime of the
write FD. A standalone stress program reproduced the race in 86 of
200 execs under a fork storm and in 0 of 200 with the lock held.

Updates #15868
Updates #15865

Change-Id: I8a6be72826c9073a9187d7e6c90c8733d2dadb05
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

tstest/integration/integration.go

@@ -31,6 +31,7 @@
 	"strconv"
 	"strings"
 	"sync"
+	"syscall"
 	"testing"
 	"time"
 
@@ -113,29 +114,16 @@ func (b BinaryInfo) CopyTo(dir string) (BinaryInfo, error) {
 		// full copy of the binary. We can't use os.Link(b.Path, ret.Path)
 		// because b.Path is in the first test's TempDir, which may be
 		// cleaned up before later tests call CopyTo. The open FD keeps the
-		// inode alive after the path is deleted.
+		// inode alive after the path is deleted, but only for reading:
+		// once the inode's link count drops to zero the kernel refuses
+		// to hardlink it again, so this fails and we fall through to
+		// copying the bytes instead.
 		if err := tryLinkat(b.FD, ret.Path); err == nil {
 			return ret, nil
 		}
 		fallthrough
 	case "darwin", "freebsd", "openbsd", "netbsd":
-		f, err := os.OpenFile(ret.Path, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o755)
-		if err != nil {
-			return BinaryInfo{}, err
-		}
-		b.FDMu.Lock()
-		b.FD.Seek(0, 0)
-		size, err := io.Copy(f, b.FD)
-		b.FDMu.Unlock()
-		if err != nil {
-			f.Close()
-			return BinaryInfo{}, fmt.Errorf("copying %q: %w", b.Path, err)
-		}
-		if size != b.Size {
-			f.Close()
-			return BinaryInfo{}, fmt.Errorf("copy %q: size mismatch: %d != %d", b.Path, size, b.Size)
-		}
-		if err := f.Close(); err != nil {
+		if err := b.writeCopy(ret.Path); err != nil {
 			return BinaryInfo{}, err
 		}
 		return ret, nil
@@ -146,6 +134,38 @@ func (b BinaryInfo) CopyTo(dir string) (BinaryInfo, error) {
 	}
 }
 
+// writeCopy writes the binary's contents from b.FD to path.
+//
+// It holds syscall.ForkLock for reading for the duration of the write
+// so that no concurrently forked child inherits the transient write
+// FD. A forked child holds inherited FDs (even O_CLOEXEC ones) until
+// it execs, and an exec of the new copy fails with ETXTBSY as long as
+// any process holds a write FD on it (golang.org/issue/22315). This
+// was the cause of the once-mysterious ETXTBSY errors
+// (https://github.com/tailscale/tailscale/issues/15868) that
+// [TestNode.awaitTailscaledRunnable] retries around.
+func (b BinaryInfo) writeCopy(path string) error {
+	syscall.ForkLock.RLock()
+	defer syscall.ForkLock.RUnlock()
+
+	f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o755)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+	b.FDMu.Lock()
+	b.FD.Seek(0, 0)
+	size, err := io.Copy(f, b.FD)
+	b.FDMu.Unlock()
+	if err != nil {
+		return fmt.Errorf("copying %q: %w", b.Path, err)
+	}
+	if size != b.Size {
+		return fmt.Errorf("copy %q: size mismatch: %d != %d", b.Path, size, b.Size)
+	}
+	return f.Close()
+}
+
 // GetBinaries create a temp directory using tb and builds (or copies previously
 // built) cmd/tailscale and cmd/tailscaled binaries into that directory.
 //
@@ -890,10 +910,11 @@ type waitResult struct {
 }
 
 // awaitTailscaledRunnable tries to run `tailscaled --version` until it
-// works. This is an unsatisfying workaround for ETXTBSY we were seeing
-// on GitHub Actions that aren't understood. It's not clear what's holding
-// a writable fd to tailscaled after `go install` completes.
-// See https://github.com/tailscale/tailscale/issues/15868.
+// works. It began as a workaround for mysterious ETXTBSY errors on
+// GitHub Actions (https://github.com/tailscale/tailscale/issues/15868),
+// whose cause is now understood and fixed (see [BinaryInfo.writeCopy]).
+// It remains as cheap insurance against any other transient exec
+// failure.
 func (n *TestNode) awaitTailscaledRunnable() error {
 	t := n.env.t
 	t.Helper()