code review 6996060: runtime: fix potential crash in sigqueue

runtime: fix potential crash in sigqueue
Fixes issue 4383.

[dvyukov, 2012-12-24 17:12:09]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[minux1, 2012-12-25 05:31:37]

https://codereview.appspot.com/6996060/diff/3003/src/pkg/os/signal/signal_tes...
File src/pkg/os/signal/signal_test.go (right):

https://codereview.appspot.com/6996060/diff/3003/src/pkg/os/signal/signal_tes...
src/pkg/os/signal/signal_test.go:65: return
why skip the test in short mode?
you'd better add a t.Log sbout skipping
if you really don't want to test it in
short mode.
also note that the builder test only short
mode, skipping means the builders won't
catch future breaks.

https://codereview.appspot.com/6996060/diff/3003/src/pkg/os/signal/signal_tes...
src/pkg/os/signal/signal_test.go:68: go func() {
need a way to tear down these two goroutines?
maybe this is the reason why yoy skip the test
in short mode?

https://codereview.appspot.com/6996060/diff/3003/src/pkg/runtime/sigqueue.goc
File src/pkg/runtime/sigqueue.goc (right):

https://codereview.appspot.com/6996060/diff/3003/src/pkg/runtime/sigqueue.goc...
src/pkg/runtime/sigqueue.goc:22: // calls notewakeup(&sig).
these text need updating?
it seems the ownership of note is denoted
by state var. now.

https://codereview.appspot.com/6996060/diff/3003/src/pkg/runtime/sigqueue.goc...
src/pkg/runtime/sigqueue.goc:52: #define	HASWAITER 1
please use enum for these two constants.

https://codereview.appspot.com/6996060/diff/3003/src/pkg/runtime/sigqueue.goc...
src/pkg/runtime/sigqueue.goc:139: // goc requires that we fall off the end of
functions
is this still true?

[dvyukov, 2012-12-25 10:31:55]

Done. PTAL. Thanks!

https://codereview.appspot.com/6996060/diff/3003/src/pkg/os/signal/signal_tes...
File src/pkg/os/signal/signal_test.go (right):

https://codereview.appspot.com/6996060/diff/3003/src/pkg/os/signal/signal_tes...
src/pkg/os/signal/signal_test.go:65: return
On 2012/12/25 05:31:37, minux wrote:
> why skip the test in short mode?
> you'd better add a t.Log sbout skipping
> if you really don't want to test it in
> short mode.
> also note that the builder test only short
> mode, skipping means the builders won't
> catch future breaks.

Done.

https://codereview.appspot.com/6996060/diff/3003/src/pkg/os/signal/signal_tes...
src/pkg/os/signal/signal_test.go:68: go func() {
On 2012/12/25 05:31:37, minux wrote:
> need a way to tear down these two goroutines?
> maybe this is the reason why yoy skip the test
> in short mode?

Done.

https://codereview.appspot.com/6996060/diff/3003/src/pkg/runtime/sigqueue.goc
File src/pkg/runtime/sigqueue.goc (right):

https://codereview.appspot.com/6996060/diff/3003/src/pkg/runtime/sigqueue.goc...
src/pkg/runtime/sigqueue.goc:22: // calls notewakeup(&sig).
On 2012/12/25 05:31:37, minux wrote:
> these text need updating?
> it seems the ownership of note is denoted
> by state var. now.

It seems it was outdated before me (it does not mention kick logic).
Done.

https://codereview.appspot.com/6996060/diff/3003/src/pkg/runtime/sigqueue.goc...
src/pkg/runtime/sigqueue.goc:52: #define	HASWAITER 1
On 2012/12/25 05:31:37, minux wrote:
> please use enum for these two constants.

Done.

https://codereview.appspot.com/6996060/diff/3003/src/pkg/runtime/sigqueue.goc...
src/pkg/runtime/sigqueue.goc:139: // goc requires that we fall off the end of
functions
On 2012/12/25 05:31:37, minux wrote:
> is this still true?

Yes

[minux1, 2012-12-26 02:46:24]

LGTM. Please wait for rsc or iant to sign off.

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.goc
File src/pkg/runtime/sigqueue.goc (right):

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.go...
src/pkg/runtime/sigqueue.goc:15: // new pedning signals.
s/pedning/pending/

[iant, 2012-12-27 00:05:35]

LGTM

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.goc
File src/pkg/runtime/sigqueue.goc (right):

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.go...
src/pkg/runtime/sigqueue.goc:8: // the signal handler (it cannot block, allocate
memory and cannot use locks)
Clause is not parallel.  Suggest "it cannot block, allocate memory, or use
locks".

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.go...
src/pkg/runtime/sigqueue.goc:12: // Synchronization between sigsend() and
signal_recv() is based on the sig.state
I think this needs an introductory sentence or two.  E.g., "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."

[dvyukov, 2012-12-28 11:33:19]

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.goc
File src/pkg/runtime/sigqueue.goc (right):

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.go...
src/pkg/runtime/sigqueue.goc:8: // the signal handler (it cannot block, allocate
memory and cannot use locks)
On 2012/12/27 00:05:35, iant wrote:
> Clause is not parallel.  Suggest "it cannot block, allocate memory, or use
> locks".

Done.

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.go...
src/pkg/runtime/sigqueue.goc:12: // Synchronization between sigsend() and
signal_recv() is based on the sig.state
On 2012/12/27 00:05:35, iant wrote:
> I think this needs an introductory sentence or two.  E.g., "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."

Done.

https://codereview.appspot.com/6996060/diff/12001/src/pkg/runtime/sigqueue.go...
src/pkg/runtime/sigqueue.goc:15: // new pedning signals.
On 2012/12/26 02:46:24, minux wrote:
> s/pedning/pending/

Done.

[dvyukov, 2012-12-28 11:36:23]

*** Submitted as https://code.google.com/p/go/source/detail?r=b07c0f6d6b5f ***

runtime: fix potential crash in sigqueue
Fixes issue 4383.

R=golang-dev, minux.ma, rsc, iant
CC=golang-dev
https://codereview.appspot.com/6996060