code review 12541052: runtime: impose stack size limit

runtime: impose stack size limit

The goal is to stop only those programs that would keep
going and run the machine out of memory, but before they do that.
1 GB on 64-bit, 250 MB on 32-bit.
That seems implausibly large, and it can be adjusted.

Fixes issue 2556.
Fixes issue 4494.
Fixes issue 5173.

[rsc, 2013-08-09 20:01:03]

Hello khr, r (cc: golang-dev@googlegroups.com),

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

[r, 2013-08-09 20:58:57]

perhaps the number should be less on 32-bit architectures.

[khr, 2013-08-09 22:12:46]

LGTM.

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/debug/garba...
File src/pkg/runtime/debug/garbage.go (right):

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/debug/garba...
src/pkg/runtime/debug/garbage.go:27: func setMaxStack(int) int
Why is this in runtime/debug/garbage.go and not runtime/debug/stack.go?

[rsc, 2013-08-09 22:31:22]

On Fri, Aug 9, 2013 at 6:12 PM, <khr@golang.org> wrote:

> LGTM.
>
> https://codereview.appspot.**com/12541052/diff/5001/src/**
>
pkg/runtime/debug/garbage.go<https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/debug/garbage.go>
> File src/pkg/runtime/debug/garbage.**go (right):
>
> https://codereview.appspot.**com/12541052/diff/5001/src/**
>
pkg/runtime/debug/garbage.go#**newcode27<https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/debug/garbage.go#newcode27>
> src/pkg/runtime/debug/garbage.**go:27: func setMaxStack(int) int
> Why is this in runtime/debug/garbage.go and not runtime/debug/stack.go?
>

Because stack.go turns out to be a bunch of deprecated code. :-(

[dvyukov, 2013-08-10 13:22:55]

Fixes issue 4692.

[dvyukov, 2013-08-10 13:37:18]

crash test please

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/debug/garba...
File src/pkg/runtime/debug/garbage.go (right):

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/debug/garba...
src/pkg/runtime/debug/garbage.go:112: // goroutines that enter an infinite
recursion.
probably add that it should be called early and does not affect existing stacks
it may be obvious, though

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/stack.c#new...
src/pkg/runtime/stack.c:188: uintptr runtime·maxstacksize = 1000000000; // 1 GB
1<<30

[dvyukov, 2013-08-10 13:39:37]

>
https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/stack.c#new...
> src/pkg/runtime/stack.c:188: uintptr runtime·maxstacksize = 1000000000; // 1
GB
> 1<<30

hey, do you actually do not mean *gibi*byte here?

[rsc, 2013-08-10 13:51:00]

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/stack.c#new...
src/pkg/runtime/stack.c:188: uintptr runtime·maxstacksize = 1000000000; // 1 GB
On 2013/08/10 13:37:18, dvyukov wrote:
> 1<<30

I used to have 1<<30 but then when it triggers it prints the limit as an ugly
number. The message is easier to read if the limit is 1 billion bytes.

[dvyukov, 2013-08-10 13:57:17]

On Sat, Aug 10, 2013 at 5:51 PM, <rsc@golang.org> wrote:

>
> https://codereview.appspot.**com/12541052/diff/5001/src/**
>
pkg/runtime/stack.c<https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/stack.c>
> File src/pkg/runtime/stack.c (right):
>
> https://codereview.appspot.**com/12541052/diff/5001/src/**
>
pkg/runtime/stack.c#newcode188<https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/stack.c#newcode188>
> src/pkg/runtime/stack.c:188: uintptr runtime·maxstacksize = 1000000000;
> // 1 GB
> On 2013/08/10 13:37:18, dvyukov wrote:
>
>> 1<<30
>>
>
> I used to have 1<<30 but then when it triggers it prints the limit as an
> ugly number. The message is easier to read if the limit is 1 billion
> bytes.
>

OK, at least it is consistent with documentation, 1GB is indeed 1000000000
bytes today :)

[r, 2013-08-10 21:27:00]

I still think the number should be lowered on 32-bit architectures, which
means the constant needs to be defined in architecture-specific files.

[rsc, 2013-08-13 17:34:03]

On Sat, Aug 10, 2013 at 5:26 PM, Rob Pike <r@golang.org> wrote:

> I still think the number should be lowered on 32-bit architectures
>

Sure. I haven't done anything yet because one of the TestCgoCrash... tests
is failing with an overflow, and I am skeptical that it actually used 1 GB
of stack. Perhaps the accounting is not subtracting correctly. I haven't
had a chance to check.

Russ

[rsc, 2013-08-16 02:33:42]

I can't reproduce the CgoCrash failures I was seeing. Maybe there was a
different bug that has been fixed. Or maybe we'll find it again later.

I set the 32-bit limit to 250 MB.

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/debug/garba...
File src/pkg/runtime/debug/garbage.go (right):

https://codereview.appspot.com/12541052/diff/5001/src/pkg/runtime/debug/garba...
src/pkg/runtime/debug/garbage.go:112: // goroutines that enter an infinite
recursion.
On 2013/08/10 13:37:18, dvyukov wrote:
> probably add that it should be called early and does not affect existing
stacks
> it may be obvious, though

I added "It only limits future stack growth". I don't want to tell people when
to call it.

[rsc, 2013-08-16 02:34:10]

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

runtime: impose stack size limit

The goal is to stop only those programs that would keep
going and run the machine out of memory, but before they do that.
1 GB on 64-bit, 250 MB on 32-bit.
That seems implausibly large, and it can be adjusted.

Fixes issue 2556.
Fixes issue 4494.
Fixes issue 5173.

R=khr, r, dvyukov
CC=golang-dev
https://codereview.appspot.com/12541052

[bradfitz, 2013-08-16 03:42:24]

This caused:

run                  nilptr2.go          : incorrect output
runtime: goroutine stack exceeds 250000000-byte limit
fatal error: stack overflow

goroutine 1 [stack split]:
main.func·031()
	/private/tmp/gobuilder/darwin-386-ca87889605e6/go/test/nilptr2.go:76 fp=0xc0f44
main.func·054()
	/private/tmp/gobuilder/darwin-386-ca87889605e6/go/test/nilptr2.go:19 +0x79
fp=0xc0f5c
main.main()
	/private/tmp/gobuilder/darwin-386-ca87889605e6/go/test/nilptr2.go:20 +0xad
fp=0xc0f9c
runtime.main()
	/usr/local/go/src/pkg/runtime/proc.c:209 +0xff fp=0xc0fd0
runtime.goexit()
	/usr/local/go/src/pkg/runtime/proc.c:1373 fp=0xc0fd4

goroutine 2 [runnable]:
runtime.MHeap_Scavenger()
	/usr/local/go/src/pkg/runtime/mheap.c:448
runtime.goexit()
	/usr/local/go/src/pkg/runtime/proc.c:1373
exit status 2

exit status 1