code review 72480043: runtime: fix memory corruption and leak in recursive pa...

runtime: fix memory corruption and leak in recursive panic handling
Recursive panics leave dangling Panic structs in g->panic stack.
At best it leads to a Defer leak and incorrect output on a subsequent panic.
At worst it arbitrary corrupts heap.

[dvyukov, 2014-03-07 15:47:27]

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

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

[rsc, 2014-03-07 16:34:48]

LGTM

Thanks very much for finding this. Just some naming and comment comments.

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:208: static void skippanic(Panic*);
throughout, please s/skip/abort/

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:215: Defer *d, d0;
s/d0/dabort/

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:241: // setup the defer to remove this panic from
g->panic stack
// The deferred function may cause another panic,
// so newstackcall may not return. Set up a defer
// to mark this panic aborted if that happens.
dabort.link = ...

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:249: // there were no recursive panic, remove the defer
// Newstackcall did not panic. Remove dabort.

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:257: // check if there are some nested panics that we
need
// Aborted panics are marked but remain on the g->panic list.
// Recovery will unwind the stack frames containing their Panic structs.
// Remove them from the list and free the associated defers.
while(...

[dvyukov, 2014-03-07 16:49:38]

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:208: static void skippanic(Panic*);
On 2014/03/07 16:34:49, rsc wrote:
> throughout, please s/skip/abort/
> 

Done.

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:215: Defer *d, d0;
On 2014/03/07 16:34:49, rsc wrote:
> s/d0/dabort/

Done.

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:241: // setup the defer to remove this panic from
g->panic stack
On 2014/03/07 16:34:49, rsc wrote:
> // The deferred function may cause another panic,
> // so newstackcall may not return. Set up a defer
> // to mark this panic aborted if that happens.
> dabort.link = ...

Done.

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:249: // there were no recursive panic, remove the defer
On 2014/03/07 16:34:49, rsc wrote:
> // Newstackcall did not panic. Remove dabort.

Done.

https://codereview.appspot.com/72480043/diff/60001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:257: // check if there are some nested panics that we
need
On 2014/03/07 16:34:49, rsc wrote:
> // Aborted panics are marked but remain on the g->panic list.
> // Recovery will unwind the stack frames containing their Panic structs.
> // Remove them from the list and free the associated defers.
> while(...

Done.

[dvyukov, 2014-03-07 16:50:34]

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

runtime: fix memory corruption and leak in recursive panic handling
Recursive panics leave dangling Panic structs in g->panic stack.
At best it leads to a Defer leak and incorrect output on a subsequent panic.
At worst it arbitrary corrupts heap.

LGTM=rsc
R=rsc
CC=golang-codereviews
https://codereview.appspot.com/72480043