From eb97160f46cb18770ccb0bd57a08457c9ff20ccc Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 22 Nov 2017 19:12:12 -0800 Subject: [PATCH] runtime: don't block signals that will kill the program Otherwise we may delay the delivery of these signals for an arbitrary length of time. We are already careful to not block signals that the program has asked to see. Also make sure that we don't miss a signal delivery if a thread decides to stop for a while while executing the signal handler. Also clean up the TestAtomicStop output a little bit. Fixes #21433 Change-Id: Ic0c1a4eaf7eba80d1abc1e9537570bf4687c2434 Reviewed-on: https://go-review.googlesource.com/79581 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Austin Clements --- src/os/signal/signal_test.go | 6 ++++-- src/runtime/runtime2.go | 2 +- src/runtime/signal_unix.go | 25 ++++++++++++++++++++++--- src/runtime/sigqueue.go | 30 +++++++++++++++++++++++------- 4 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/os/signal/signal_test.go b/src/os/signal/signal_test.go index dea2add0ab..e4df8af816 100644 --- a/src/os/signal/signal_test.go +++ b/src/os/signal/signal_test.go @@ -321,7 +321,9 @@ func TestAtomicStop(t *testing.T) { cmd.Env = append(os.Environ(), "GO_TEST_ATOMIC_STOP=1") out, err := cmd.CombinedOutput() if err == nil { - t.Logf("iteration %d: output %s", i, out) + if len(out) > 0 { + t.Logf("iteration %d: output %s", i, out) + } } else { t.Logf("iteration %d: exit status %q: output: %s", i, err, out) } @@ -378,7 +380,7 @@ func atomicStopTestProgram() { case <-cs: case <-time.After(1 * time.Second): if !printed { - fmt.Print("lost signal on iterations:") + fmt.Print("lost signal on tries:") printed = true } fmt.Printf(" %d", i) diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index 0e7ef2fda2..c75f0b1b7a 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -622,7 +622,7 @@ const ( _SigDefault // if the signal isn't explicitly requested, don't monitor it _SigGoExit // cause all runtime procs to exit (only used on Plan 9). _SigSetStack // add SA_ONSTACK to libc handler - _SigUnblock // unblocked in minit + _SigUnblock // always unblock; see blockableSig _SigIgn // _SIG_DFL action is to ignore the signal ) diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go index 9dae058f2c..e1ba2dbc78 100644 --- a/src/runtime/signal_unix.go +++ b/src/runtime/signal_unix.go @@ -526,7 +526,7 @@ func ensureSigM() { // mask accordingly. sigBlocked := sigset_all for i := range sigtable { - if sigtable[i].flags&_SigUnblock != 0 { + if !blockableSig(uint32(i)) { sigdelset(&sigBlocked, i) } } @@ -538,7 +538,7 @@ func ensureSigM() { sigdelset(&sigBlocked, int(sig)) } case sig := <-disableSigChan: - if sig > 0 { + if sig > 0 && blockableSig(sig) { sigaddset(&sigBlocked, int(sig)) } } @@ -736,7 +736,7 @@ func minitSignalStack() { func minitSignalMask() { nmask := getg().m.sigmask for i := range sigtable { - if sigtable[i].flags&_SigUnblock != 0 { + if !blockableSig(uint32(i)) { sigdelset(&nmask, i) } } @@ -757,6 +757,25 @@ func unminitSignals() { } } +// blockableSig returns whether sig may be blocked by the signal mask. +// We never want to block the signals marked _SigUnblock; +// these are the synchronous signals that turn into a Go panic. +// In a Go program--not a c-archive/c-shared--we never want to block +// the signals marked _SigKill or _SigThrow, as otherwise it's possible +// for all running threads to block them and delay their delivery until +// we start a new thread. When linked into a C program we let the C code +// decide on the disposition of those signals. +func blockableSig(sig uint32) bool { + flags := sigtable[sig].flags + if flags&_SigUnblock != 0 { + return false + } + if isarchive || islibrary { + return true + } + return flags&(_SigKill|_SigThrow) == 0 +} + // gsignalStack saves the fields of the gsignal stack changed by // setGsignalStack. type gsignalStack struct { diff --git a/src/runtime/sigqueue.go b/src/runtime/sigqueue.go index 236bb29929..94e2b69d92 100644 --- a/src/runtime/sigqueue.go +++ b/src/runtime/sigqueue.go @@ -45,13 +45,14 @@ import ( // as there is no connection between handling a signal and receiving one, // but atomic instructions should minimize it. var sig struct { - note note - mask [(_NSIG + 31) / 32]uint32 - wanted [(_NSIG + 31) / 32]uint32 - ignored [(_NSIG + 31) / 32]uint32 - recv [(_NSIG + 31) / 32]uint32 - state uint32 - inuse bool + note note + mask [(_NSIG + 31) / 32]uint32 + wanted [(_NSIG + 31) / 32]uint32 + ignored [(_NSIG + 31) / 32]uint32 + recv [(_NSIG + 31) / 32]uint32 + state uint32 + delivering uint32 + inuse bool } const ( @@ -68,7 +69,11 @@ func sigsend(s uint32) bool { return false } + atomic.Xadd(&sig.delivering, 1) + // We are running in the signal handler; defer is not available. + if w := atomic.Load(&sig.wanted[s/32]); w&bit == 0 { + atomic.Xadd(&sig.delivering, -1) return false } @@ -76,6 +81,7 @@ func sigsend(s uint32) bool { for { mask := sig.mask[s/32] if mask&bit != 0 { + atomic.Xadd(&sig.delivering, -1) return true // signal already in queue } if atomic.Cas(&sig.mask[s/32], mask, mask|bit) { @@ -104,6 +110,7 @@ Send: } } + atomic.Xadd(&sig.delivering, -1) return true } @@ -155,6 +162,15 @@ func signal_recv() uint32 { // by the os/signal package. //go:linkname signalWaitUntilIdle os/signal.signalWaitUntilIdle func signalWaitUntilIdle() { + // Although the signals we care about have been removed from + // sig.wanted, it is possible that another thread has received + // a signal, has read from sig.wanted, is now updating sig.mask, + // and has not yet woken up the processor thread. We need to wait + // until all current signal deliveries have completed. + for atomic.Load(&sig.delivering) != 0 { + Gosched() + } + // Although WaitUntilIdle seems like the right name for this // function, the state we are looking for is sigReceiving, not // sigIdle. The sigIdle state is really more like sigProcessing.