os/exec: clean up pipe-closing logic

Change the childFiles field to a local variable, since it was
populated during Start and (as far as I can determine) has no purpose
after Start returns.

Rename closeAfterStart and closeAfterWait to childIOFiles and
parentIOPipes respectively. That makes their contents clearer, and also
helps to clarify what should happen on error (when, for example, Wait
shouldn't be called at all).

Use a deferred call instead of individual calls to close child (and,
if necessary, pipe) FDs after Start. That helps to clarify the
invariants around when they are closed, and also makes the function a
bit more robust for future refactoring.

Also nil out the slices containing the file closers so that they can
be collected earlier.

This CL is intended as a pure refactor in preparation for #50436.

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

diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index 737aaab6a7dc3a3a5f829ee120a8fbd97c7c3eb8..9bef38533e5819d9146f79a572d89df4dd717460 100644 (file)
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -217,14 +217,24 @@ type Cmd struct {
        // available after a call to Wait or Run.
        ProcessState *os.ProcessState
 
-       ctx             context.Context // nil means none
-       Err             error           // LookPath error, if any.
-       childFiles      []*os.File
-       closeAfterStart []io.Closer
-       closeAfterWait  []io.Closer
-       goroutine       []func() error
-       goroutineErrs   <-chan error // one receive per goroutine
-       ctxErr          <-chan error // if non nil, receives the error from watchCtx exactly once
+       ctx context.Context // nil means none
+       Err error           // LookPath error, if any.
+
+       // childIOFiles holds closers for any of the child process's
+       // stdin, stdout, and/or stderr files that were opened by the Cmd itself
+       // (not supplied by the caller). These should be closed as soon as they
+       // are inherited by the child process.
+       childIOFiles []io.Closer
+
+       // parentIOPipes holds closers for the parent's end of any pipes
+       // connected to the child's stdin, stdout, and/or stderr streams
+       // that were opened by the Cmd itself (not supplied by the caller).
+       // These should be closed after Wait sees the command exit.
+       parentIOPipes []io.Closer
+
+       goroutine     []func() error
+       goroutineErrs <-chan error // one receive per goroutine
+       ctxErr        <-chan error // if non nil, receives the error from watchCtx exactly once
 
        // For a security release long ago, we created x/sys/execabs,
        // which manipulated the unexported lookPathErr error field
@@ -337,14 +347,14 @@ func (c *Cmd) argv() []string {
        return []string{c.Path}
 }
 
-func (c *Cmd) stdin() (f *os.File, err error) {
+func (c *Cmd) childStdin() (*os.File, error) {
        if c.Stdin == nil {
-               f, err = os.Open(os.DevNull)
+               f, err := os.Open(os.DevNull)
                if err != nil {
-                       return
+                       return nil, err
                }
-               c.closeAfterStart = append(c.closeAfterStart, f)
-               return
+               c.childIOFiles = append(c.childIOFiles, f)
+               return f, nil
        }
 
        if f, ok := c.Stdin.(*os.File); ok {
@@ -353,11 +363,11 @@ func (c *Cmd) stdin() (f *os.File, err error) {
 
        pr, pw, err := os.Pipe()
        if err != nil {
-               return
+               return nil, err
        }
 
-       c.closeAfterStart = append(c.closeAfterStart, pr)
-       c.closeAfterWait = append(c.closeAfterWait, pw)
+       c.childIOFiles = append(c.childIOFiles, pr)
+       c.parentIOPipes = append(c.parentIOPipes, pw)
        c.goroutine = append(c.goroutine, func() error {
                _, err := io.Copy(pw, c.Stdin)
                if skipStdinCopyError(err) {
@@ -371,25 +381,29 @@ func (c *Cmd) stdin() (f *os.File, err error) {
        return pr, nil
 }
 
-func (c *Cmd) stdout() (f *os.File, err error) {
+func (c *Cmd) childStdout() (*os.File, error) {
        return c.writerDescriptor(c.Stdout)
 }
 
-func (c *Cmd) stderr() (f *os.File, err error) {
+func (c *Cmd) childStderr(childStdout *os.File) (*os.File, error) {
        if c.Stderr != nil && interfaceEqual(c.Stderr, c.Stdout) {
-               return c.childFiles[1], nil
+               return childStdout, nil
        }
        return c.writerDescriptor(c.Stderr)
 }
 
-func (c *Cmd) writerDescriptor(w io.Writer) (f *os.File, err error) {
+// writerDescriptor returns an os.File to which the child process
+// can write to send data to w.
+//
+// If w is nil, writerDescriptor returns a File that writes to os.DevNull.
+func (c *Cmd) writerDescriptor(w io.Writer) (*os.File, error) {
        if w == nil {
-               f, err = os.OpenFile(os.DevNull, os.O_WRONLY, 0)
+               f, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0)
                if err != nil {
-                       return
+                       return nil, err
                }
-               c.closeAfterStart = append(c.closeAfterStart, f)
-               return
+               c.childIOFiles = append(c.childIOFiles, f)
+               return f, nil
        }
 
        if f, ok := w.(*os.File); ok {
@@ -398,11 +412,11 @@ func (c *Cmd) writerDescriptor(w io.Writer) (f *os.File, err error) {
 
        pr, pw, err := os.Pipe()
        if err != nil {
-               return
+               return nil, err
        }
 
-       c.closeAfterStart = append(c.closeAfterStart, pw)
-       c.closeAfterWait = append(c.closeAfterWait, pr)
+       c.childIOFiles = append(c.childIOFiles, pw)
+       c.parentIOPipes = append(c.parentIOPipes, pr)
        c.goroutine = append(c.goroutine, func() error {
                _, err := io.Copy(w, pr)
                pr.Close() // in case io.Copy stopped due to write error
@@ -470,12 +484,21 @@ func lookExtensions(path, dir string) (string, error) {
 // After a successful call to Start the Wait method must be called in
 // order to release associated system resources.
 func (c *Cmd) Start() error {
+       started := false
+       defer func() {
+               c.closeDescriptors(c.childIOFiles)
+               c.childIOFiles = nil
+
+               if !started {
+                       c.closeDescriptors(c.parentIOPipes)
+                       c.parentIOPipes = nil
+               }
+       }()
+
        if c.Path == "" && c.Err == nil && c.lookPathErr == nil {
                c.Err = errors.New("exec: no command")
        }
        if c.Err != nil || c.lookPathErr != nil {
-               c.closeDescriptors(c.closeAfterStart)
-               c.closeDescriptors(c.closeAfterWait)
                if c.lookPathErr != nil {
                        return c.lookPathErr
                }
@@ -484,8 +507,6 @@ func (c *Cmd) Start() error {
        if runtime.GOOS == "windows" {
                lp, err := lookExtensions(c.Path, c.Dir)
                if err != nil {
-                       c.closeDescriptors(c.closeAfterStart)
-                       c.closeDescriptors(c.closeAfterWait)
                        return err
                }
                c.Path = lp
@@ -496,25 +517,28 @@ func (c *Cmd) Start() error {
        if c.ctx != nil {
                select {
                case <-c.ctx.Done():
-                       c.closeDescriptors(c.closeAfterStart)
-                       c.closeDescriptors(c.closeAfterWait)
                        return c.ctx.Err()
                default:
                }
        }
 
-       c.childFiles = make([]*os.File, 0, 3+len(c.ExtraFiles))
-       type F func(*Cmd) (*os.File, error)
-       for _, setupFd := range []F{(*Cmd).stdin, (*Cmd).stdout, (*Cmd).stderr} {
-               fd, err := setupFd(c)
-               if err != nil {
-                       c.closeDescriptors(c.closeAfterStart)
-                       c.closeDescriptors(c.closeAfterWait)
-                       return err
-               }
-               c.childFiles = append(c.childFiles, fd)
+       childFiles := make([]*os.File, 0, 3+len(c.ExtraFiles))
+       stdin, err := c.childStdin()
+       if err != nil {
+               return err
        }
-       c.childFiles = append(c.childFiles, c.ExtraFiles...)
+       childFiles = append(childFiles, stdin)
+       stdout, err := c.childStdout()
+       if err != nil {
+               return err
+       }
+       childFiles = append(childFiles, stdout)
+       stderr, err := c.childStderr(stdout)
+       if err != nil {
+               return err
+       }
+       childFiles = append(childFiles, stderr)
+       childFiles = append(childFiles, c.ExtraFiles...)
 
        env, err := c.environ()
        if err != nil {
@@ -523,17 +547,14 @@ func (c *Cmd) Start() error {
 
        c.Process, err = os.StartProcess(c.Path, c.argv(), &os.ProcAttr{
                Dir:   c.Dir,
-               Files: c.childFiles,
+               Files: childFiles,
                Env:   env,
                Sys:   c.SysProcAttr,
        })
        if err != nil {
-               c.closeDescriptors(c.closeAfterStart)
-               c.closeDescriptors(c.closeAfterWait)
                return err
        }
-
-       c.closeDescriptors(c.closeAfterStart)
+       started = true
 
        // Don't allocate the goroutineErrs channel unless there are goroutines to fire.
        if len(c.goroutine) > 0 {
@@ -626,8 +647,8 @@ func (c *Cmd) Wait() error {
                err = copyError
        }
 
-       c.closeDescriptors(c.closeAfterWait)
-       c.closeAfterWait = nil
+       c.closeDescriptors(c.parentIOPipes)
+       c.parentIOPipes = nil
 
        return err
 }
@@ -726,9 +747,9 @@ func (c *Cmd) StdinPipe() (io.WriteCloser, error) {
                return nil, err
        }
        c.Stdin = pr
-       c.closeAfterStart = append(c.closeAfterStart, pr)
+       c.childIOFiles = append(c.childIOFiles, pr)
        wc := &closeOnce{File: pw}
-       c.closeAfterWait = append(c.closeAfterWait, wc)
+       c.parentIOPipes = append(c.parentIOPipes, wc)
        return wc, nil
 }
 
@@ -768,8 +789,8 @@ func (c *Cmd) StdoutPipe() (io.ReadCloser, error) {
                return nil, err
        }
        c.Stdout = pw
-       c.closeAfterStart = append(c.closeAfterStart, pw)
-       c.closeAfterWait = append(c.closeAfterWait, pr)
+       c.childIOFiles = append(c.childIOFiles, pw)
+       c.parentIOPipes = append(c.parentIOPipes, pr)
        return pr, nil
 }
 
@@ -793,8 +814,8 @@ func (c *Cmd) StderrPipe() (io.ReadCloser, error) {
                return nil, err
        }
        c.Stderr = pw
-       c.closeAfterStart = append(c.closeAfterStart, pw)
-       c.closeAfterWait = append(c.closeAfterWait, pr)
+       c.childIOFiles = append(c.childIOFiles, pw)
+       c.parentIOPipes = append(c.parentIOPipes, pr)
        return pr, nil
 }