runtime: don't race cmd.Wait and cmd.StdoutPipe read

os/exec.Cmd.Wait closes the read end of os/exec.Cmd.StdoutPipe, meaning
that io.ReadAll can return fs.ErrClosed if the child exits too early,
allowing Wait to complete. The StdoutPipe docs already note this sharp
edge.

Move cmd.Wait until after we finish waiting on stdout. If the child
crashes for some reason, the write end of the pipe will implicitly close
causing io.ReadAll to return as well, so we won't get stuck.

Fixes #52725.

Change-Id: Ifae5745d86206879af2f1523a664236972e07252
Reviewed-on: https://go-review.googlesource.com/c/go/+/420597
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>

diff --git a/src/runtime/semasleep_test.go b/src/runtime/semasleep_test.go
index d56733c0cf195b46080926bf2a31ebfc18a7754e..726285346555d5738d2841731ef6ac71629bcd09 100644 (file)
--- a/src/runtime/semasleep_test.go
+++ b/src/runtime/semasleep_test.go
@@ -37,14 +37,16 @@ func TestSpuriousWakeupsNeverHangSemasleep(t *testing.T) {
        if err := cmd.Start(); err != nil {
                t.Fatalf("Failed to start command: %v", err)
        }
+
+       waiting := false
        doneCh := make(chan error, 1)
-       go func() {
-               doneCh <- cmd.Wait()
-               close(doneCh)
-       }()
        t.Cleanup(func() {
                cmd.Process.Kill()
-               <-doneCh
+               if waiting {
+                       <-doneCh
+               } else {
+                       cmd.Wait()
+               }
        })
 
        // Wait for After1 to close its stdout so that we know the runtime's SIGIO
@@ -57,6 +59,19 @@ func TestSpuriousWakeupsNeverHangSemasleep(t *testing.T) {
                t.Fatalf("error reading from testprog: %v", err)
        }
 
+       // Wait for child exit.
+       //
+       // Note that we must do this after waiting for the write/child end of
+       // stdout to close. Wait closes the read/parent end of stdout, so
+       // starting this goroutine prior to io.ReadAll introduces a race
+       // condition where ReadAll may get fs.ErrClosed if the child exits too
+       // quickly.
+       waiting = true
+       go func() {
+               doneCh <- cmd.Wait()
+               close(doneCh)
+       }()
+
        // Wait for an arbitrary timeout longer than one second. The subprocess itself
        // attempts to sleep for one second, but if the machine running the test is
        // heavily loaded that subprocess may not schedule very quickly even if the