From 4d6ca68a85e42c75683bc96aa540207566f54e26 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 28 Sep 2022 13:29:12 -0400 Subject: [PATCH] os/exec: do not close pipes on a double-Start error This fixes a bug introduced in CL 401834 in which calling Start twice with pipes attached to a command would spuriously close those pipes when returning the error from the second Start call. For #50436. Change-Id: I3563cc99c0a0987752190fef95da3e9927a76fda Reviewed-on: https://go-review.googlesource.com/c/go/+/436095 Reviewed-by: Ian Lance Taylor TryBot-Result: Gopher Robot Run-TryBot: Bryan Mills Auto-Submit: Bryan Mills --- src/os/exec/exec.go | 9 ++++++--- src/os/exec/exec_test.go | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index 67dd379b71..0dac34447f 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -490,6 +490,12 @@ 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 { + // Check for doubled Start calls before we defer failure cleanup. If the prior + // call to Start succeeded, we don't want to spuriously close its pipes. + if c.Process != nil { + return errors.New("exec: already started") + } + started := false defer func() { c.closeDescriptors(c.childIOFiles) @@ -519,9 +525,6 @@ func (c *Cmd) Start() error { } c.Path = lp } - if c.Process != nil { - return errors.New("exec: already started") - } if c.ctx != nil { select { case <-c.ctx.Done(): diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go index 52001bf9e3..07ac0cf3d4 100644 --- a/src/os/exec/exec_test.go +++ b/src/os/exec/exec_test.go @@ -1094,3 +1094,46 @@ func TestNoPath(t *testing.T) { t.Errorf("new(Cmd).Start() = %v, want %q", err, want) } } + +// TestDoubleStartLeavesPipesOpen checks for a regression in which calling +// Start twice, which returns an error on the second call, would spuriously +// close the pipes established in the first call. +func TestDoubleStartLeavesPipesOpen(t *testing.T) { + cmd := helperCommand(t, "pipetest") + in, err := cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + out, err := cmd.StdoutPipe() + if err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err == nil || !strings.HasSuffix(err.Error(), "already started") { + t.Fatalf("second call to Start returned a nil; want an 'already started' error") + } + + outc := make(chan []byte, 1) + go func() { + b, err := io.ReadAll(out) + if err != nil { + t.Error(err) + } + outc <- b + }() + + const msg = "O:Hello, pipe!\n" + + _, err = io.WriteString(in, msg) + if err != nil { + t.Fatal(err) + } + in.Close() + + b := <-outc + if !bytes.Equal(b, []byte(msg)) { + t.Fatalf("read %q from stdout pipe; want %q", b, msg) + } +} -- 2.50.0