code review 7128059: syscall: fix fork-exec/wait inconsistencies for Plan 9

syscall: fix fork-exec/wait inconsistencies for Plan 9

Fixes the fork-exec/wait race condition for ForkExec
as well, by making it use startProcess. This makes the
comment for StartProcess consistent as well.

Further, the passing of Waitmsg data in startProcess
and WaitProcess is protected against possible forks
from outside of ForkExec and StartProcess, which might
cause interference with the Await call.

[akumar, 2013-01-18 23:05:33]

Hello rsc@golang.org, rminnich@gmail.com, npe@plan9.bell-labs.com (cc:
ality@pbrane.org, golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[ality, 2013-01-20 09:56:53]

https://codereview.appspot.com/7128059/diff/3002/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

https://codereview.appspot.com/7128059/diff/3002/src/pkg/syscall/exec_plan9.g...
src/pkg/syscall/exec_plan9.go:550: if procs.waits[w.Pid] != nil {
I don't understand why this is necessary.
We are the parent of a single child. How
can w.Pid ever be different from ret.pid?

The only way is if someone does a RawSyscall
amounting to rfork(RFPROC) in whatever g last
ran on this m. That is a really bad idea and
they deserve the consequences. :P

We should simply document that you must use
ForkExec instead. This should be familiar to
Plan 9 users since it's exactly the same for
the thread(2) library.

https://codereview.appspot.com/7128059/diff/3002/src/pkg/syscall/exec_plan9.g...
src/pkg/syscall/exec_plan9.go:628: if procs.waits[wmsg.Pid] == ch {
If pid != wmsg.Pid we'll return a Waitmsg that doesn't
correspond to pid. That breaks the (*os.Process).Wait
contract.

[akumar, 2013-01-22 03:47:19]

PTAL.

If the feeling is still that the for-loop is unnecessary, I'll withdraw it in
another update.

https://codereview.appspot.com/7128059/diff/3002/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

https://codereview.appspot.com/7128059/diff/3002/src/pkg/syscall/exec_plan9.g...
src/pkg/syscall/exec_plan9.go:550: if procs.waits[w.Pid] != nil {
On 2013/01/20 09:56:53, ality wrote:
> I don't understand why this is necessary.
> We are the parent of a single child. How
> can w.Pid ever be different from ret.pid?
> 
> The only way is if someone does a RawSyscall
> amounting to rfork(RFPROC) in whatever g last
> ran on this m. That is a really bad idea and
> they deserve the consequences. :P
> 
> We should simply document that you must use
> ForkExec instead. This should be familiar to
> Plan 9 users since it's exactly the same for
> the thread(2) library.

Indeed, it should be documented that ForkExec is preferred, and with the changes
to ForkExec and Exec proposed here, this change becomes unnecessary. However, I
think that in the case that proper and improper methods occur, the proper usage
should be robust and not be vulnerable to interference (upholding the
(*os.Process).Wait contract, as you mention below). So I've updated this to
instead wait for the Waitmsg for ret.pid and drop anything else until then.

https://codereview.appspot.com/7128059/diff/3002/src/pkg/syscall/exec_plan9.g...
src/pkg/syscall/exec_plan9.go:628: if procs.waits[wmsg.Pid] == ch {
On 2013/01/20 09:56:53, ality wrote:
> If pid != wmsg.Pid we'll return a Waitmsg that doesn't
> correspond to pid. That breaks the (*os.Process).Wait
> contract.

As long as startProcess is making sure that the Waitmsg data is being directed
properly, we don't have to worry here, since all the data is going over channels
as far as WaitProcess is concerned.

Also, on second thought, this change and the one on the next line are not
needed.

[rsc, 2013-01-23 03:25:22]

The loop is not really necessary anymore with ForkExec calling startProcess, but
you can keep it if you want. It would only guard against people calling
syscall.RawSyscall directly.

https://codereview.appspot.com/7128059/diff/9001/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

https://codereview.appspot.com/7128059/diff/9001/src/pkg/syscall/exec_plan9.g...
src/pkg/syscall/exec_plan9.go:549: for w.Pid != ret.pid {
for w.err == nil && w.Pid != ret.pid {

https://codereview.appspot.com/7128059/diff/9001/src/pkg/syscall/exec_plan9.g...
src/pkg/syscall/exec_plan9.go:573: r1, _, _ := RawSyscall(SYS_RFORK,
RFCENVG|RFNOWAIT, 0, 0)
RFNOWAIT is meaningless here, since RFPROC has not been specified. There's no
new process, so nothing to wait for.

[akumar, 2013-01-23 03:40:54]

PTAL.

Also updated the description.

https://codereview.appspot.com/7128059/diff/9001/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

https://codereview.appspot.com/7128059/diff/9001/src/pkg/syscall/exec_plan9.g...
src/pkg/syscall/exec_plan9.go:549: for w.Pid != ret.pid {
On 2013/01/23 03:25:22, rsc wrote:
> for w.err == nil && w.Pid != ret.pid {

Done.

https://codereview.appspot.com/7128059/diff/9001/src/pkg/syscall/exec_plan9.g...
src/pkg/syscall/exec_plan9.go:573: r1, _, _ := RawSyscall(SYS_RFORK,
RFCENVG|RFNOWAIT, 0, 0)
On 2013/01/23 03:25:22, rsc wrote:
> RFNOWAIT is meaningless here, since RFPROC has not been specified. There's no
> new process, so nothing to wait for.

Done.

[rsc, 2013-01-23 03:41:40]

LGTM

[rsc, 2013-01-23 03:42:49]

*** Submitted as https://code.google.com/p/go/source/detail?r=5b5399bc3335 ***

syscall: fix fork-exec/wait inconsistencies for Plan 9

Fixes the fork-exec/wait race condition for ForkExec
as well, by making it use startProcess. This makes the
comment for StartProcess consistent as well.

Further, the passing of Waitmsg data in startProcess
and WaitProcess is protected against possible forks
from outside of ForkExec and StartProcess, which might
cause interference with the Await call.

R=rsc, rminnich, npe, ality
CC=golang-dev
https://codereview.appspot.com/7128059

Committer: Russ Cox <rsc@golang.org>