os/exec: race on cmd.Wait() might lead to panic

[empijei]

The current implementation of cmd.Wait has a race condition: if multiple goroutines call it, it might cause a panic.

In the first part of the method, copied below, two concurrent calls might check c.finished, get false, set it to true, invoke c.Process.Wait() and close c.waitDone before any error checking is performed. c.waitDone is a chan struct{}, and a double close will cause a panic.

func (c *Cmd) Wait() error {
	if c.Process == nil {
		return errors.New("exec: not started")
	}
	if c.finished {
		return errors.New("exec: Wait was already called")
	}
	c.finished = true

	state, err := c.Process.Wait()
	if c.waitDone != nil {
		close(c.waitDone)
	}
	//[...]

Since waiting is a synchronization primitive I'd expect one of the two:

• The documentation should state that this is not safe for concurrent use (probably the best approach here)
• Some form of synchronization to prevent races. I would either atomically CAS c.finished (but I'm not a fan of atomics and would require to change type to some sort of int) or protect c with a mutex, which would be my suggested solution for this case

I would happily send in CLs in both cases.

[mvdan]

I'm unclear as to whether every standard library function that isn't safe for concurrent use needs to be documented as such. For this particular case it seems fine to me, as it wasn't obvious by reading the godoc, and I can imagine someone making the mistake.

/cc @bradfitz @ianlancetaylor as per https://dev.golang.org/owners/ for a decision.

[bradfitz]

In general we only document the abnormal cases. The assumption when unstated is that things are not safe for concurrent use, that zero values are not usable, that implementations implement interfaces faithfully, and that only one return values is non-zero and meaningful.

That said, in this case I agree it would be a reasonable assumption for users to think that two concurrent Wait calls are valid, so I wouldn't object to docs or a fix here to allow concurrent calls.

[ianlancetaylor]

Fixing this to fully permit concurrent wait calls would require us to save the *ProcessState value, so let's not do that. But I think it would be nice to fix it so that all but one of the calls returns an error, if possible. We may be able to rely on the kernel returning ECHILD for all but the first call that it sees.

[bradfitz]

Fixing this to fully permit concurrent wait calls would require us to save the *ProcessState value, so let's not do that.

Why do you dismiss that option? Just curious. It's not a huge struct, and the lifetime of *exec.Cmd isn't typically long. It's not usually retained anywhere.

[ianlancetaylor]

It just doesn't feel like the right approach to me. I'm open to counter-argument.

[empijei]

Thanks @bradfitz for clarifying assumptions, I just pointed this out as this might be a peculiar case due to the synchronizing nature of the Wait method.

One more question: should the doc change be on cmd (specifying that no concurrent calls to its methods are safe) or just on Wait?

About fixing it I agree with @ianlancetaylor. I think that external processes should be treated as "outside of go boundaries". More specifically: if synchronization or orchestration is needed w.r.t. an external process, interactions with the process should be proxied by a goroutine. It would then use channels and other primitives in an idiomatic way. I wouldn't like to read some Go code using external processes and wait on them instead of a sync.WaitGroup or reading from a chan.

[bcmills]

Fixing this to fully permit concurrent wait calls would require us to save the *ProcessState value

I think I'm missing something: don't we already save the *ProcessState in the Cmd.ProcessState field?

[ianlancetaylor]

@bcmills In this issue we're talking about the os package, not the os/exec package.

[bcmills]

I'm still not sure that I follow. As @empijei notes, it seems like we could address the reported race with a mutex in *exec.Cmd, without touching the os package itself. I'm envisioning something like:

func (c *Cmd) Wait() error {
	if c.Process == nil {
		return errors.New("exec: not started")
	}

	c.waitMu.Lock()
	defer c.waitMu.Unlock()

	if c.waitErr != nil {
		return c.waitErr
	}

	if c.ProcessState == nil {
		if c.finished {
			return errors.New("exec: ProcessState is nil after Wait was called")
		}
		c.ProcessState, c.waitErr := c.Process.Wait()
		if c.waitDone != nil {
			close(c.waitDone)
		}
		for range c.goroutine {
			if err := <-c.errch; err != nil && c.waitErr == nil && state.Success() {
				c.waitErr = err
			}
		}
		c.closeDescriptors(c.closeAfterWait)
		c.finished = true
	}

	if c.waitErr == nil && !c.ProcessState.Success() {
		return &ExitError{ProcessState: c.ProcessState}
	}
	return c.waitErr
}

[ianlancetaylor]

I suppose that I have mentally transmuted this issue to applying to os.(*Process).Wait rather than os/exec.(*Cmd).Wait. You're right that the original report is about os/exec, but it still sort of seems to me that we should fix the problem in the os package rather than only in the os/exec package.

[bcmills]

Maybe, although I think there is a qualitative difference: calling Wait manually on a *os.Process obtained from FindProcess or StartProcess seems like a power-user feature, whereas I see (*exec.Cmd).Wait pretty frequently in ordinary application code.

[ianlancetaylor]

Sure, agreed, but presumably if we fix the os package we will get the os/exec fix for free. So, why not?

[bcmills]

I don't think we can get the os/exec fix for free: even if we make c.Process.Wait safe to call concurrently, we'll still need to synchronize access to fields such as c.ProcessState, c.finished, and c.waitDone.

And then there is the question of whether Wait should be idempotent or should return an error for all but one call. You suggested the latter (in #28461 (comment)), but I would expect a method that allows concurrent (not just repeated) Wait calls to return the same result to all callers, just as (*sync.WaitGroup).Wait allows multiple goroutines to wait on the same set of events.