os: on GNU/Linux use waitid to avoid wait/kill race

On systems that support the POSIX.1-2008 waitid function, we can use it
to block until a wait will succeed. This avoids a possible race
condition: if a program calls p.Kill/p.Signal and p.Wait from two
different goroutines, then it is possible for the wait to complete just
before the signal is sent. In that case, it is possible that the system
will start a new process using the same PID between the wait and the
signal, causing the signal to be sent to the wrong process. The
Process.isdone field attempts to avoid that race, but there is a small
gap of time between when wait returns and isdone is set when the race
can occur.

This CL avoids that race by using waitid to wait until the process has
exited without actually collecting the PID. Then it sets isdone, then
waits for any active signals to complete, and only then collects the PID.

No test because any plausible test would require starting enough
processes to recycle all the process IDs.

Update #13987.
Update #16028.

Change-Id: Id2939431991d3b355dfb22f08793585fc0568ce8
Reviewed-on: https://go-review.googlesource.com/23967
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/os/exec.go b/src/os/exec.go
index 239fd9288897e68545aaa39c69a8cc0e8983fced..bf3249864d33e3f847835d94010e8f0d015fd0e5 100644 (file)
--- a/src/os/exec.go
+++ b/src/os/exec.go
@@ -6,6 +6,7 @@ package os
 
 import (
        "runtime"
+       "sync"
        "sync/atomic"
        "syscall"
 )
@@ -13,8 +14,9 @@ import (
 // Process stores the information about a process created by StartProcess.
 type Process struct {
        Pid    int
-       handle uintptr // handle is accessed atomically on Windows
-       isdone uint32  // process has been successfully waited on, non zero if true
+       handle uintptr      // handle is accessed atomically on Windows
+       isdone uint32       // process has been successfully waited on, non zero if true
+       sigMu  sync.RWMutex // avoid race between wait and signal
 }
 
 func newProcess(pid int, handle uintptr) *Process {

diff --git a/src/os/exec_unix.go b/src/os/exec_unix.go
index ed97f85e22ff3d8336549f13d9726cc2cb4f4121..c4999db57f09051e9111199f429018cdf3871509 100644 (file)
--- a/src/os/exec_unix.go
+++ b/src/os/exec_unix.go
@@ -17,6 +17,22 @@ func (p *Process) wait() (ps *ProcessState, err error) {
        if p.Pid == -1 {
                return nil, syscall.EINVAL
        }
+
+       // If we can block until Wait4 will succeed immediately, do so.
+       ready, err := p.blockUntilWaitable()
+       if err != nil {
+               return nil, err
+       }
+       if ready {
+               // Mark the process done now, before the call to Wait4,
+               // so that Process.signal will not send a signal.
+               p.setDone()
+               // Acquire a write lock on sigMu to wait for any
+               // active call to the signal method to complete.
+               p.sigMu.Lock()
+               p.sigMu.Unlock()
+       }
+
        var status syscall.WaitStatus
        var rusage syscall.Rusage
        pid1, e := syscall.Wait4(p.Pid, &status, 0, &rusage)
@@ -43,6 +59,8 @@ func (p *Process) signal(sig Signal) error {
        if p.Pid == 0 {
                return errors.New("os: process not initialized")
        }
+       p.sigMu.RLock()
+       defer p.sigMu.RUnlock()
        if p.done() {
                return errFinished
        }

diff --git a/src/os/wait_linux.go b/src/os/wait_linux.go
new file mode 100644 (file)
index 0000000..7707539
--- /dev/null
+++ b/src/os/wait_linux.go
@@ -0,0 +1,29 @@
+// Copyright 2016 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package os
+
+import (
+       "runtime"
+       "syscall"
+       "unsafe"
+)
+
+const _P_PID = 1
+
+// blockUntilWaitable attempts to block until a call to p.Wait will
+// succeed immediately, and returns whether it has done so.
+// It does not actually call p.Wait.
+func (p *Process) blockUntilWaitable() (bool, error) {
+       // waitid expects a pointer to a siginfo_t, which is 128 bytes
+       // on all systems. We don't care about the values it returns.
+       var siginfo [128]byte
+       psig := &siginfo[0]
+       _, _, e := syscall.Syscall6(syscall.SYS_WAITID, _P_PID, uintptr(p.Pid), uintptr(unsafe.Pointer(psig)), syscall.WEXITED|syscall.WNOWAIT, 0, 0)
+       runtime.KeepAlive(psig)
+       if e != 0 {
+               return false, NewSyscallError("waitid", e)
+       }
+       return true, nil
+}

diff --git a/src/os/wait_unimp.go b/src/os/wait_unimp.go
new file mode 100644 (file)
index 0000000..802b032
--- /dev/null
+++ b/src/os/wait_unimp.go
@@ -0,0 +1,16 @@
+// Copyright 2016 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build darwin dragonfly freebsd nacl netbsd openbsd solaris
+
+package os
+
+// blockUntilWaitable attempts to block until a call to p.Wait will
+// succeed immediately, and returns whether it has done so.
+// It does not actually call p.Wait.
+// This version is used on systems that do not implement waitid,
+// or where we have not implemented it yet.
+func (p *Process) blockUntilWaitable() (bool, error) {
+       return false, nil
+}