syscall: serialize locks on ForkLock on platforms where forkExecPipe is not atomic

In CL 421441, we changed syscall to allow concurrent calls to
forkExec.

On platforms that support the pipe2 syscall that is the right
behavior, because pipe2 atomically opens the pipe with CLOEXEC already
set.

However, on platforms that do not support pipe2 (currently aix and
darwin), syscall.forkExecPipe is not atomic, and the pipes do not
initially have CLOEXEC set. If two calls to forkExec proceed
concurrently, a pipe intended for one child process can be
accidentally inherited by the other. If the process is long-lived, the
pipe can be held open unexpectedly and prevent the parent process from
reaching EOF reading the child's status from the pipe.

Fixes #61080.
Updates #23558.
Updates #54162.

Change-Id: I83edcc80674ff267a39d06260c5697c654ff5a4b
Reviewed-on: https://go-review.googlesource.com/c/go/+/507355
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>

1 files changed, 72 insertions, 0 deletions

diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index 67cd446f42..d37fffd39d 100644
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -1708,3 +1708,75 @@ func TestCancelErrors(t *testing.T) {
 		}
 	})
 }
+
+// TestConcurrentExec is a regression test for https://go.dev/issue/61080.
+//
+// Forking multiple child processes concurrently would sometimes hang on darwin.
+// (This test hung on a gomote with -count=100 after only a few iterations.)
+func TestConcurrentExec(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+
+	// This test will spawn nHangs subprocesses that hang reading from stdin,
+	// and nExits subprocesses that exit immediately.
+	//
+	// When issue #61080 was present, a long-lived "hang" subprocess would
+	// occasionally inherit the fork/exec status pipe from an "exit" subprocess,
+	// causing the parent process (which expects to see an EOF on that pipe almost
+	// immediately) to unexpectedly block on reading from the pipe.
+	var (
+		nHangs       = runtime.GOMAXPROCS(0)
+		nExits       = runtime.GOMAXPROCS(0)
+		hangs, exits sync.WaitGroup
+	)
+	hangs.Add(nHangs)
+	exits.Add(nExits)
+
+	// ready is done when the goroutines have done as much work as possible to
+	// prepare to create subprocesses. It isn't strictly necessary for the test,
+	// but helps to increase the repro rate by making it more likely that calls to
+	// syscall.StartProcess for the "hang" and "exit" goroutines overlap.
+	var ready sync.WaitGroup
+	ready.Add(nHangs + nExits)
+
+	for i := 0; i < nHangs; i++ {
+		go func() {
+			defer hangs.Done()
+
+			cmd := helperCommandContext(t, ctx, "pipetest")
+			stdin, err := cmd.StdinPipe()
+			if err != nil {
+				ready.Done()
+				t.Error(err)
+				return
+			}
+			cmd.Cancel = stdin.Close
+			ready.Done()
+
+			ready.Wait()
+			if err := cmd.Start(); err != nil {
+				t.Error(err)
+				return
+			}
+
+			cmd.Wait()
+		}()
+	}
+
+	for i := 0; i < nExits; i++ {
+		go func() {
+			defer exits.Done()
+
+			cmd := helperCommandContext(t, ctx, "exit", "0")
+			ready.Done()
+
+			ready.Wait()
+			if err := cmd.Run(); err != nil {
+				t.Error(err)
+			}
+		}()
+	}
+
+	exits.Wait()
+	cancel()
+	hangs.Wait()
+}