os/exec: leaks os.pipes if cmd.Start() is never called

[lucasec]

What version of Go are you using (go version)?

$ go version
go version go1.19.3 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/[REDACTED]/Library/Caches/go-build"
GOENV="/Users/[REDACTED]/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/[REDACTED]/go/pkg/mod"
GONOPROXY="[REDACTED]"
GONOSUMDB="[REDACTED]"
GOOS="darwin"
GOPATH="/Users/[REDACTED]/go"
GOPRIVATE="[REDACTED]"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.19.3/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.19.3/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cf/t924_8dj02lf0wkmq06hcxsh0000gn/T/go-build892371702=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

cmd := exec.Command(myCommand, myArgs...)

stdout, err := cmd.StdoutPipe()
if err != nil {
	// log warning or bail out
}

stderr, err := cmd.StderrPipe()
if err != nil {
	// log warning, if we bail out we will leak the os.Pipe allocated internally in cmd.StdoutPipe()
}

err = cmd.Start() // if this fails, it *will* clean up both pipes

What did you expect to see?

There should be some pattern I can follow to bail out and never Start() the command, without also leaking the pipes allocated by StdoutPipe() and StderrPipe().

What did you see instead?

A close reading of the latest source for the os/Exec module shows there is no mechanism to clean up the allocated pipes without first calling cmd.Start() and then invoking cmd.Wait(). While error conditions in StdoutPipe() and StderrPipe() should be rare (the system would likely have to run out of file descriptors), such scenarios can and do happen in production code, especially when services get stressed under load (such as during a denial of service attack). It should be possible to write "safe" code that can at least react to these scenarios and not pour more fuel on the fire (e.g. by leaking os.pipe file descriptors).

Discussion

I'd be open to contributing a solution to this API limitation, but would appreciate the community's input on what an idiomatic solution would be.

The simplest approach may be to modify cmd.Wait() to clean up pipes even when the reason for erroring out is "not started" (e.g. https://cs.opensource.google/go/go/+/refs/tags/go1.19.5:src/os/exec/exec.go;l=594). Otherwise, a new method could be introduced for the explicit purpose of cleaning up a command without starting it (since for backwards compatibility we could not separate out the "cleanup" function from Wait).

[seankhliao]

If you run Cmd.Start() with a command with a canceled context, it should just return while also cleaning up the descriptors

[ianlancetaylor]

@seankhliao Thanks, that is clever. Perhaps we should simply document that somewhere.

[lucasec]

I actually thought of that as I was writing this. I would not describe using a cancelled context as a remotely intuitive behavior though—documentation (and maybe adding to one of the examples) could help but ideally it would be simple and obvious how to use the API safely.

[bcmills]

The simplest approach may be to modify cmd.Wait() to clean up pipes even when the reason for erroring out is "not started"

I think that makes sense, and it would be the most consistent with the existing guidance that “Wait will close the pipe”.

The workaround of calling Start with the canceled context assumes that you have control over what Context the Cmd is created with, and may also require allocating an otherwise-unnecessary context.WithCancel. As a workaround it's not terrible, but it doesn't seem as clean as something like

defer func() {
	if err != nil {
		cmd.Wait()  // Clean up any allocated pipes.
	}
}()

[bcmills]

Another possible workaround is to call os.Pipe yourself and plumb it in through the Stdin etc. fields, but given that the (*Cmd).…Pipe methods already exist I would rather make them possible to use safely. 😅

[seankhliao]

Currently it's safe to retry Wait if it errors on not started. Will we break people if we change that?

[bcmills]

If the call to Wait closes any pipes, it should cause a subsequent Start to return an error. That sequence (Pipe; Wait; Start) seems obscure enough that I would be surprised if it breaks any existing program that isn't already broken.

(I would guess that most existing programs that call Start after Wait do so by accident and omit the second Wait needed to clean up and check errors.)

[beoran]

Maybe we should add a Close() function for this?

[bcmills]

I don't think it warrants a Close function — it would have exactly the same signature and semantics as Wait. 😅