From a10da772a68d692c2c8805b11aba9b6cb9920b15 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Mon, 1 Aug 2022 16:25:15 -0400 Subject: [PATCH] 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 TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills --- src/runtime/semasleep_test.go | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/runtime/semasleep_test.go b/src/runtime/semasleep_test.go index d56733c0cf..7262853465 100644 --- 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 -- 2.50.0