From 8cb2952f2f9c80246572b951e2663e79962796c0 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Fri, 22 Sep 2017 12:03:52 -0700 Subject: [PATCH] os/exec: remove protection against simultaneous Wait/Write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CL 31148 added code to protect again simultaneous calls to Close and Wait when using the standard input pipe, to fix the race condition described in issue #9307. That issue is a special case of the race between Close and Write described by issue #7970. Since issue #7970 was not fixed, CL 31148 fixed the problem specific to os/exec. Since then, issue #7970 has been fixed, so the specific fix in os/exec is no longer necessary. Remove it, effectively reverting CL 31148 and followup CL 33298. Updates #7970 Updates #9307 Updates #17647 Change-Id: Ic0b62569cb0aba44b32153cf5f9632bd1f1b411a Reviewed-on: https://go-review.googlesource.com/65490 Run-TryBot: Ian Lance Taylor Reviewed-by: Daniel Martí Reviewed-by: Miguel Bernabeu Reviewed-by: Russ Cox Reviewed-by: Joe Tsai TryBot-Result: Gobot Gobot --- src/os/exec/exec.go | 56 +++------------------------------------------ 1 file changed, 3 insertions(+), 53 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 893d8ee99a8..b0fe14d6fdd 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -527,16 +527,15 @@ 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, closerFunc(wc.safeClose)) + c.closeAfterWait = append(c.closeAfterWait, wc) return wc, nil } type closeOnce struct { *os.File - writers sync.RWMutex // coordinate safeClose and Write - once sync.Once - err error + once sync.Once + err error } func (c *closeOnce) Close() error { @@ -548,55 +547,6 @@ 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 -} - -func (c *closeOnce) WriteString(s string) (int, error) { - c.writers.RLock() - n, err := c.File.WriteString(s) - c.writers.RUnlock() - return n, err -} - // StdoutPipe returns a pipe that will be connected to the command's // standard output when the command starts. //