os/exec: race between Wait and ctx.Done

[ianlancetaylor]

There is an unlikely race condition when using os/exec.CommandContext. In Cmd.Wait, one goroutine sleeps in a select on ctx.Done and waitDone, while another goroutine sleeps in c.Process.Wait. If the process terminates at the same time as the context, then it is possible for the first goroutine to wake up and receive from ctx.Done and call c.Process.Kill just as the code in os.Process.Wait returns from syscall.Wait4. At that point the process ID is available for reuse, and a new process may be started with the same process ID. The call to c.Process.Kill will call c.Process.Signal. Because Process.wait has not yet had a chance to call Process.setDone, c.Process.Signal will call syscall.Kill with the old process ID. This could potentially kill an unrelated process.

This is similar to the problem described in #13987; the race described there has now been brought into the standard library.

Marking as 1.8 because I don't see a simple way to fix this.

[cespare]

Would it be better to remove CommandContext for this release if we cannot ship a fixed version?

[ianlancetaylor]

Here is a complex way to fix the problem. Add the following internal API to the runtime package.

// sigchldCount returns the number of SIGCHLD signals received since the process started.
runtime.sigchldCount() int
// waitForSIGCHLD sleeps until more than c SIGCHLD signals have been received.
runtime.waitForSIGCHLD(c int)

With those functions, the code in os.Process.wait can be written as

for {
    c := runtime.sigchldCount()
    pid1, e := syscall.Wait4(p.Pid, &status, syscall.WNOHANG, &rusage)
    if e != nil {
        return nil, NewSyscallError("wait", e)
    }
    if pid1 != 0 {
        break
    }
    runtime.waitForSIGCHLD(c)
}
p.setDone()

[ianlancetaylor]

@cespare I'm inclined to leave it in. It's a very unlikely race. Not only do the process and the context have to end at exactly the same time quantum, but the process ID has to be reused within that same time quantum as well.

Happy to hear other opinions, though.

[ianlancetaylor]

I think I see how to do this on systems that support the POSIX.1-2008 waitid function.

[gopherbot]

CL https://golang.org/cl/23967 mentions this issue.

[ianlancetaylor]

This is now fixed on GNU/Linux.

Since the fix was to the os package, not the os/exec package, I'm now declaring this issue a dup of #13987.

[gopherbot]

CL https://golang.org/cl/24021 mentions this issue.

[gopherbot]

CL https://golang.org/cl/24020 mentions this issue.