From: Dmitriy Vyukov Date: Fri, 28 Dec 2012 11:36:06 +0000 (+0400) Subject: runtime: fix potential crash in sigqueue X-Git-Tag: go1.1rc2~1523 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=91484c6c4861b56c77702d0d9ccb9315192bb0e4;p=gostls13.git runtime: fix potential crash in sigqueue Fixes #4383. R=golang-dev, minux.ma, rsc, iant CC=golang-dev https://golang.org/cl/6996060 --- diff --git a/src/pkg/os/signal/signal_test.go b/src/pkg/os/signal/signal_test.go index 3494f8c34c..509b273aa2 100644 --- a/src/pkg/os/signal/signal_test.go +++ b/src/pkg/os/signal/signal_test.go @@ -8,6 +8,7 @@ package signal import ( "os" + "runtime" "syscall" "testing" "time" @@ -58,3 +59,43 @@ func TestSignal(t *testing.T) { // The first SIGHUP should be waiting for us on c. waitSig(t, c, syscall.SIGHUP) } + +func TestStress(t *testing.T) { + dur := 3 * time.Second + if testing.Short() { + dur = 100 * time.Millisecond + } + defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(4)) + done := make(chan bool) + finished := make(chan bool) + go func() { + sig := make(chan os.Signal, 1) + Notify(sig, syscall.SIGUSR1) + Loop: + for { + select { + case <-sig: + case <-done: + break Loop + } + } + finished <- true + }() + go func() { + Loop: + for { + select { + case <-done: + break Loop + default: + syscall.Kill(syscall.Getpid(), syscall.SIGUSR1) + runtime.Gosched() + } + } + finished <- true + }() + time.Sleep(dur) + close(done) + <-finished + <-finished +} diff --git a/src/pkg/runtime/sigqueue.goc b/src/pkg/runtime/sigqueue.goc index b49fdba86e..ecc3846625 100644 --- a/src/pkg/runtime/sigqueue.goc +++ b/src/pkg/runtime/sigqueue.goc @@ -5,36 +5,24 @@ // This file implements runtime support for signal handling. // // Most synchronization primitives are not available from -// the signal handler (it cannot block and cannot use locks) +// the signal handler (it cannot block, allocate memory, or use locks) // so the handler communicates with a processing goroutine // via struct sig, below. // -// Ownership for sig.Note passes back and forth between -// the signal handler and the signal goroutine in rounds. -// The initial state is that sig.note is cleared (setup by signal_enable). -// At the beginning of each round, mask == 0. -// The round goes through three stages: -// -// (In parallel) -// 1a) One or more signals arrive and are handled -// by sigsend using cas to set bits in sig.mask. -// The handler that changes sig.mask from zero to non-zero -// calls notewakeup(&sig). -// 1b) Sigrecv calls notesleep(&sig) to wait for the wakeup. -// -// 2) Having received the wakeup, sigrecv knows that sigsend -// will not send another wakeup, so it can noteclear(&sig) -// to prepare for the next round. (Sigsend may still be adding -// signals to sig.mask at this point, which is fine.) -// -// 3) Sigrecv uses cas to grab the current sig.mask and zero it, -// triggering the next round. -// -// The signal handler takes ownership of the note by atomically -// changing mask from a zero to non-zero value. It gives up -// ownership by calling notewakeup. The signal goroutine takes -// ownership by returning from notesleep (caused by the notewakeup) -// and gives up ownership by clearing mask. +// sigsend() is called by the signal handler to queue a new signal. +// signal_recv() is called by the Go program to receive a newly queued signal. +// Synchronization between sigsend() and signal_recv() is based on the sig.state +// variable. It can be in 3 states: 0, HASWAITER and HASSIGNAL. +// HASWAITER means that signal_recv() is blocked on sig.Note and there are no +// new pending signals. +// HASSIGNAL means that sig.mask *may* contain new pending signals, +// signal_recv() can't be blocked in this state. +// 0 means that there are no new pending signals and signal_recv() is not blocked. +// Transitions between states are done atomically with CAS. +// When signal_recv() is unblocked, it resets sig.Note and rechecks sig.mask. +// If several sigsend()'s and signal_recv() execute concurrently, it can lead to +// unnecessary rechecks of sig.mask, but must not lead to missed signals +// nor deadlocks. package runtime #include "runtime.h" @@ -45,15 +33,20 @@ static struct { Note; uint32 mask[(NSIG+31)/32]; uint32 wanted[(NSIG+31)/32]; - uint32 kick; + uint32 state; bool inuse; } sig; +enum { + HASWAITER = 1, + HASSIGNAL = 2, +}; + // Called from sighandler to send a signal back out of the signal handling thread. bool runtime·sigsend(int32 s) { - uint32 bit, mask; + uint32 bit, mask, old, new; if(!sig.inuse || s < 0 || s >= 32*nelem(sig.wanted) || !(sig.wanted[s/32]&(1U<<(s&31)))) return false; @@ -65,8 +58,20 @@ runtime·sigsend(int32 s) if(runtime·cas(&sig.mask[s/32], mask, mask|bit)) { // Added to queue. // Only send a wakeup if the receiver needs a kick. - if(runtime·cas(&sig.kick, 1, 0)) - runtime·notewakeup(&sig); + for(;;) { + old = runtime·atomicload(&sig.state); + if(old == HASSIGNAL) + break; + if(old == HASWAITER) + new = 0; + else // if(old == 0) + new = HASSIGNAL; + if(runtime·cas(&sig.state, old, new)) { + if (old == HASWAITER) + runtime·notewakeup(&sig); + break; + } + } break; } } @@ -77,7 +82,7 @@ runtime·sigsend(int32 s) // Must only be called from a single goroutine at a time. func signal_recv() (m uint32) { static uint32 recv[nelem(sig.mask)]; - int32 i, more; + uint32 i, old, new; for(;;) { // Serve from local copy if there are bits left. @@ -89,15 +94,27 @@ func signal_recv() (m uint32) { } } - // Get a new local copy. - // Ask for a kick if more signals come in - // during or after our check (before the sleep). - if(sig.kick == 0) { - runtime·noteclear(&sig); - runtime·cas(&sig.kick, 0, 1); + // Check and update sig.state. + for(;;) { + old = runtime·atomicload(&sig.state); + if(old == HASWAITER) + runtime·throw("inconsistent state in signal_recv"); + if(old == HASSIGNAL) + new = 0; + else // if(old == 0) + new = HASWAITER; + if(runtime·cas(&sig.state, old, new)) { + if (new == HASWAITER) { + runtime·entersyscall(); + runtime·notesleep(&sig); + runtime·exitsyscall(); + runtime·noteclear(&sig); + } + break; + } } - more = 0; + // Get a new local copy. for(i=0; i