Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
dsnet opened this issue Jun 21, 2023 · 3 comments
Open

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

dsnet opened this issue Jun 21, 2023 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@dsnet
Copy link
Member

dsnet commented Jun 21, 2023 •
edited

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
@dsnet dsnet changed the title os/exec: detect incorrect usages of Exec.StdoutPipe os/exec: detect incorrect usages of Cmd.StdoutPipe Jun 21, 2023
@bcmills
Copy link
Contributor

bcmills commented Jun 21, 2023 •
edited

See also:

@bcmills
Copy link
Contributor

bcmills commented Jun 21, 2023

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)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 21, 2023
@bcmills bcmills added this to the Backlog milestone Jun 21, 2023
@ianlancetaylor
Copy link
Contributor

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

dsnet added a commit to tailscale/tailscale that referenced this issue Jun 22, 2023
@dsnet @maisem
ssh/tailssh: fix double race condition with non-pty command
58a3d75 
There are two race conditions in output handling.

The first race condition is due to a misuse of exec.Cmd.StdoutPipe.
The documentation explicitly forbids concurrent use of StdoutPipe
with exec.Cmd.Wait (see golang/go#60908) because Wait will
close both sides of the pipe once the process ends without
any guarantees that all data has been read from the pipe.
To fix this, we allocate the os.Pipes ourselves and
manage cleanup ourselves when the process has ended.

The second race condition is because sshSession.run waits
upon exec.Cmd to finish and then immediately proceeds to call ss.Exit,
which will close all output streams going to the SSH client.
This may interrupt any asynchronous io.Copy still copying data.
To fix this, we close the write-side of the os.Pipes after
the process has finished (and before calling ss.Exit) and
synchronously wait for the io.Copy routines to finish.

Fixes #7601

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
Co-authored-by: Maisem Ali <maisem@tailscale.com>
@dsnet dsnet mentioned this issue Jun 22, 2023
Merged
dsnet added a commit to tailscale/tailscale that referenced this issue Jun 22, 2023
@dsnet @maisem
ssh/tailssh: fix double race condition with non-pty command
789504d 
There are two race conditions in output handling.

The first race condition is due to a misuse of exec.Cmd.StdoutPipe.
The documentation explicitly forbids concurrent use of StdoutPipe
with exec.Cmd.Wait (see golang/go#60908) because Wait will
close both sides of the pipe once the process ends without
any guarantees that all data has been read from the pipe.
To fix this, we allocate the os.Pipes ourselves and
manage cleanup ourselves when the process has ended.

The second race condition is because sshSession.run waits
upon exec.Cmd to finish and then immediately proceeds to call ss.Exit,
which will close all output streams going to the SSH client.
This may interrupt any asynchronous io.Copy still copying data.
To fix this, we close the write-side of the os.Pipes after
the process has finished (and before calling ss.Exit) and
synchronously wait for the io.Copy routines to finish.

Fixes #7601

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
Co-authored-by: Maisem Ali <maisem@tailscale.com>
dsnet added a commit to tailscale/tailscale that referenced this issue Jun 22, 2023
@dsnet @maisem
ssh/tailssh: fix double race condition with non-pty command
1c3cd6f 
There are two race conditions in output handling.

The first race condition is due to a misuse of exec.Cmd.StdoutPipe.
The documentation explicitly forbids concurrent use of StdoutPipe
with exec.Cmd.Wait (see golang/go#60908) because Wait will
close both sides of the pipe once the process ends without
any guarantees that all data has been read from the pipe.
To fix this, we allocate the os.Pipes ourselves and
manage cleanup ourselves when the process has ended.

The second race condition is because sshSession.run waits
upon exec.Cmd to finish and then immediately proceeds to call ss.Exit,
which will close all output streams going to the SSH client.
This may interrupt any asynchronous io.Copy still copying data.
To fix this, we close the write-side of the os.Pipes after
the process has finished (and before calling ss.Exit) and
synchronously wait for the io.Copy routines to finish.

Fixes #7601

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
Co-authored-by: Maisem Ali <maisem@tailscale.com>
dsnet added a commit to tailscale/tailscale that referenced this issue Jun 22, 2023
@dsnet @maisem
ssh/tailssh: fix double race condition with non-pty command (#8405)
61886e0 
There are two race conditions in output handling.

The first race condition is due to a misuse of exec.Cmd.StdoutPipe.
The documentation explicitly forbids concurrent use of StdoutPipe
with exec.Cmd.Wait (see golang/go#60908) because Wait will
close both sides of the pipe once the process ends without
any guarantees that all data has been read from the pipe.
To fix this, we allocate the os.Pipes ourselves and
manage cleanup ourselves when the process has ended.

The second race condition is because sshSession.run waits
upon exec.Cmd to finish and then immediately proceeds to call ss.Exit,
which will close all output streams going to the SSH client.
This may interrupt any asynchronous io.Copy still copying data.
To fix this, we close the write-side of the os.Pipes after
the process has finished (and before calling ss.Exit) and
synchronously wait for the io.Copy routines to finish.

Fixes #7601

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
Co-authored-by: Maisem Ali <maisem@tailscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@ianlancetaylor @bcmills @dsnet