os/exec: allow simultaneous cmd.Wait and Write of cmd.StdinPipe

cmd.StdinPipe returns an io.WriteCloser.
It's reasonable to expect the caller not to call Write and Close simultaneously,
but there is an implicit Close in cmd.Wait that's not obvious.
We already synchronize the implicit Close in cmd.Wait against
any explicit Close from the caller. Also synchronize that implicit
Close against any explicit Write from the caller.

Fixes #9307.

Change-Id: I8561e9369d6e5ac88dfbca1175549f6dfa04b8ac
Reviewed-on: https://go-review.googlesource.com/31148
Reviewed-by: Brad Fitzpatrick <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>

Author: rsc

src/os/exec/exec.go

@@ -515,15 +515,16 @@ func (c *Cmd) StdinPipe() (io.WriteCloser, error) {
 	c.Stdin = pr
 	c.closeAfterStart = append(c.closeAfterStart, pr)
 	wc := &closeOnce{File: pw}
-	c.closeAfterWait = append(c.closeAfterWait, wc)
+	c.closeAfterWait = append(c.closeAfterWait, closerFunc(wc.safeClose))
 	return wc, nil
 }
 
 type closeOnce struct {
 	*os.File
 
-	once sync.Once
-	err  error
+	writers sync.RWMutex // coordinate safeClose and Write
+	once    sync.Once
+	err     error
 }
 
 func (c *closeOnce) Close() error {
@@ -535,6 +536,48 @@ func (c *closeOnce) close() {
 	c.err = c.File.Close()
 }
 
+type closerFunc func() error
+
+func (f closerFunc) Close() error { return f() }
+
+// safeClose closes c being careful not to race with any calls to c.Write.
+// See golang.org/issue/9307 and TestEchoFileRace in exec_test.go.
+// In theory other calls could also be excluded (by writing appropriate
+// wrappers like c.Write's implementation below), but since c is most
+// commonly used as a WriteCloser, Write is the main one to worry about.
+// See also #7970, for which this is a partial fix for this specific instance.
+// The idea is that we return a WriteCloser, and so the caller can be
+// relied upon not to call Write and Close simultaneously, but it's less
+// obvious that cmd.Wait calls Close and that the caller must not call
+// Write and cmd.Wait simultaneously. In fact that seems too onerous.
+// So we change the use of Close in cmd.Wait to use safeClose, which will
+// synchronize with any Write.
+//
+// It's important that we know this won't block forever waiting for the
+// operations being excluded. At the point where this is called,
+// the invoked command has exited and the parent copy of the read side
+// of the pipe has also been closed, so there should really be no read side
+// of the pipe left. Any active writes should return very shortly with an EPIPE,
+// making it reasonable to wait for them.
+// Technically it is possible that the child forked a sub-process or otherwise
+// handed off the read side of the pipe before exiting and the current holder
+// is not reading from the pipe, and the pipe is full, in which case the close here
+// might block waiting for the write to complete. That's probably OK.
+// It's a small enough problem to be outweighed by eliminating the race here.
+func (c *closeOnce) safeClose() error {
+	c.writers.Lock()
+	err := c.Close()
+	c.writers.Unlock()
+	return err
+}
+
+func (c *closeOnce) Write(b []byte) (int, error) {
+	c.writers.RLock()
+	n, err := c.File.Write(b)
+	c.writers.RUnlock()
+	return n, err
+}
+
 // StdoutPipe returns a pipe that will be connected to the command's
 // standard output when the command starts.
 //

src/os/exec/exec_test.go

@@ -101,6 +101,26 @@ func TestCatStdin(t *testing.T) {
 	}
 }
 
+func TestEchoFileRace(t *testing.T) {
+	cmd := helperCommand(t, "echo")
+	stdin, err := cmd.StdinPipe()
+	if err != nil {
+		t.Fatalf("StdinPipe: %v", err)
+	}
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("Start: %v", err)
+	}
+	wrote := make(chan bool)
+	go func() {
+		defer close(wrote)
+		fmt.Fprint(stdin, "echo\n")
+	}()
+	if err := cmd.Wait(); err != nil {
+		t.Fatalf("Wait: %v", err)
+	}
+	<-wrote
+}
+
 func TestCatGoodAndBadFile(t *testing.T) {
 	// Testing combined output and error values.
 	bs, err := helperCommand(t, "cat", "/bogus/file.foo", "exec_test.go").CombinedOutput()