runtime: handle stray profiling signal better

In c-archive mode, when we turn off profiling, we  restore the
previous handler for SIGPROF, and ignore SIGPROF signals if no
handler was installed. So if a pending signal lands after we
remove the Go signal handler, it will not kill the program.

In the current code there is a small window, where we can still
receive signals but we are set to not handling the signal. If a
signal lands in this window (possibly on another thread), it will
see that we are not handling this signal and no previous handler
installed, and kill the program. To avoid this race, we set the
previous handler to SIG_IGN (ignoring the signal) when turning on
profiling. So when turning off profiling we'll ignore the signal
even if a stray signal lands in the small window.

Fixes #43828.

Change-Id: I304bc85a93ca0e63b0c0d8e902b097bfdc8e3f1d
Reviewed-on: https://go-review.googlesource.com/c/go/+/374074
Trust: Cherry Mui <cherryyz@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

1 files changed, 18 insertions, 13 deletions

diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go
index dbcbfc67bc..08f266cc67 100644
--- a/src/runtime/signal_unix.go
+++ b/src/runtime/signal_unix.go
@@ -271,7 +271,24 @@ func setProcessCPUProfilerTimer(hz int32) {
 	if hz != 0 {
 		// Enable the Go signal handler if not enabled.
 		if atomic.Cas(&handlingSig[_SIGPROF], 0, 1) {
-			atomic.Storeuintptr(&fwdSig[_SIGPROF], getsig(_SIGPROF))
+			h := getsig(_SIGPROF)
+			// If no signal handler was installed before, then we record
+			// _SIG_IGN here. When we turn off profiling (below) we'll start
+			// ignoring SIGPROF signals. We do this, rather than change
+			// to SIG_DFL, because there may be a pending SIGPROF
+			// signal that has not yet been delivered to some other thread.
+			// If we change to SIG_DFL when turning off profiling, the
+			// program will crash when that SIGPROF is delivered. We assume
+			// that programs that use profiling don't want to crash on a
+			// stray SIGPROF. See issue 19320.
+			// We do the change here instead of when turning off profiling,
+			// because there we may race with a signal handler running
+			// concurrently, in particular, sigfwdgo may observe _SIG_DFL and
+			// die. See issue 43828.
+			if h == _SIG_DFL {
+				h = _SIG_IGN
+			}
+			atomic.Storeuintptr(&fwdSig[_SIGPROF], h)
 			setsig(_SIGPROF, abi.FuncPCABIInternal(sighandler))
 		}
 
@@ -288,21 +305,9 @@ func setProcessCPUProfilerTimer(hz int32) {
 		// when we enabled profiling. We don't try to handle the case
 		// of a program that changes the SIGPROF handler while Go
 		// profiling is enabled.
-		//
-		// If no signal handler was installed before, then start
-		// ignoring SIGPROF signals. We do this, rather than change
-		// to SIG_DFL, because there may be a pending SIGPROF
-		// signal that has not yet been delivered to some other thread.
-		// If we change to SIG_DFL here, the program will crash
-		// when that SIGPROF is delivered. We assume that programs
-		// that use profiling don't want to crash on a stray SIGPROF.
-		// See issue 19320.
 		if !sigInstallGoHandler(_SIGPROF) {
 			if atomic.Cas(&handlingSig[_SIGPROF], 1, 0) {
 				h := atomic.Loaduintptr(&fwdSig[_SIGPROF])
-				if h == _SIG_DFL {
-					h = _SIG_IGN
-				}
 				setsig(_SIGPROF, h)
 			}
 		}