code review 9226043: runtime: fix crash in badsignal()

runtime: fix crash in badsignal()
The linker can generate split stack prolog when a textflag 7 function
makes an indirect function call.  If it happens, badsignal() crashes
trying to dereference g.
Fixes issue 5337.

[bradfitz, 2013-05-06 16:29:13]

https://codereview.appspot.com/9226043/diff/2001/src/pkg/runtime/os_linux.c
File src/pkg/runtime/os_linux.c (right):

https://codereview.appspot.com/9226043/diff/2001/src/pkg/runtime/os_linux.c#n...
src/pkg/runtime/os_linux.c:310: for(i = 0; runtime·sigtab[sig].name[i]; i++) {}
If this fixes it,

a) this deserves a comment and why findnull (strlen?) isn't used.

b) can we fix 6l to not allow calling a stack-split function from a
no-stack-split function?

[dvyukov, 2013-05-06 17:14:57]

On 2013/05/06 16:29:13, bradfitz wrote:
> https://codereview.appspot.com/9226043/diff/2001/src/pkg/runtime/os_linux.c
> File src/pkg/runtime/os_linux.c (right):
> 
>
https://codereview.appspot.com/9226043/diff/2001/src/pkg/runtime/os_linux.c#n...
> src/pkg/runtime/os_linux.c:310: for(i = 0; runtime·sigtab[sig].name[i]; i++)
{}
> If this fixes it,
> 
> a) this deserves a comment and why findnull (strlen?) isn't used.

Yes, it's just a proof of concept.
More importantly I need to handle other platforms :)


> b) can we fix 6l to not allow calling a stack-split function from a
> no-stack-split function?

It's fine in itself. And there are such case in runtime. And there are legal
cases when NOSPLIT stack function makes an indirect call.
What we need here is a much stronger guarantee - that the function does not
access m/g. But we don't have anything similar as of now AFAIK.

[dvyukov, 2013-05-06 17:30:37]

Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),

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

[dfc, 2013-05-06 17:32:57]

Very nice, thank you for getting to the bottom of this.

[bradfitz, 2013-05-06 17:33:41]

LGTM



On Mon, May 6, 2013 at 10:30 AM, <dvyukov@google.com> wrote:

> Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
>
https://dvyukov%40google.com@**code.google.com/p/go/<http://40google.com@code...
>
>
>
https://codereview.appspot.**com/9226043/<https://codereview.appspot.com/9226...
>

[adg, 2013-05-06 17:34:30]

LGTM

[dfc, 2013-05-06 17:35:01]

On 2013/05/06 17:32:57, dfc wrote:
> Very nice, thank you for getting to the bottom of this.

Also, this was very subtle and tricky to track down. Is there any way to write a
test, or a check to ensure that we don't split the stack inside a #pragma
textflag 7 function ?

[iant, 2013-05-06 17:50:13]

See the comment #9 in issue 4048, where adonovan says that inlining findnull
does not work.  What has changed such that it works now?

[iant, 2013-05-06 17:52:50]

On 2013/05/06 17:35:01, dfc wrote:
> 
> Also, this was very subtle and tricky to track down. Is there any way to write
a
> test, or a check to ensure that we don't split the stack inside a #pragma
> textflag 7 function ?

I don't see a way.  We could have the compiler prohibit indirect function calls
in textflag 7 functions, but that would prevent the kind of hack that is
discussed in issue 4048.  The analysis there was evidently wrong, but I don't
think we want a conservative compiler check that makes it impossible to do this
kind of analysis.

[r, 2013-05-06 18:28:46]

https://codereview.appspot.com/9226043/diff/9001/src/pkg/runtime/os_darwin.c
File src/pkg/runtime/os_darwin.c (right):

https://codereview.appspot.com/9226043/diff/9001/src/pkg/runtime/os_darwin.c#...
src/pkg/runtime/os_darwin.c:551: for(len = 0; runtime·sigtab[sig].name[len];
len++) {}
please put this on multiple lines. this isn't the usual style in the C code.
same for the other files

[bradfitz, 2013-05-06 18:36:01]

On Mon, May 6, 2013 at 10:50 AM, <iant@golang.org> wrote:

> See the comment #9 in issue 4048, where adonovan says that inlining
> findnull does not work.  What has changed such that it works now?
>

I don't think we have a repro yet.  This was just a hunch from Dmitry.

[iant, 2013-05-06 20:10:21]

On Mon, May 6, 2013 at 11:35 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Mon, May 6, 2013 at 10:50 AM, <iant@golang.org> wrote:
>>
>> See the comment #9 in issue 4048, where adonovan says that inlining
>> findnull does not work.  What has changed such that it works now?
>
>
> I don't think we have a repro yet.  This was just a hunch from Dmitry.

I think the inlining issue was build failures.  It's possible that it
was addressed by shrinking the stack frame size elsewhere, I just want
to make sure we know where.

Ian

[iant, 2013-05-06 21:20:55]

LGTM

I've confirmed that the previous attempt to use a local loop caused build
failures.  I rolled back to revision 15745 and applied this patch to it. 
Building that gave an error building the go tool:

runtime.sigtramp: nosplit stack overflow
	120	assumed on entry to runtime.sigtramp
	56	after runtime.sigtramp uses 64
	48	on entry to runtime.badsignal
	0	after runtime.badsignal uses 48
	-8	on entry to runtime.write
go tool dist: FAILED: /home/iant/go4/pkg/tool/linux_amd64/6l -o
/home/iant/go4/pkg/tool/linux_amd64/go_bootstrap $WORK/_go_.6

Looking at the code that the linker generates for badsignal with Dmitriy's patch
in the current source shows that it uses only 32 bytes of stack space, not 48. 
So presumably something has changed in the compiler that caused this approach to
fail earlier but work today.

[minux1, 2013-05-06 21:28:02]

On Tue, May 7, 2013 at 5:20 AM, <iant@golang.org> wrote:

> Looking at the code that the linker generates for badsignal with
> Dmitriy's patch in the current source shows that it uses only 32 bytes
> of stack space, not 48.  So presumably something has changed in the
> compiler that caused this approach to fail earlier but work today.
>
It's a bug fixed in https://codereview.appspot.com/7303099.

i suggest we make the linker complain if it must add splitstack prolog to
a textflag == 7 function (or fix the bug in the linker so that it won't
generate
splitstack prolog for such functions).

i remember we have other use cases for this trick.

[bradfitz, 2013-05-06 21:39:38]

On Mon, May 6, 2013 at 2:27 PM, minux <minux.ma@gmail.com> wrote:

>
> On Tue, May 7, 2013 at 5:20 AM, <iant@golang.org> wrote:
>
>> Looking at the code that the linker generates for badsignal with
>> Dmitriy's patch in the current source shows that it uses only 32 bytes
>> of stack space, not 48.  So presumably something has changed in the
>> compiler that caused this approach to fail earlier but work today.
>>
> It's a bug fixed in https://codereview.appspot.com/7303099.
>

Thanks for tracking that down.

[minux1, 2013-05-06 23:01:08]

we need a test in runtime for signal happening on foreign threads.

[r, 2013-05-06 23:09:43]

LGTM

[iant, 2013-05-06 23:15:08]

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

runtime: fix crash in badsignal()
The linker can generate split stack prolog when a textflag 7 function
makes an indirect function call.  If it happens, badsignal() crashes
trying to dereference g.
Fixes issue 5337.

R=bradfitz, dave, adg, iant, r, minux.ma
CC=adonovan, golang-dev
https://codereview.appspot.com/9226043

Committer: Ian Lance Taylor <iant@golang.org>

[minux1, 2013-05-06 23:19:48]

The root cause is cmd/cc failed to annotate runtime.badsignal as textflag 7.

(pkg/runtime/os_linux.c:301)  TEXT    runtime.badsignal+0(SB),$32-8
(pkg/runtime/slice.c:77)      TEXT    runtime.appendslice+0(SB),7,$104-0

[dfc, 2013-05-06 23:21:11]

Could you please raise a bug for this failure. 

Thanks

Dave



On 06/05/2013, at 4:19 PM, minux.ma@gmail.com wrote:

> The root cause is cmd/cc failed to annotate runtime.badsignal as
> textflag 7.
> 
> (pkg/runtime/os_linux.c:301)  TEXT    runtime.badsignal+0(SB),$32-8
> (pkg/runtime/slice.c:77)      TEXT    runtime.appendslice+0(SB),7,$104-0
> 
> https://codereview.appspot.com/9226043/

[iant, 2013-05-06 23:25:08]

On Mon, May 6, 2013 at 4:01 PM,  <minux.ma@gmail.com> wrote:
> we need a test in runtime for signal happening on foreign threads.

I think it will have to be a test in misc/cgo.  Could you please open
an issue for that?  Thanks.  We should be able to do it by invoking C
code that starts a thread, and having the Go code import
runtime/pprof, and then waiting for a profiling signal while running
in the C thread.  Or something like that.

Ian

[minux1, 2013-05-06 23:29:30]

On 2013/05/06 23:19:48, minux wrote:
> The root cause is cmd/cc failed to annotate runtime.badsignal as textflag 7.
> 
> (pkg/runtime/os_linux.c:301)  TEXT    runtime.badsignal+0(SB),$32-8
> (pkg/runtime/slice.c:77)      TEXT    runtime.appendslice+0(SB),7,$104-0
because the textflag 7 is incorrectly attached to the static variable findnull.

[minux1, 2013-05-06 23:53:58]

On Tue, May 7, 2013 at 7:25 AM, Ian Lance Taylor <iant@golang.org> wrote:

> On Mon, May 6, 2013 at 4:01 PM,  <minux.ma@gmail.com> wrote:
> > we need a test in runtime for signal happening on foreign threads.
>
> I think it will have to be a test in misc/cgo.  Could you please open
> an issue for that?  Thanks.  We should be able to do it by invoking C
> code that starts a thread, and having the Go code import
> runtime/pprof, and then waiting for a profiling signal while running
> in the C thread.  Or something like that.
>
how about https://codereview.appspot.com/9249043/ ?

[minux1, 2013-05-07 02:20:22]

On Tue, May 7, 2013 at 7:21 AM, Dave Cheney <dave@cheney.net> wrote:

> Could you please raise a bug for this failure
>
filed https://code.google.com/p/go/issues/detail?id=5419

[dfc, 2013-05-07 05:37:30]

Thank you.

On Tue, May 7, 2013 at 12:20 PM, minux <minux.ma@gmail.com> wrote:
>
> On Tue, May 7, 2013 at 7:21 AM, Dave Cheney <dave@cheney.net> wrote:
>>
>> Could you please raise a bug for this failure
>
> filed https://code.google.com/p/go/issues/detail?id=5419