os/exec: detect incorrect usages of Cmd.StdoutPipe

[dsnet]

Consider the following program:

// Create some bogus file.
f := mustGet(os.CreateTemp("", ""))
mustGet(f.Write(make([]byte, 4e6)))

// Print the contents of the file and read it.
cmd := exec.Command("cat", f.Name())
rd := mustGet(cmd.StdoutPipe())
wr := slowWriter{}
go func() { fmt.Println(io.Copy(wr, rd)) }() // should read 4e6 bytes without error
mustDo(cmd.Run())

This fairly reliably fails with:

3964928 read |0: file already closed

The code above is in fact incorrect as it violates the documentation of Cmd.StdoutPipe which states:

Wait will close the pipe after seeing the command exit, so most callers need not close the pipe themselves. It is thus incorrect to call Wait before all reads from the pipe have completed. For the same reason, it is incorrect to call Run when using StdoutPipe. See the example for idiomatic usage.

However, reading the code itself does not obviously reveal the problem, nor does the problem always manifest (as it relies on the io.Copy being slow).

Given how subtle this problem is, perhaps we should provide static and/or dynamic analysis to detect this problem:

• static analysis: perhaps go vet can detect dynamic usages of StdoutPipe with concurrent Cmd.Wait or Cmd.Run.
• dynamic analysis: perhaps building with -race should produce a runtime Go race warning.

\cc @bcmills @maisem @bradfitz

[bcmills]

See also:

• os/exec: Current requirement for all reads to be completed before calling cmd.Wait can block indefinitely #60309
• os/exec: leaks os.pipes if cmd.Start() is never called #58369
• os: document concurrency properties for File methods #56043
• os/exec: data race between StdoutPipe and Wait #19685
• os/exec: data race between StdinPipe and Wait #9307
• os/exec: fix StdinPipe and maybe StdoutPipe #6270

[bcmills]

perhaps building with -race should produce a runtime Go race warning.

That's tricky, because the io.ReadCloser returned by StdoutPipe is currently always of type *os.File, and Hyrum's Law makes that property unfortunately sticky.

As of https://go.dev/cl/438347, a call to (*os.File).Read concurrent with a call to (*os.File).Close on a pipe (or a regular file) intentionally does not race.

So it seems to me that in order for the race detector to diagnose this mistake, we would need to either break existing assumptions about the concrete type returned by StdoutPipe, or add some kind of os.File hook (perhaps via the interface returned by the SyscallConn method?) to allow os/exec to deliberately cause a race.

(CC @ianlancetaylor)

[ianlancetaylor]

It seems to me that the code behaves as it should: you get an error if the file is already closed. This doesn't seem like a common enough problem that we should add hooks to the os package.

(If this is a common enough problem, then perhaps we should deprecate the StdoutPipe and StderrPipe methods. It always possible to set up a pipe yourself to get the same functionality without the undesired closing behavior.)