diff options
author | Austin Clements <austin@google.com> | 2019-11-18 20:21:38 -0500 |
---|---|---|
committer | Austin Clements <austin@google.com> | 2019-11-20 17:13:48 +0000 |
commit | 6fd467ee29226bf4b875921b7cb3b692c9db52ef (patch) | |
tree | b808cbf92d357cc258fafc66a3954c44bdc34008 /src/runtime/os_windows.go | |
parent | 91bb1d734ecb44f687f3549ace10ee8f70e977a8 (diff) | |
download | go-6fd467ee29226bf4b875921b7cb3b692c9db52ef.tar.gz go-6fd467ee29226bf4b875921b7cb3b692c9db52ef.zip |
runtime: ensure thread handle is valid in profileloop1
On Windows, there is currently a race between unminit closing the
thread's handle and profileloop1 suspending the thread using its
handle. If another handle reuses the same handle value, this can lead
to unpredictable results.
To fix this, we protect the thread handle with a lock and duplicate it
under this lock in profileloop1 before using it.
This is going to become a much bigger problem with non-cooperative
preemption (#10958, #24543), which uses the same basic mechanism as
profileloop1.
Change-Id: I9d62b83051df8c03f3363344438e37781a69ce16
Reviewed-on: https://go-review.googlesource.com/c/go/+/207779
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Diffstat (limited to 'src/runtime/os_windows.go')
-rw-r--r-- | src/runtime/os_windows.go | 34 |
1 files changed, 25 insertions, 9 deletions
diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go index b4df08322c..00f4c6ec28 100644 --- a/src/runtime/os_windows.go +++ b/src/runtime/os_windows.go @@ -143,7 +143,8 @@ func tstart_stdcall(newm *m) func ctrlhandler() type mOS struct { - thread uintptr // thread handle; accessed atomically + threadLock mutex // protects "thread" and prevents closing + thread uintptr // thread handle waitsema uintptr // semaphore for parking on locks resumesema uintptr // semaphore to indicate suspend/resume @@ -814,7 +815,11 @@ func sigblock() { func minit() { var thandle uintptr stdcall7(_DuplicateHandle, currentProcess, currentThread, currentProcess, uintptr(unsafe.Pointer(&thandle)), 0, 0, _DUPLICATE_SAME_ACCESS) - atomic.Storeuintptr(&getg().m.thread, thandle) + + mp := getg().m + lock(&mp.threadLock) + mp.thread = thandle + unlock(&mp.threadLock) // Query the true stack base from the OS. Currently we're // running on a small assumed stack. @@ -847,9 +852,11 @@ func minit() { // Called from dropm to undo the effect of an minit. //go:nosplit func unminit() { - tp := &getg().m.thread - stdcall1(_CloseHandle, *tp) - *tp = 0 + mp := getg().m + lock(&mp.threadLock) + stdcall1(_CloseHandle, mp.thread) + mp.thread = 0 + unlock(&mp.threadLock) } // Calling stdcall on os stack. @@ -1018,17 +1025,25 @@ func profileloop1(param uintptr) uint32 { stdcall2(_WaitForSingleObject, profiletimer, _INFINITE) first := (*m)(atomic.Loadp(unsafe.Pointer(&allm))) for mp := first; mp != nil; mp = mp.alllink { - thread := atomic.Loaduintptr(&mp.thread) + lock(&mp.threadLock) // Do not profile threads blocked on Notes, // this includes idle worker threads, // idle timer thread, idle heap scavenger, etc. - if thread == 0 || mp.profilehz == 0 || mp.blocked { + if mp.thread == 0 || mp.profilehz == 0 || mp.blocked { + unlock(&mp.threadLock) continue } - // mp may exit between the load above and the - // SuspendThread, so be careful. + // Acquire our own handle to the thread. + var thread uintptr + stdcall7(_DuplicateHandle, currentProcess, mp.thread, currentProcess, uintptr(unsafe.Pointer(&thread)), 0, 0, _DUPLICATE_SAME_ACCESS) + unlock(&mp.threadLock) + // mp may exit between the DuplicateHandle + // above and the SuspendThread. The handle + // will remain valid, but SuspendThread may + // fail. if int32(stdcall1(_SuspendThread, thread)) == -1 { // The thread no longer exists. + stdcall1(_CloseHandle, thread) continue } if mp.profilehz != 0 && !mp.blocked { @@ -1037,6 +1052,7 @@ func profileloop1(param uintptr) uint32 { profilem(mp, thread) } stdcall1(_ResumeThread, thread) + stdcall1(_CloseHandle, thread) } } } |