os/exec: Cmd.Start should fail when called more than once

[everzakov]

Go version

go version go1.24.6 linux/amd64

Output of go env in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/vboxuser/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/vboxuser/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build4029059829=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/dev/null'
GOMODCACHE='/home/vboxuser/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/vboxuser/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/vboxuser/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.6'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

I'm trying to start again Cmd if the first try was unsuccessful. However, Cmd.Wait() returns an error if it is OK with command.

The cause is that the goroutines are saved from the first try but the pipes are closed. The process is not started that's why parentsIOPipes will be closed. And if this function try to read from pipe then the we will get the error.

Example:

package main

import (
	"bytes"
	"fmt"
	"os/exec"
)

func main() {
	var stdout2 bytes.Buffer
	cmd := exec.Command("bash", "-c", "sleep 10")
	cmd.Stdout = &stdout2

	var str string
	// Set 0 to env to return err in cmd.Start()
	str = string([]byte{0})
	cmd.Env = []string{str}

	err := cmd.Start()
	fmt.Println(err)

	cmd.Env = nil
	err = cmd.Start()
	fmt.Println(err)

	err = cmd.Wait()
	fmt.Println(err)
}

Example where cmd.Start is called twice - https://github.com/opencontainers/runc/blob/v1.4.0/libcontainer/process_linux.go#L370

What did you see happen?

vboxuser@vboxuser:~/golang-bug$ go run main.go 
exec: environment variable contains NUL
<nil>
read |0: file already closed

What did you expect to see?

Cmd.Wait() do not return an error. If c.goroutine = nil is set in defer function then the error is not returned.

vboxuser@vboxuser:~/golang-bug$ go run main.go 
exec: environment variable contains NUL
<nil>
<nil>

[gabyhelp]

Related Issues

• os/exec: possible race handling stdout&stderr pipes #69060
• os/exec: race on cmd.Wait() might lead to panic #28461
• os/exec: consider changing Wait to stop copying goroutines rather than waiting for them #23019 (closed)
• os/exec: .Wait(): non-deterministic behavior (race?) #68308 (closed)
• os/exec: Unable to close pipes properly if the caller can't close the pipes ahead #76057 (closed)
• os/exec: close read end of stdout/stderr pipe when copy loop stops #10400 (closed)
• affected/package: os/exec, context not work. #53700 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[adonovan]

A Cmd object can be used for at most one process. See:

// A Cmd cannot be reused after calling its [Cmd.Start], [Cmd.Run],
// [Cmd.Output], or [Cmd.CombinedOutput] methods.

Therefore this is working as intended. That said, it would be better if Start detected this misuse and panicked, rather than returning an error, since errors from Start are very often handled in such a way that the program recovers and does something else, meaning that the mistake is not detected.

(Update: Start does detect this misuse, but returns an error. It doesn't seem worth the trouble to make it panic.)

[everzakov]

Therefore this is working as intended

I don't think so because the error is returned by cmd.Wait not by cmd.Start. cmd.Start does not return an error.

[adonovan]

Therefore this is working as intended

I don't think so because the error is returned by cmd.Wait not by cmd.Start. cmd.Start does not return an error.

If you violate the precondition, the package's behavior is undefined. Ideally it would panic, but it doesn't promise to do that. (Go tries to avoid C++'s expansive definition of undefined behavior, which has traditionally permitted the program to do anything, including spend your bitcoin to hire a hitman to kill you.)

[everzakov]

A Cmd object can be used for at most one process. See:

// A Cmd cannot be reused after calling its [Cmd.Start], [Cmd.Run],
// [Cmd.Output], or [Cmd.CombinedOutput] methods.
Therefore this is working as intended. That said, it would be better if Start detected this misuse and panicked, rather than returning an error, since errors from Start are very often handled in such a way that the program recovers and does something else, meaning that the mistake is not detected.

In my opinion, this rule should be related only if one of these methods didn't return an error. In my example, the first cmd.Start() returns an error. Also, this rule was added after this issue. The argument was that cmd.Start() is blocked for reuse because we have c.Process. However, in my example c.Process is equal to nil because the first cmd.Start() returns an error.

Also, i think it will be a good practice that we will clear c.goroutine in the defer function like we do with parentIOPipes.

[adonovan]

In my opinion, this rule should be related only if one of these methods didn't return an error. In my example, the first cmd.Start() returns an error. Also, this rule was added after this issue. The argument was that cmd.Start() is blocked for reuse because we have c.Process. However, in my example c.Process is equal to nil because the first cmd.Start() returns an error.

It seems to me that the recently added comment is trying to communicate the simple invariant that you should not call Start more than once. It happens to be implemented in terms of the Process field, but I don't think that's the point. If Start fails and you need to try again, create a new Cmd object.

[adonovan]

Since Start already does detect re-use (by returning an error) I don't think any changes are necessary, so I'm going to close this issue.

[everzakov]

Since Start already does detect re-use (by returning an error) I

Sorry but you are wrong. The second cmd.Start do not return an error. The error is returned by cmd.Wait. I have changed the code to be more transparent

package main

import (
	"bytes"
	"fmt"
	"os/exec"
)

func main() {
	var stdout2 bytes.Buffer
	cmd := exec.Command("sleep", "3")
	cmd.Stdout = &stdout2

	var str string
	// Set 0 to env to return err in cmd.Start()
	str = string([]byte{0})
	cmd.Env = []string{str}

	err := cmd.Start()
	fmt.Printf("First cmd.Start: %v\n", err)

	cmd.Env = nil
	err = cmd.Start()
	fmt.Printf("Second cmd.Start: %v\n", err)

	err = cmd.Wait()
	fmt.Printf("cmd.Wait: %v\n", err)
}

First cmd.Start: exec: environment variable contains NUL
Second cmd.Start: <nil>
cmd.Wait: read |0: file already closed

If you don't want to copy paste the example, you can use playground https://play.golang.com/p/5C53cNzebfH .

[adonovan]

Since Start already does detect re-use (by returning an error)...

Sorry but you are wrong. The second cmd.Start do not return an error. The error is returned by cmd.Wait. I have changed the code to be more transparent

The point is that it is a mistake to call Start more than once, and Start makes some effort to detect it (by checking .Process). It could make more effort, by checking a hidden boolean that Starts set even before it attempts to create a process. We can certainly implement that (I'll reopen the issue) but I suspect it's a pretty small minority of uses of Start that fail in this way.

Sorry but you are wrong.

By the way, this particular phrasing comes across as quite rude in English. You might find your future interactions with English-speaking strangers on the Internet are smoother if you choose different words to express disagreement.