From 3d77a0b15ea2a6d2f7b3e2ba483f148d7c6ee174 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 9 Dec 2025 10:06:23 -0500 Subject: os/exec: second call to Cmd.Start is always an error Previously it would return an error only if the first call resulted in process creation, contra the intent of the comment at exec.Cmd: // A Cmd cannot be reused after calling its [Cmd.Start], [Cmd.Run], // [Cmd.Output], or [Cmd.CombinedOutput] methods. Also, clear the Cmd.goroutines slice in case of failure to start a process, so that the closures can be GC'd and their pipe fds finalized and closed. Fixes #76746 Change-Id: Ic63a4dced0aa52c2d4be7d44f6dcfc84ee22282c Reviewed-on: https://go-review.googlesource.com/c/go/+/728642 LUCI-TryBot-Result: Go LUCI Reviewed-by: Damien Neil --- src/os/exec/exec.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'src/os/exec/exec.go') diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index e84ebfc453..e0c509cbef 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -102,6 +102,7 @@ import ( "runtime" "strconv" "strings" + "sync/atomic" "syscall" "time" ) @@ -354,6 +355,9 @@ type Cmd struct { // the work of resolving the extension, so Start doesn't need to do it again. // This is only used on Windows. cachedLookExtensions struct{ in, out string } + + // startCalled records that Start was attempted, regardless of outcome. + startCalled atomic.Bool } // A ctxResult reports the result of watching the Context associated with a @@ -635,7 +639,8 @@ func (c *Cmd) Run() error { 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 { + // It is an error to call Start twice even if the first call did not create a process. + if c.startCalled.Swap(true) { return errors.New("exec: already started") } @@ -647,6 +652,7 @@ func (c *Cmd) Start() error { if !started { closeDescriptors(c.parentIOPipes) c.parentIOPipes = nil + c.goroutine = nil // aid GC, finalization of pipe fds } }() -- cgit v1.3 From d1d0fc7a97539206e33f04bac935f6450597137c Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 6 Jan 2026 14:48:31 -0500 Subject: os/exec: avoid atomic.Bool for Cmd.startCalled An atomic.Bool isn't necessary here since, unless otherwise specified, the methods of an object are not concurrency-safe w.r.t. each other. Using an atomic causes the copylocks vet check to warn about copying of Cmd, which is not wrong, because one shouldn't be copying opaque complex structs from other packages, but it is a nuisance in the absence of any safe way to copy a Cmd. If and when we add a Clone method to Cmd (see #77075) then it would be appropriate to revert this change so that we get the benefit of the static check (though ideally we would make a more explicit tool-readable declaration of the "do not copy" attribute than merely happening to use an atomic.Bool). For #77075 Change-Id: I982d4e86623ca165a3e76bbf648fd44041d5f6bb Reviewed-on: https://go-review.googlesource.com/c/go/+/734200 Reviewed-by: Michael Pratt LUCI-TryBot-Result: Go LUCI --- src/os/exec/exec.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src/os/exec/exec.go') diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index e0c509cbef..aa7a6be7f0 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -357,7 +357,9 @@ type Cmd struct { cachedLookExtensions struct{ in, out string } // startCalled records that Start was attempted, regardless of outcome. - startCalled atomic.Bool + // (Until go.dev/issue/77075 is resolved, we use atomic.SwapInt32, + // not atomic.Bool.Swap, to avoid triggering the copylocks vet check.) + startCalled int32 } // A ctxResult reports the result of watching the Context associated with a @@ -640,7 +642,7 @@ 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. // It is an error to call Start twice even if the first call did not create a process. - if c.startCalled.Swap(true) { + if atomic.SwapInt32(&c.startCalled, 1) != 0 { return errors.New("exec: already started") } -- cgit v1.3