code review 97620043: runtime: fix go of nil func value

runtime: fix go of nil func value
Currently runtime derefences nil with m->locks>0,
which causes unrecoverable fatal error.
Panic instead.
Fixes issue 8045.

[dvyukov, 2014-05-20 16:22:42]

Hello golang-codereviews@googlegroups.com (cc: khr@golang.org, rsc@golang.org),

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

[dvyukov, 2014-05-20 16:23:04]

At your discretion as to whether it needs to go into 1.3.

[rsc, 2014-05-20 16:58:13]

I do want to see this fixed for 1.3.

https://codereview.appspot.com/97620043/diff/20002/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/97620043/diff/20002/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:1820: runtime·panicstring("go of nil func value");
I don't think this is correct. This will panic in the goroutine executing the go
statement. I suspect it should panic in the new goroutine. That should happen
automatically if you can avoid the dereference here when fn == nil. I expect
that means adding a clause to 1867 and also to gostartcallfn (if fv == nil, call
gostartcall(gobuf, nil, nil)).

[dvyukov, 2014-05-20 17:08:17]

https://codereview.appspot.com/97620043/diff/20002/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/97620043/diff/20002/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:1820: runtime·panicstring("go of nil func value");
On 2014/05/20 16:58:13, rsc wrote:
> I don't think this is correct. This will panic in the goroutine executing the
go
> statement. I suspect it should panic in the new goroutine. That should happen
> automatically if you can avoid the dereference here when fn == nil. I expect
> that means adding a clause to 1867 and also to gostartcallfn (if fv == nil,
call
> gostartcall(gobuf, nil, nil)).

If it panics in the new goroutine, then it will crash the program anyway (but
with a better error message).

Some super-defensive can programs wrap goroutines in defer/recover and send the
error to the parent goroutine. This is expected to relieve from checking all
pointers for nils. If we panic in the new goroutine, it won't work.

Do you still think we should panic in the new goroutine?

[rsc, 2014-05-20 17:13:11]

On Tue, May 20, 2014 at 1:08 PM, <dvyukov@google.com> wrote:

>
> https://codereview.appspot.com/97620043/diff/20002/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
>
> https://codereview.appspot.com/97620043/diff/20002/src/
> pkg/runtime/proc.c#newcode1820
> src/pkg/runtime/proc.c:1820: runtime·panicstring("go of nil func
> value");
> On 2014/05/20 16:58:13, rsc wrote:
>
>> I don't think this is correct. This will panic in the goroutine
>>
> executing the go
>
>> statement. I suspect it should panic in the new goroutine. That should
>>
> happen
>
>> automatically if you can avoid the dereference here when fn == nil. I
>>
> expect
>
>> that means adding a clause to 1867 and also to gostartcallfn (if fv ==
>>
> nil, call
>
>> gostartcall(gobuf, nil, nil)).
>>
>
> If it panics in the new goroutine, then it will crash the program anyway
> (but with a better error message).
>
> Some super-defensive can programs wrap goroutines in defer/recover and
> send the error to the parent goroutine. This is expected to relieve from
> checking all pointers for nils. If we panic in the new goroutine, it
> won't work.
>
> Do you still think we should panic in the new goroutine?


Yes, even though it is uncatchable, it is what the code says to do.

Russ

[rsc, 2014-05-20 20:17:43]

Calling runtime.throw is probably a good enough simulation of the uncatchable
panic.

[dvyukov, 2014-05-27 16:46:09]

On 2014/05/20 20:17:43, rsc wrote:
> Calling runtime.throw is probably a good enough simulation of the uncatchable
> panic.

Done
PTAL

[rsc, 2014-05-27 22:59:47]

LGTM

[rsc, 2014-05-28 03:59:59]

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

runtime: fix go of nil func value
Currently runtime derefences nil with m->locks>0,
which causes unrecoverable fatal error.
Panic instead.
Fixes issue 8045.

LGTM=rsc
R=golang-codereviews, rsc
CC=golang-codereviews, khr
https://codereview.appspot.com/97620043

Committer: Russ Cox <rsc@golang.org>