Commit 957259b7 authored by Austin Clements's avatar Austin Clements

runtime: protect against external code calling ExitProcess

On Windows, we implement asynchronous preemption using SuspendThread
to suspend other threads in our process. However, SuspendThread is
itself actually asynchronous (it enqueues a kernel "asynchronous
procedure call" and returns). Unfortunately, Windows' ExitProcess API
kills all threads except the calling one and then runs APCs. As a
result, if SuspendThread and ExitProcess are called simultaneously,
the exiting thread can be suspended and the suspending thread can be
exited, leaving behind a ghost process consisting of a single thread
that's suspended.

We've already protected against the runtime's own calls to
ExitProcess, but if Go code calls external code, there's nothing
stopping that code from calling ExitProcess. For example, in #35775,
our own call to racefini leads to C code calling ExitProcess and
occasionally causing a deadlock.

This CL fixes this by introducing synchronization between calling
external code on Windows and preemption. It adds an atomic field to
the M that participates in a simple CAS-based synchronization protocol
to prevent suspending a thread running external code. We use this to
protect cgocall (which is used for both cgo calls and system calls on
Windows) and racefini.

Tested by running the flag package's TestParse test compiled in race
mode in a loop. Before this change, this would reliably deadlock after
a few minutes.

Fixes #35775.
Updates #10958, #24543.

Change-Id: I50d847abcdc2688b4f71eee6a75eca0f2fee892c
Reviewed-on: https://go-review.googlesource.com/c/go/+/213837
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: default avatarCherry Zhang <cherryyz@google.com>
Reviewed-by: default avatarDavid Chase <drchase@google.com>
parent 6dbcc8b8
...@@ -90,6 +90,11 @@ import ( ...@@ -90,6 +90,11 @@ import (
type cgoCallers [32]uintptr type cgoCallers [32]uintptr
// Call from Go to C. // Call from Go to C.
//
// This must be nosplit because it's used for syscalls on some
// platforms. Syscalls may have untyped arguments on the stack, so
// it's not safe to grow or scan the stack.
//
//go:nosplit //go:nosplit
func cgocall(fn, arg unsafe.Pointer) int32 { func cgocall(fn, arg unsafe.Pointer) int32 {
if !iscgo && GOOS != "solaris" && GOOS != "illumos" && GOOS != "windows" { if !iscgo && GOOS != "solaris" && GOOS != "illumos" && GOOS != "windows" {
...@@ -127,6 +132,13 @@ func cgocall(fn, arg unsafe.Pointer) int32 { ...@@ -127,6 +132,13 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
// saved by entersyscall here. // saved by entersyscall here.
entersyscall() entersyscall()
// Tell asynchronous preemption that we're entering external
// code. We do this after entersyscall because this may block
// and cause an async preemption to fail, but at this point a
// sync preemption will succeed (though this is not a matter
// of correctness).
osPreemptExtEnter(mp)
mp.incgo = true mp.incgo = true
errno := asmcgocall(fn, arg) errno := asmcgocall(fn, arg)
...@@ -135,6 +147,8 @@ func cgocall(fn, arg unsafe.Pointer) int32 { ...@@ -135,6 +147,8 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
mp.incgo = false mp.incgo = false
mp.ncgo-- mp.ncgo--
osPreemptExtExit(mp)
exitsyscall() exitsyscall()
// Note that raceacquire must be called only after exitsyscall has // Note that raceacquire must be called only after exitsyscall has
...@@ -188,12 +202,16 @@ func cgocallbackg(ctxt uintptr) { ...@@ -188,12 +202,16 @@ func cgocallbackg(ctxt uintptr) {
exitsyscall() // coming out of cgo call exitsyscall() // coming out of cgo call
gp.m.incgo = false gp.m.incgo = false
osPreemptExtExit(gp.m)
cgocallbackg1(ctxt) cgocallbackg1(ctxt)
// At this point unlockOSThread has been called. // At this point unlockOSThread has been called.
// The following code must not change to a different m. // The following code must not change to a different m.
// This is enforced by checking incgo in the schedule function. // This is enforced by checking incgo in the schedule function.
osPreemptExtEnter(gp.m)
gp.m.incgo = true gp.m.incgo = true
// going back to cgo call // going back to cgo call
reentersyscall(savedpc, uintptr(savedsp)) reentersyscall(savedpc, uintptr(savedsp))
...@@ -352,6 +370,7 @@ func unwindm(restore *bool) { ...@@ -352,6 +370,7 @@ func unwindm(restore *bool) {
if mp.ncgo > 0 { if mp.ncgo > 0 {
mp.incgo = false mp.incgo = false
mp.ncgo-- mp.ncgo--
osPreemptExtExit(mp)
} }
releasem(mp) releasem(mp)
......
...@@ -151,6 +151,29 @@ type mOS struct { ...@@ -151,6 +151,29 @@ type mOS struct {
waitsema uintptr // semaphore for parking on locks waitsema uintptr // semaphore for parking on locks
resumesema uintptr // semaphore to indicate suspend/resume resumesema uintptr // semaphore to indicate suspend/resume
// preemptExtLock synchronizes preemptM with entry/exit from
// external C code.
//
// This protects against races between preemptM calling
// SuspendThread and external code on this thread calling
// ExitProcess. If these happen concurrently, it's possible to
// exit the suspending thread and suspend the exiting thread,
// leading to deadlock.
//
// 0 indicates this M is not being preempted or in external
// code. Entering external code CASes this from 0 to 1. If
// this fails, a preemption is in progress, so the thread must
// wait for the preemption. preemptM also CASes this from 0 to
// 1. If this fails, the preemption fails (as it would if the
// PC weren't in Go code). The value is reset to 0 when
// returning from external code or after a preemption is
// complete.
//
// TODO(austin): We may not need this if preemption were more
// tightly synchronized on the G/P status and preemption
// blocked transition into _Gsyscall/_Psyscall.
preemptExtLock uint32
} }
//go:linkname os_sigpipe os.sigpipe //go:linkname os_sigpipe os.sigpipe
...@@ -1121,11 +1144,20 @@ func preemptM(mp *m) { ...@@ -1121,11 +1144,20 @@ func preemptM(mp *m) {
throw("self-preempt") throw("self-preempt")
} }
// Synchronize with external code that may try to ExitProcess.
if !atomic.Cas(&mp.preemptExtLock, 0, 1) {
// External code is running. Fail the preemption
// attempt.
atomic.Xadd(&mp.preemptGen, 1)
return
}
// Acquire our own handle to mp's thread. // Acquire our own handle to mp's thread.
lock(&mp.threadLock) lock(&mp.threadLock)
if mp.thread == 0 { if mp.thread == 0 {
// The M hasn't been minit'd yet (or was just unminit'd). // The M hasn't been minit'd yet (or was just unminit'd).
unlock(&mp.threadLock) unlock(&mp.threadLock)
atomic.Store(&mp.preemptExtLock, 0)
atomic.Xadd(&mp.preemptGen, 1) atomic.Xadd(&mp.preemptGen, 1)
return return
} }
...@@ -1151,6 +1183,7 @@ func preemptM(mp *m) { ...@@ -1151,6 +1183,7 @@ func preemptM(mp *m) {
if int32(stdcall1(_SuspendThread, thread)) == -1 { if int32(stdcall1(_SuspendThread, thread)) == -1 {
unlock(&suspendLock) unlock(&suspendLock)
stdcall1(_CloseHandle, thread) stdcall1(_CloseHandle, thread)
atomic.Store(&mp.preemptExtLock, 0)
// The thread no longer exists. This shouldn't be // The thread no longer exists. This shouldn't be
// possible, but just acknowledge the request. // possible, but just acknowledge the request.
atomic.Xadd(&mp.preemptGen, 1) atomic.Xadd(&mp.preemptGen, 1)
...@@ -1191,9 +1224,43 @@ func preemptM(mp *m) { ...@@ -1191,9 +1224,43 @@ func preemptM(mp *m) {
stdcall2(_SetThreadContext, thread, uintptr(unsafe.Pointer(c))) stdcall2(_SetThreadContext, thread, uintptr(unsafe.Pointer(c)))
} }
atomic.Store(&mp.preemptExtLock, 0)
// Acknowledge the preemption. // Acknowledge the preemption.
atomic.Xadd(&mp.preemptGen, 1) atomic.Xadd(&mp.preemptGen, 1)
stdcall1(_ResumeThread, thread) stdcall1(_ResumeThread, thread)
stdcall1(_CloseHandle, thread) stdcall1(_CloseHandle, thread)
} }
// osPreemptExtEnter is called before entering external code that may
// call ExitProcess.
//
// This must be nosplit because it may be called from a syscall with
// untyped stack slots, so the stack must not be grown or scanned.
//
//go:nosplit
func osPreemptExtEnter(mp *m) {
for !atomic.Cas(&mp.preemptExtLock, 0, 1) {
// An asynchronous preemption is in progress. It's not
// safe to enter external code because it may call
// ExitProcess and deadlock with SuspendThread.
// Ideally we would do the preemption ourselves, but
// can't since there may be untyped syscall arguments
// on the stack. Instead, just wait and encourage the
// SuspendThread APC to run. The preemption should be
// done shortly.
osyield()
}
// Asynchronous preemption is now blocked.
}
// osPreemptExtExit is called after returning from external code that
// may call ExitProcess.
//
// See osPreemptExtEnter for why this is nosplit.
//
//go:nosplit
func osPreemptExtExit(mp *m) {
atomic.Store(&mp.preemptExtLock, 0)
}
// Copyright 2020 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 !windows
package runtime
//go:nosplit
func osPreemptExtEnter(mp *m) {}
//go:nosplit
func osPreemptExtExit(mp *m) {}
...@@ -403,6 +403,9 @@ func racefini() { ...@@ -403,6 +403,9 @@ func racefini() {
// already held it's assumed that the first caller exits the program // already held it's assumed that the first caller exits the program
// so other calls can hang forever without an issue. // so other calls can hang forever without an issue.
lock(&raceFiniLock) lock(&raceFiniLock)
// We're entering external code that may call ExitProcess on
// Windows.
osPreemptExtEnter(getg().m)
racecall(&__tsan_fini, 0, 0, 0, 0) racecall(&__tsan_fini, 0, 0, 0, 0)
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment