code review 7392048: os: make FindProcess do some error checking

os: make FindProcess do some error checking

Makes FindProcess on UNIX and Plan 9 check that the
process with the given pid is running and that we
at least have permissions to send it signals. An
equivalent check is already present in the Windows
implementation.

[akumar, 2013-02-22 23:21:46]

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

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

[ality, 2013-02-23 04:32:49]

I don't agree with this change. The syscall
API is not required to be uniform across all
supported systems.

I'd prefer to go the other way. Remove any
hint of Unix from the Plan 9 syscall API
and remove these assumptions from higher
level code.

It will help keep us honest.

  Anthony

[dave_cheney.net, 2013-02-23 04:34:32]

I agree. Can we try that first ?

On Sat, Feb 23, 2013 at 3:32 PM, Anthony Martin <ality@pbrane.org> wrote:
> I don't agree with this change. The syscall
> API is not required to be uniform across all
> supported systems.
>
> I'd prefer to go the other way. Remove any
> hint of Unix from the Plan 9 syscall API
> and remove these assumptions from higher
> level code.
>
> It will help keep us honest.
>
>   Anthony
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[rminnich, 2013-02-23 04:46:08]

On Fri, Feb 22, 2013 at 8:34 PM, Dave Cheney <dave@cheney.net> wrote:
> I agree. Can we try that first ?

Sure, what would you suggest?

ron

[dave_cheney.net, 2013-02-23 04:46:49]

I was echoing Anthony, "I'd prefer to go the other way. Remove any
hint of Unix from the Plan 9 syscall API
and remove these assumptions from higher
level code."

On Sat, Feb 23, 2013 at 3:46 PM, ron minnich <rminnich@gmail.com> wrote:
> On Fri, Feb 22, 2013 at 8:34 PM, Dave Cheney <dave@cheney.net> wrote:
>> I agree. Can we try that first ?
>
> Sure, what would you suggest?
>
> ron

[rminnich, 2013-02-23 04:50:06]

On Fri, Feb 22, 2013 at 8:46 PM, Dave Cheney <dave@cheney.net> wrote:
> I was echoing Anthony, "I'd prefer to go the other way. Remove any
> hint of Unix from the Plan 9 syscall API
> and remove these assumptions from higher
> level code."

Sure, do either of you have a suggestion, on how to do this? Because
we took a pretty good look and nothing came up that didn't look like
it would impact a lot of existing runtime, which seemed unlikely to be
acceptable.

In other words, doing it this way did not happen without some
discussion. That's why I'm looking for suggestions. We ran out of
ideas.

ron

[ality, 2013-02-23 05:20:32]

Well, the net/http/cgi test is calling p.Signal
with a syscall.Signal(0) which means no signal
will actually be sent. That portion of the test
is just trying to determine if the child proc is
running.

There's two options: either make the test stat(2)
/proc/p.Pid on Plan 9 or move the logic into both
the Unix and Plan 9 versions of os.findProcess.

  Anthony

[rminnich, 2013-02-23 07:10:52]

OK, we'll see about that, seems like a nice idea.

On Fri, Feb 22, 2013 at 9:20 PM, Anthony Martin <ality@pbrane.org> wrote:
> Well, the net/http/cgi test is calling p.Signal
> with a syscall.Signal(0) which means no signal
> will actually be sent. That portion of the test
> is just trying to determine if the child proc is
> running.
>
> There's two options: either make the test stat(2)
> /proc/p.Pid on Plan 9 or move the logic into both
> the Unix and Plan 9 versions of os.findProcess.
>
>   Anthony

[akumar, 2013-02-23 14:02:52]

This is an update of the "syscall: Plan9: Define the Signal
type" CL.

PTAL.

Also updated the description.

[minux1, 2013-02-23 14:13:14]

https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#new...
src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err error) {
this is an API change.

the old findProcess doesn't give an error if we don't have access
to the process.

[ality, 2013-02-23 14:21:41]

minux.ma@gmail.com once said:
>
https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#new...
> src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err
> error) {
> this is an API change.
> 
> the old findProcess doesn't give an error if we don't have access
> to the process.

It does on Windows.

  Anthony

[rminnich, 2013-02-23 17:26:27]

https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#new...
src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err error) {
That seems to beg the question. Would you not want it to error if it didn't have
access? In what way does this test hurt? Does this change break something or are
you concerned that it might break something we don't understand?

[bradfitz, 2013-02-23 17:46:29]

On Fri, Feb 22, 2013 at 9:20 PM, Anthony Martin <ality@pbrane.org> wrote:

> Well, the net/http/cgi test is calling p.Signal
> with a syscall.Signal(0) which means no signal
> will actually be sent. That portion of the test
> is just trying to determine if the child proc is
> running.
>
> There's two options: either make the test stat(2)
> /proc/p.Pid on Plan 9 or move the logic into both
> the Unix and Plan 9 versions of os.findProcess.
>

Either sounds acceptable.  We don't have to leave the cgi test as it is.

[akumar, 2013-02-23 19:43:50]

This CL is the implementation of the second option.
The test has been changed.

On 23 February 2013 09:46, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>
> On Fri, Feb 22, 2013 at 9:20 PM, Anthony Martin <ality@pbrane.org> wrote:
>>
>> Well, the net/http/cgi test is calling p.Signal
>> with a syscall.Signal(0) which means no signal
>> will actually be sent. That portion of the test
>> is just trying to determine if the child proc is
>> running.
>>
>> There's two options: either make the test stat(2)
>> /proc/p.Pid on Plan 9 or move the logic into both
>> the Unix and Plan 9 versions of os.findProcess.
>
>
> Either sounds acceptable.  We don't have to leave the cgi test as it is.

[bradfitz, 2013-02-23 19:53:10]

I see both views here:

a) FindProcess on unix never did anything before, and never returned an
error. Keep doing the same?
b) but Windows did, and our docs make it sound like Unix could. So might as
well, if it helps?

Alternatively, we could add a new portable pkg/os Signal variable defined
to the "zero signal" or "check alive signal", for use with
(*os.Process).Signal.

But that's kinda gross.  Better would be just to define a new method on
*os.Process, like:

    // Exists returns whether the process p still exists
    func (p *Process) Exists() bool

Or something.

For now to get unblocked you can just plan9-vs-everybody-else stuff with
+build files for the cgi test.  Or are there things besides the cgi test
that need to know whether a process is still running?


On Sat, Feb 23, 2013 at 9:26 AM, <rminnich@gmail.com> wrote:

>
> https://codereview.appspot.**com/7392048/diff/12001/src/**
>
pkg/os/exec_unix.go<https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go>
> File src/pkg/os/exec_unix.go (right):
>
> https://codereview.appspot.**com/7392048/diff/12001/src/**
>
pkg/os/exec_unix.go#newcode59<https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#newcode59>
> src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err
> error) {
> That seems to beg the question. Would you not want it to error if it
> didn't have access? In what way does this test hurt? Does this change
> break something or are you concerned that it might break something we
> don't understand?
>
>
https://codereview.appspot.**com/7392048/<https://codereview.appspot.com/7392...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[akumar, 2013-02-23 20:16:15]

Sorry, I sent the message below privately to Brad on
accident - it was meant for general discussion.

--

I prefer (a) as is implemented here, since the documentation for
FindProcess says what you would want Exists to do. Moreover,
the Windows implementation already does all of this. If one
wants to simply create a new Process struct, they can do that
without the help of an API. To me, it seems that the current
implementation is inconsistent with documentation.

On 23 February 2013 11:53, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I see both views here:
>
> a) FindProcess on unix never did anything before, and never returned an
> error. Keep doing the same?
> b) but Windows did, and our docs make it sound like Unix could. So might as
> well, if it helps?
>
> Alternatively, we could add a new portable pkg/os Signal variable defined to
> the "zero signal" or "check alive signal", for use with
> (*os.Process).Signal.
>
> But that's kinda gross.  Better would be just to define a new method on
> *os.Process, like:
>
>     // Exists returns whether the process p still exists
>     func (p *Process) Exists() bool
>
> Or something.
>
> For now to get unblocked you can just plan9-vs-everybody-else stuff with
> +build files for the cgi test.  Or are there things besides the cgi test
> that need to know whether a process is still running?
>
>
> On Sat, Feb 23, 2013 at 9:26 AM, <rminnich@gmail.com> wrote:
>>
>>
>> https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go
>> File src/pkg/os/exec_unix.go (right):
>>
>>
>>
https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#new...
>> src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err
>> error) {
>> That seems to beg the question. Would you not want it to error if it
>> didn't have access? In what way does this test hurt? Does this change
>> break something or are you concerned that it might break something we
>> don't understand?
>>
>> https://codereview.appspot.com/7392048/
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>>
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>>
>

[minux1, 2013-02-23 21:05:23]

On Sun, Feb 24, 2013 at 4:15 AM, Akshat Kumar <seed@mail.nanosouffle.net>wrote:

> I prefer (a) as is implemented here, since the documentation for
> FindProcess says what you would want Exists to do. Moreover,
> the Windows implementation already does all of this. If one
> wants to simply create a new Process struct, they can do that
> without the help of an API. To me, it seems that the current
> implementation is inconsistent with documentation.

right, the docs says "running process", however, that doesn't mean a process
the caller can send signal to (i.e. with matching effective or real uid or
the user
has special privileges): on unix, there is no easy and portable way to
detect
whether a pid is really running (when not using /proc), so that's why i
think the
current code doesn't verify the validity of the pid argument.

[dave_cheney.net, 2013-02-24 01:34:56]

I think this solution is racy. wrt. the windows impl, it might racy as well, I
don't know enough about windows.

I suggest deferring changing the behavior of os.FindProcess and following Brad's
suggestion to make a plan version of this test.

https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#new...
src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err error) {
On 2013/02/23 14:13:14, minux wrote:
> this is an API change.
> 
> the old findProcess doesn't give an error if we don't have access
> to the process.

Is it also racy ? I thought the only pid you could count on not to change in
unix was your own.

[akumar, 2013-02-25 21:03:24]

On 24 February 2013 01:34,  <dave@cheney.net> wrote:
> I think this solution is racy. wrt. the windows impl, it might racy as
> well, I don't know enough about windows.

I think any solution to the problem would be racy, even just by
the fact that the process may exit at any time (of course, many
other things might happen as well -- we have no control here).

> I suggest deferring changing the behavior of os.FindProcess and
> following Brad's suggestion to make a plan version of this test.

Done. See CL 7377053.


> on unix, there is no easy and portable way to detect
> whether a pid is really running (when not using /proc), so that's why i
> think the current code doesn't verify the validity of the pid argument.

Would you be happier if I just checked for existence of
the pid's corresponding directory in /proc?

[ioe, 2013-04-20 23:23:03]

On 2013/02/23 21:05:23, minux wrote:
> right, the docs says "running process", however, that doesn't mean a process
> the caller can send signal to (i.e. with matching effective or real uid or
> the user
> has special privileges): on unix, there is no easy and portable way to
> detect
> whether a pid is really running (when not using /proc), so that's why i
> think the
> current code doesn't verify the validity of the pid argument.

There is a easy and portable way for unixes implemented in about every program
doing pid file handling:

a) Does a process with PID (where pid > 0) exist (or did it die already)?
- kill(pid, 0) does NOT return -1 and sets errno=ESRCH -> yes
- every other (return, errno) tuple: -> no
- documented behavior of (OpenBSD, NetBSD, FreeBSD, Linux)
b) Can I signal process PID?
- kill(pid, 0) does return 0 -> yes
- other return values -> no
- is racy, if you mean a specific program with this pid and your pid source is
old, but that is just a property of pids.
- documented behavior of (OpenBSD, NetBSD, FreeBSD, Linux)

Do you have any other currently supported Unix in mind?

I would like to have FindProcess return situation a)
Process.Signal method could check for situation b)

I would like to write code like:

if _, err := os.FindProcess(pid); err != nil {
        os.Remove(STALE_PIDFILE)
}

This would be working Go code on ALL supported operating systems, if the
abstraction in the Unix case wasn't broken.

Even a panic("Not implemented") Unix implementation of os.FindProcess (honestly
saying that it actually isn't implemented for Unix)
would have saved me a few hours. The current Unix implementation is completely
useless.

Siince you have to handle the return code of Process.Signal() and Process.Kill()
anyway, 
it can return EPERM (killability) an ESRCH (process gone between find and
signal/kill) too.

[rsc, 2013-12-18 18:27:34]

R=close (timed out)