os: fix a race between Process.signal() and wait() on Windows

Process.handle was accessed without synchronization while wait() and
signal() could be called concurrently.

A first solution was to add a Mutex in Process but it was probably too
invasive given Process.handle is only used on Windows.

This version uses atomic operations to read the handle value. There is
still a race between isDone() and the value of the handle, but it only
leads to slightly incorrect error codes. The caller may get a:

  errors.New("os: process already finished")

instead of:

  syscall.EINVAL

which sounds harmless.

Fixes #9382

Change-Id: Iefcc687a1166d5961c8f27154647b9b15a0f748a
Reviewed-on: https://go-review.googlesource.com/9904
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/os/exec.go b/src/os/exec.go
index 5aea3098b542354d27630d1cc51b66d6efd5c2fe..15e95b917223e603d305d10ec4c8d2863509c3ce 100644 (file)
--- a/src/os/exec.go
+++ b/src/os/exec.go
@@ -13,8 +13,8 @@ import (
 // Process stores the information about a process created by StartProcess.
 type Process struct {
        Pid    int
-       handle uintptr
-       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
 }
 
 func newProcess(pid int, handle uintptr) *Process {

diff --git a/src/os/exec_windows.go b/src/os/exec_windows.go
index 393393b2375a381aa39e44c129acf851e500bc68..3264271b2e95b6682c754dd51911d0a8d420db5c 100644 (file)
--- a/src/os/exec_windows.go
+++ b/src/os/exec_windows.go
@@ -7,13 +7,15 @@ package os
 import (
        "errors"
        "runtime"
+       "sync/atomic"
        "syscall"
        "time"
        "unsafe"
 )
 
 func (p *Process) wait() (ps *ProcessState, err error) {
-       s, e := syscall.WaitForSingleObject(syscall.Handle(p.handle), syscall.INFINITE)
+       handle := atomic.LoadUintptr(&p.handle)
+       s, e := syscall.WaitForSingleObject(syscall.Handle(handle), syscall.INFINITE)
        switch s {
        case syscall.WAIT_OBJECT_0:
                break
@@ -23,12 +25,12 @@ func (p *Process) wait() (ps *ProcessState, err error) {
                return nil, errors.New("os: unexpected result from WaitForSingleObject")
        }
        var ec uint32
-       e = syscall.GetExitCodeProcess(syscall.Handle(p.handle), &ec)
+       e = syscall.GetExitCodeProcess(syscall.Handle(handle), &ec)
        if e != nil {
                return nil, NewSyscallError("GetExitCodeProcess", e)
        }
        var u syscall.Rusage
-       e = syscall.GetProcessTimes(syscall.Handle(p.handle), &u.CreationTime, &u.ExitTime, &u.KernelTime, &u.UserTime)
+       e = syscall.GetProcessTimes(syscall.Handle(handle), &u.CreationTime, &u.ExitTime, &u.KernelTime, &u.UserTime)
        if e != nil {
                return nil, NewSyscallError("GetProcessTimes", e)
        }
@@ -53,7 +55,8 @@ func terminateProcess(pid, exitcode int) error {
 }
 
 func (p *Process) signal(sig Signal) error {
-       if p.handle == uintptr(syscall.InvalidHandle) {
+       handle := atomic.LoadUintptr(&p.handle)
+       if handle == uintptr(syscall.InvalidHandle) {
                return syscall.EINVAL
        }
        if p.done() {
@@ -67,14 +70,15 @@ func (p *Process) signal(sig Signal) error {
 }
 
 func (p *Process) release() error {
-       if p.handle == uintptr(syscall.InvalidHandle) {
+       handle := atomic.LoadUintptr(&p.handle)
+       if handle == uintptr(syscall.InvalidHandle) {
                return syscall.EINVAL
        }
-       e := syscall.CloseHandle(syscall.Handle(p.handle))
+       e := syscall.CloseHandle(syscall.Handle(handle))
        if e != nil {
                return NewSyscallError("CloseHandle", e)
        }
-       p.handle = uintptr(syscall.InvalidHandle)
+       atomic.StoreUintptr(&p.handle, uintptr(syscall.InvalidHandle))
        // no need for a finalizer anymore
        runtime.SetFinalizer(p, nil)
        return nil