code review 167060043: runtime: avoid gentraceback of self on user goroutine stack

runtime: avoid gentraceback of self on user goroutine stack

Gentraceback may grow the stack.
One of the gentraceback wrappers may grow the stack.
One of the gentraceback callback calls may grow the stack.
Various stack pointers are stored in various stack locations
as type uintptr during the execution of these calls.
If the stack does grow, these stack pointers will not be
updated and will start trying to decode stack memory that
is no longer valid.

It may be possible to change the type of the stack pointer
variables to be unsafe.Pointer, but that's pretty subtle and
may still have problems, even if we catch every last one.
An easier, more obviously correct fix is to require that
gentraceback of the currently running goroutine must run
on the g0 stack, not on the goroutine's own stack.

Not doing this causes faults when you set
        StackFromSystem = 1
        StackFaultOnFree = 1

The new check in gentraceback will catch future lapses.

The more general problem is calling getcallersp but then
calling a function that might relocate the stack, which would
invalidate the result of getcallersp. Add note to stubs.go
declaration of getcallersp explaining the problem, and
check all existing calls to getcallersp. Most needed fixes.

This affects Callers, Stack, and nearly all the runtime
profiling routines. It does not affect stack copying directly
nor garbage collection.

[rsc, 2014-11-02 15:31:05]

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

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

[bradfitz, 2014-11-02 16:35:25]

its instead of it's in two places.
On Nov 2, 2014 12:31 PM, <rsc@golang.org> wrote:

> Reviewers: khr,
>
> Message:
> Hello khr (cc: golang-codereviews@googlegroups.com, r),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: avoid gentraceback of self on user goroutine stack
>
> Gentraceback may grow the stack.
> One of the gentraceback wrappers may grow the stack.
> One of the gentraceback callback calls may grow the stack.
> Various stack pointers are stored in various stack locations
> as type uintptr during the execution of these calls.
> If the stack does grow, these stack pointers will not be
> updated and will start trying to decode stack memory that
> is no longer valid.
>
> It may be possible to change the type of the stack pointer
> variables to be unsafe.Pointer, but that's pretty subtle and
> may still have problems, even if we catch every last one.
> An easier, more obviously correct fix is to require that
> gentraceback of the currently running goroutine must run
> on the g0 stack, not on the goroutine's own stack.
>
> Not doing this causes faults when you set
>         StackFromSystem = 1
>         StackFaultOnFree = 1
>
> The new check in gentraceback will catch future lapses.
>
> The more general problem is calling getcallersp but then
> calling a function that might relocate the stack, which would
> invalidate the result of getcallersp. Add note to stubs.go
> declaration of getcallersp explaining the problem, and
> check all existing calls to getcallersp. Most needed fixes.
>
> This affects Callers, Stack, and nearly all the runtime
> profiling routines. It does not affect stack copying directly
> nor garbage collection.
>
> Please review this at https://codereview.appspot.com/167060043/
>
> Affected files (+73, -9 lines):
>   M src/runtime/mprof.go
>   M src/runtime/stubs.go
>   M src/runtime/traceback.go
>
>
> Index: src/runtime/mprof.go
> ===================================================================
> --- a/src/runtime/mprof.go
> +++ b/src/runtime/mprof.go
> @@ -528,8 +528,6 @@
>  // Most clients should use the runtime/pprof package instead
>  // of calling GoroutineProfile directly.
>  func GoroutineProfile(p []StackRecord) (n int, ok bool) {
> -       sp := getcallersp(unsafe.Pointer(&p))
> -       pc := getcallerpc(unsafe.Pointer(&p))
>
>         n = NumGoroutine()
>         if n <= len(p) {
> @@ -542,7 +540,11 @@
>                 if n <= len(p) {
>                         ok = true
>                         r := p
> -                       saveg(pc, sp, gp, &r[0])
> +                       sp := getcallersp(unsafe.Pointer(&p))
> +                       pc := getcallerpc(unsafe.Pointer(&p))
> +                       onM(func() {
> +                               saveg(pc, sp, gp, &r[0])
> +                       })
>                         r = r[1:]
>                         for _, gp1 := range allgs {
>                                 if gp1 == gp || readgstatus(gp1) == _Gdead
> {
> @@ -573,8 +575,6 @@
>  // If all is true, Stack formats stack traces of all other goroutines
>  // into buf after the trace for the current goroutine.
>  func Stack(buf []byte, all bool) int {
> -       sp := getcallersp(unsafe.Pointer(&buf))
> -       pc := getcallerpc(unsafe.Pointer(&buf))
>         mp := acquirem()
>         gp := mp.curg
>         if all {
> @@ -591,7 +591,15 @@
>         if len(buf) > 0 {
>                 gp.writebuf = buf[0:0:len(buf)]
>                 goroutineheader(gp)
> -               traceback(pc, sp, 0, gp)
> +               sp := getcallersp(unsafe.Pointer(&buf))
> +               pc := getcallerpc(unsafe.Pointer(&buf))
> +               onM(func() {
> +                       g0 := getg()
> +                       g0.writebuf = gp.writebuf
> +                       traceback(pc, sp, 0, gp)
> +                       gp.writebuf = g0.writebuf
> +                       g0.writebuf = nil
> +               })
>                 if all {
>                         tracebackothers(gp)
>                 }
> @@ -623,7 +631,11 @@
>         }
>         if gp.m.curg == nil || gp == gp.m.curg {
>                 goroutineheader(gp)
> -               traceback(getcallerpc(unsafe.Pointer(&p)),
> getcallersp(unsafe.Pointer(&p)), 0, gp)
> +               pc := getcallerpc(unsafe.Pointer(&p))
> +               sp := getcallersp(unsafe.Pointer(&p))
> +               onM(func() {
> +                       traceback(pc, sp, 0, gp)
> +               })
>         } else {
>                 goroutineheader(gp.m.curg)
>                 traceback(^uintptr(0), ^uintptr(0), 0, gp.m.curg)
> @@ -639,7 +651,11 @@
>         gp.m.traceback = 2
>         print("tracefree(", p, ", ", hex(size), ")\n")
>         goroutineheader(gp)
> -       traceback(getcallerpc(unsafe.Pointer(&p)),
> getcallersp(unsafe.Pointer(&p)), 0, gp)
> +       pc := getcallerpc(unsafe.Pointer(&p))
> +       sp := getcallersp(unsafe.Pointer(&p))
> +       onM(func() {
> +               traceback(pc, sp, 0, gp)
> +       })
>         print("\n")
>         gp.m.traceback = 0
>         unlock(&tracelock)
> Index: src/runtime/stubs.go
> ===================================================================
> --- a/src/runtime/stubs.go
> +++ b/src/runtime/stubs.go
> @@ -221,6 +221,34 @@
>  //go:noescape
>  func setcallerpc(argp unsafe.Pointer, pc uintptr)
>
> +// getcallerpc returns the program counter (PC) of it's caller's caller.
> +// getcallersp returns the stack pointer (SP) of it's caller's caller.
> +// For both, the argp must be a pointer to the caller's first function
> argument.
> +// The implementation may or may not use argp, depending on
> +// the architecture.
> +//
> +// For example:
> +//
> +//     func f(arg1, arg2, arg3 int) {
> +//             pc := getcallerpc(unsafe.Pointer(&arg1))
> +//             sp := getcallerpc(unsafe.Pointer(&arg2))
> +//     }
> +//
> +// These two lines find the PC and SP immediately following
> +// the call to f (where f will return).
> +//
> +// The call to getcallerpc and getcallersp must be done in the
> +// frame being asked out. It would not be correct for f to pass &arg1
> +// to another function g and let g call getcallerpc/getcallersp.
> +// The call inside g might return information about g's caller or
> +// information about f's caller or complete garbage.
> +//
> +// The result of getcallersp is correct at the time of the return,
> +// but it may be invalidated by any subsequent call to a function
> +// that might relocate the stack in order to grow or shrink it.
> +// A general rule is that the result of getcallersp should be used
> +// immediately and can only be passed to nosplit functions.
> +
>  //go:noescape
>  func getcallerpc(argp unsafe.Pointer) uintptr
>
> Index: src/runtime/traceback.go
> ===================================================================
> --- a/src/runtime/traceback.go
> +++ b/src/runtime/traceback.go
> @@ -101,6 +101,22 @@
>                 gothrow("gentraceback before goexitPC initialization")
>         }
>         g := getg()
> +       if g == gp && g == g.m.curg {
> +               // The starting sp has been passed in as a uintptr, and
> the caller may
> +               // have other uintptr-typed stack references as well.
> +               // If during one of the calls that got us here or during
> one of the
> +               // callbacks below the stack must be grown, all these
> uintptr references
> +               // to the stack will not be updated, and gentraceback will
> continue
> +               // to inspect the old stack memory, which may no longer be
> valid.
> +               // Even if all the variables were updated correctly, it is
> not clear that
> +               // we want to expose a traceback that begins on one stack
> and ends
> +               // on another stack. That could confuse callers quite a
> bit.
> +               // Instead, we require that gentraceback and any other
> function that
> +               // accepts an sp for the current goroutine (typically
> obtained by
> +               // calling getcallersp) must not run on that goroutine's
> stack but
> +               // instead on the g0 stack.
> +               gothrow("gentraceback cannot trace user goroutine on its
> own stack")
> +       }
>         gotraceback := gotraceback(nil)
>         if pc0 == ^uintptr(0) && sp0 == ^uintptr(0) { // Signal to fetch
> saved values from gp.
>                 if gp.syscallsp != 0 {
> @@ -511,7 +527,11 @@
>  func callers(skip int, pcbuf *uintptr, m int) int {
>         sp := getcallersp(unsafe.Pointer(&skip))
>         pc := uintptr(getcallerpc(unsafe.Pointer(&skip)))
> -       return gentraceback(pc, sp, 0, getg(), skip, pcbuf, m, nil, nil, 0)
> +       var n int
> +       onM(func() {
> +               n = gentraceback(pc, sp, 0, getg(), skip, pcbuf, m, nil,
> nil, 0)
> +       })
> +       return n
>  }
>
>  func gcallers(gp *g, skip int, pcbuf *uintptr, m int) int {
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[khr, 2014-11-03 17:12:44]

LGTM.

https://codereview.appspot.com/167060043/diff/60001/src/runtime/mprof.go
File src/runtime/mprof.go (right):

https://codereview.appspot.com/167060043/diff/60001/src/runtime/mprof.go#newc...
src/runtime/mprof.go:598: g0.writebuf = gp.writebuf
This is kind of strange, to write gp.writebuf = buf, then g0.writebuf =
gp.writebuf.  Any way to do it directly?  I can't think of anything offhand, as
using acquirem().*arg is probably just as ugly.

https://codereview.appspot.com/167060043/diff/60001/src/runtime/stubs.go
File src/runtime/stubs.go (right):

https://codereview.appspot.com/167060043/diff/60001/src/runtime/stubs.go#newc...
src/runtime/stubs.go:241: // frame being asked out. It would not be correct for
f to pass &arg1
s/out/about/

[rsc, 2014-11-06 04:01:55]

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

runtime: avoid gentraceback of self on user goroutine stack

Gentraceback may grow the stack.
One of the gentraceback wrappers may grow the stack.
One of the gentraceback callback calls may grow the stack.
Various stack pointers are stored in various stack locations
as type uintptr during the execution of these calls.
If the stack does grow, these stack pointers will not be
updated and will start trying to decode stack memory that
is no longer valid.

It may be possible to change the type of the stack pointer
variables to be unsafe.Pointer, but that's pretty subtle and
may still have problems, even if we catch every last one.
An easier, more obviously correct fix is to require that
gentraceback of the currently running goroutine must run
on the g0 stack, not on the goroutine's own stack.

Not doing this causes faults when you set
        StackFromSystem = 1
        StackFaultOnFree = 1

The new check in gentraceback will catch future lapses.

The more general problem is calling getcallersp but then
calling a function that might relocate the stack, which would
invalidate the result of getcallersp. Add note to stubs.go
declaration of getcallersp explaining the problem, and
check all existing calls to getcallersp. Most needed fixes.

This affects Callers, Stack, and nearly all the runtime
profiling routines. It does not affect stack copying directly
nor garbage collection.

LGTM=khr
R=khr, bradfitz
CC=golang-codereviews, r
https://codereview.appspot.com/167060043

https://codereview.appspot.com/167060043/diff/60001/src/runtime/mprof.go
File src/runtime/mprof.go (right):

https://codereview.appspot.com/167060043/diff/60001/src/runtime/mprof.go#newc...
src/runtime/mprof.go:598: g0.writebuf = gp.writebuf
It's not for argument passing. (FWIW, there's never any need for
scalararg/ptrarg in Go code. Closures take care of the variable sharing
automatically.) It's because some output is generated on the current goroutine -
goroutineheader is called before onM, tracebackothers after - and some output is
generated on the g0 goroutine. When we switch goroutines we have to carry the
output buffer with us. I was trying to minimize the amount done in the onM call,
but I guess it really doesn't matter. I moved all the output into the onM, and
now nothing needs to mention gp.writebuf.

https://codereview.appspot.com/167060043/diff/60001/src/runtime/stubs.go
File src/runtime/stubs.go (right):

https://codereview.appspot.com/167060043/diff/60001/src/runtime/stubs.go#newc...
src/runtime/stubs.go:241: // frame being asked out. It would not be correct for
f to pass &arg1
On 2014/11/03 17:12:44, khr wrote:
> s/out/about/

Done.

[rsc, 2014-11-06 14:57:44]

Fixed issue 9044.