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>

2 files changed, 13 insertions, 9 deletions

diff --git a/src/os/exec.go b/src/os/exec.go
index 5aea3098b5..15e95b9172 100644
--- 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 393393b237..3264271b2e 100644
--- 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