code review 132220044: os/exec: Do not assume process is done after Wait() returns.

os/exec: Do not assume process is done after Wait() returns.

Currently the Wait() implementation assumes that once the Wait4() syscall returns, the process has exited. This may not always be the case, and this assumption prevents the process from being killed by `proc.Kill()`. In situations such as a process tracing another process using the Ptrace() family of syscalls, Wait() may be called several times during the life of the traced process. It is not safe to assume that the process has exited. This assumption must be validated before calling `p.setDone()`.

[dparker, 2014-08-27 00:07:10]

Hello golang-codereviews@googlegroups.com,

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

[iant, 2014-08-27 21:54:30]

Ideally the first line of the description should be

os/exec: do not assume process is done after Wait returns

(Put "do" in lower case, remove "()", remove period).

In the general description omit "()" after function names; Go generally doesn't
use them.

[iant, 2014-08-27 22:02:01]

https://codereview.appspot.com/132220044/diff/40001/src/pkg/os/exec/exec_unix...
File src/pkg/os/exec/exec_unix_test.go (right):

https://codereview.appspot.com/132220044/diff/40001/src/pkg/os/exec/exec_unix...
src/pkg/os/exec/exec_unix_test.go:9: cmd := helperCommand(t, "sleep", "10")
Don't you need to add "sleep" to TestHelperProcess?

https://codereview.appspot.com/132220044/diff/40001/src/pkg/os/exec/exec_unix...
src/pkg/os/exec/exec_unix_test.go:17: err =
syscall.PtraceAttach(cmd.Process.Pid)
syscall.PtraceAttach is only defined on darwin and linux, so this test needs a
+build to restrict it to those systems.

https://codereview.appspot.com/132220044/diff/40001/src/pkg/os/exec/exec_unix...
src/pkg/os/exec/exec_unix_test.go:22: _, err = cmd.Process.Wait()
This seems wrong.  The cmd.Process.Wait method is documented as waiting until
the process exits.  This test is assuming that it does not.  It seems to me that
you are fixing the wrong thing.

I can believe that when using syscall.PtraceAttach there ought to be some way to
get the process status, and it may be that the wait4 system call is the right
way, but cmd.Process.Wait should wait until the process exits.  If it doesn't do
that, then that is what we should fix.

[dparker, 2014-08-28 01:29:11]

On 2014/08/27 22:02:01, iant wrote:
>
https://codereview.appspot.com/132220044/diff/40001/src/pkg/os/exec/exec_unix...
> File src/pkg/os/exec/exec_unix_test.go (right):
> 
>
https://codereview.appspot.com/132220044/diff/40001/src/pkg/os/exec/exec_unix...
> src/pkg/os/exec/exec_unix_test.go:9: cmd := helperCommand(t, "sleep", "10")
> Don't you need to add "sleep" to TestHelperProcess?
> 
>
https://codereview.appspot.com/132220044/diff/40001/src/pkg/os/exec/exec_unix...
> src/pkg/os/exec/exec_unix_test.go:17: err =
> syscall.PtraceAttach(cmd.Process.Pid)
> syscall.PtraceAttach is only defined on darwin and linux, so this test needs a
> +build to restrict it to those systems.
> 
>
https://codereview.appspot.com/132220044/diff/40001/src/pkg/os/exec/exec_unix...
> src/pkg/os/exec/exec_unix_test.go:22: _, err = cmd.Process.Wait()
> This seems wrong.  The cmd.Process.Wait method is documented as waiting until
> the process exits.  This test is assuming that it does not.  It seems to me
that
> you are fixing the wrong thing.
> 
> I can believe that when using syscall.PtraceAttach there ought to be some way
to
> get the process status, and it may be that the wait4 system call is the right
> way, but cmd.Process.Wait should wait until the process exits.  If it doesn't
do
> that, then that is what we should fix.

You're right, that makes complete sense. I actually ended up replacing all calls
to process.Wait with syscall.Wait4 and achieved the same result in my program.
In light of what you've mentioned, and the docs concerning process.Wait(), I
still think there is a bug where process.Wait() should technically not return
when the process has not actually exited. I can work on a CL for this fix,
separate from this one.

[gobot, 2014-12-19 05:12:14]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/132220044 is best) in the description in your
new CL.

Thanks very much.