From d574b59fc71e52ac7feead94348ee61ef8d16a1c Mon Sep 17 00:00:00 2001 From: Patrick Mezard Date: Sun, 10 May 2015 15:35:52 +0200 Subject: [PATCH] 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 Run-TryBot: Alex Brainman TryBot-Result: Gobot Gobot --- src/os/exec.go | 4 ++-- src/os/exec_windows.go | 18 +++++++++++------- 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 -- 2.48.1