From: Bryan C. Mills Date: Thu, 21 Apr 2022 20:06:00 +0000 (-0400) Subject: os/exec: clean up pipe-closing logic X-Git-Tag: go1.20rc1~937 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=b8d4a14a660827ea0331eb6cad99860bf5fdf66f;p=gostls13.git 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 Reviewed-by: Ian Lance Taylor Auto-Submit: Bryan Mills Run-TryBot: Bryan Mills --- diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 737aaab6a7..9bef38533e 100644 --- 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 }