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

lucasec · 2023-02-06T22:42:11Z

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).

The text was updated successfully, but these errors were encountered:

seankhliao · 2023-02-06T22:59:10Z

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

ianlancetaylor · 2023-02-07T00:07:40Z

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

lucasec · 2023-02-07T01:20:41Z

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 · 2023-02-07T15:48:11Z

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 · 2023-02-07T15:50:03Z

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 · 2023-02-07T16:49:04Z

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

bcmills · 2023-02-07T18:26:03Z

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 · 2023-02-08T13:56:41Z

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

bcmills · 2023-02-08T14:00:18Z

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

bcmills added this to the Backlog milestone Feb 7, 2023

bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 7, 2023

bcmills closed this as completed Feb 7, 2023

bcmills reopened this Feb 7, 2023

bcmills mentioned this issue Jun 21, 2023

os/exec: detect incorrect usages of Cmd.StdoutPipe #60908

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lucasec commented Feb 6, 2023

seankhliao commented Feb 6, 2023

ianlancetaylor commented Feb 7, 2023

lucasec commented Feb 7, 2023

bcmills commented Feb 7, 2023 •

edited

bcmills commented Feb 7, 2023

seankhliao commented Feb 7, 2023

bcmills commented Feb 7, 2023 •

edited

beoran commented Feb 8, 2023

bcmills commented Feb 8, 2023

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

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

Comments

lucasec commented Feb 6, 2023

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

Discussion

seankhliao commented Feb 6, 2023

ianlancetaylor commented Feb 7, 2023

lucasec commented Feb 7, 2023

bcmills commented Feb 7, 2023 • edited

bcmills commented Feb 7, 2023

seankhliao commented Feb 7, 2023

bcmills commented Feb 7, 2023 • edited

beoran commented Feb 8, 2023

bcmills commented Feb 8, 2023

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

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

bcmills commented Feb 7, 2023 •

edited

bcmills commented Feb 7, 2023 •

edited