runtime: improve invalid pointer error message

By default, the runtime panics if it detects a pointer to an
unallocated span. At this point, this usually catches bad uses of
unsafe or cgo in user code (though it could also catch runtime bugs).
Unfortunately, the rather cryptic error misleads users, offers users
little help with debugging their own problem, and offers the Go
developers little help with root-causing.

Improve the error message in various ways. First, the wording is
improved to make it clearer what condition was detected and to suggest
that this may be the result of incorrect use of unsafe or cgo. Second,
we add a dump of the object containing the bad pointer so that there's
at least some hope of figuring out why a bad pointer was stored in the
Go heap.

Change-Id: I57b91b12bc3cb04476399d7706679e096ce594b9
Reviewed-on: https://go-review.googlesource.com/14763
Reviewed-by: Rick Hudson <rlh@golang.org>

diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go
index 8efce4da2d4d5b626e6163dafa205f5b6f1bec1e..46e7549d592363b8de11d7d5e62550d1979211a9 100644 (file)
--- a/src/runtime/crash_test.go
+++ b/src/runtime/crash_test.go
@@ -587,3 +587,40 @@ func main() {
        fmt.Println("done")
 }
 `
+
+func TestInvalidptrCrash(t *testing.T) {
+       output := executeTest(t, invalidptrCrashSource, nil)
+       // Check that the bad pointer was detected.
+       want1 := "found bad pointer in Go heap"
+       if !strings.Contains(output, want1) {
+               t.Fatalf("failed to detect bad pointer; output does not contain %q:\n%s", want1, output)
+       }
+       // Check that we dumped the object containing the bad pointer.
+       want2 := "*(object+0) = 0x12345678"
+       if !strings.Contains(output, want2) {
+               t.Fatalf("failed to dump source object; output does not contain %q:\n%s", want2, output)
+       }
+}
+
+const invalidptrCrashSource = `
+package main
+import (
+       "runtime"
+       "unsafe"
+)
+var x = new(struct {
+       magic uintptr
+       y *byte
+})
+func main() {
+       runtime.GC()
+       x.magic = 0x12345678
+       x.y = &make([]byte, 64*1024)[0]
+       weasel := uintptr(unsafe.Pointer(x.y))
+       x.y = nil
+       runtime.GC()
+       x.y = (*byte)(unsafe.Pointer(weasel))
+       runtime.GC()
+       println("failed to detect bad pointer")
+}
+`

diff --git a/src/runtime/mbitmap.go b/src/runtime/mbitmap.go
index fd6b4b11a2f8e31eada566981e03cf9f1968c369..6998b14e7a9938124bda00a4b45e7b32440273ce 100644 (file)
--- a/src/runtime/mbitmap.go
+++ b/src/runtime/mbitmap.go
@@ -182,7 +182,11 @@ func heapBitsForSpan(base uintptr) (hbits heapBits) {
 // If p does not point into a heap object,
 // return base == 0
 // otherwise return the base of the object.
-func heapBitsForObject(p uintptr) (base uintptr, hbits heapBits, s *mspan) {
+//
+// refBase and refOff optionally give the base address of the object
+// in which the pointer p was found and the byte offset at which it
+// was found. These are used for error reporting.
+func heapBitsForObject(p, refBase, refOff uintptr) (base uintptr, hbits heapBits, s *mspan) {
        arenaStart := mheap_.arena_start
        if p < arenaStart || p >= mheap_.arena_used {
                return
@@ -203,18 +207,28 @@ func heapBitsForObject(p uintptr) (base uintptr, hbits heapBits, s *mspan) {
 
                // The following ensures that we are rigorous about what data
                // structures hold valid pointers.
-               // TODO(rsc): Check if this still happens.
                if debug.invalidptr != 0 {
-                       // Still happens sometimes. We don't know why.
+                       // Typically this indicates an incorrect use
+                       // of unsafe or cgo to store a bad pointer in
+                       // the Go heap. It may also indicate a runtime
+                       // bug.
+                       //
+                       // TODO(austin): We could be more aggressive
+                       // and detect pointers to unallocated objects
+                       // in allocated spans.
                        printlock()
-                       print("runtime:objectstart Span weird: p=", hex(p), " k=", hex(k))
-                       if s == nil {
-                               print(" s=nil\n")
+                       print("runtime: pointer ", hex(p))
+                       if s.state != mSpanInUse {
+                               print(" to unallocated span")
                        } else {
-                               print(" s.start=", hex(s.start<<_PageShift), " s.limit=", hex(s.limit), " s.state=", s.state, "\n")
+                               print(" to unused region of span")
+                       }
+                       print("idx=", hex(idx), " span.start=", hex(s.start<<_PageShift), " span.limit=", hex(s.limit), " span.state=", s.state, "\n")
+                       if refBase != 0 {
+                               print("runtime: found in object at *(", hex(refBase), "+", hex(off), ")\n")
+                               gcDumpObject("object", refBase, refOff)
                        }
-                       printunlock()
-                       throw("objectstart: bad pointer in unexpected span")
+                       throw("found bad pointer in Go heap (incorrect use of unsafe or cgo?)")
                }
                return
        }

diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go
index c8f8a66b45866ea0b94bd45c171eb4621f550a00..eb47e0918712ec9deba7e20156539595312bcea6 100644 (file)
--- a/src/runtime/mgcmark.go
+++ b/src/runtime/mgcmark.go
@@ -659,7 +659,7 @@ func scanblock(b0, n0 uintptr, ptrmask *uint8, gcw *gcWork) {
                                // Same work as in scanobject; see comments there.
                                obj := *(*uintptr)(unsafe.Pointer(b + i))
                                if obj != 0 && arena_start <= obj && obj < arena_used {
-                                       if obj, hbits, span := heapBitsForObject(obj); obj != 0 {
+                                       if obj, hbits, span := heapBitsForObject(obj, b, i); obj != 0 {
                                                greyobject(obj, b, i, hbits, span, gcw)
                                        }
                                }
@@ -725,7 +725,7 @@ func scanobject(b uintptr, gcw *gcWork) {
                // Check if it points into heap and not back at the current object.
                if obj != 0 && arena_start <= obj && obj < arena_used && obj-b >= n {
                        // Mark the object.
-                       if obj, hbits, span := heapBitsForObject(obj); obj != 0 {
+                       if obj, hbits, span := heapBitsForObject(obj, b, i); obj != 0 {
                                greyobject(obj, b, i, hbits, span, gcw)
                        }
                }
@@ -739,7 +739,7 @@ func scanobject(b uintptr, gcw *gcWork) {
 // Preemption must be disabled.
 //go:nowritebarrier
 func shade(b uintptr) {
-       if obj, hbits, span := heapBitsForObject(b); obj != 0 {
+       if obj, hbits, span := heapBitsForObject(b, 0, 0); obj != 0 {
                gcw := &getg().m.p.ptr().gcw
                greyobject(obj, 0, 0, hbits, span, gcw)
                if gcphase == _GCmarktermination || gcBlackenPromptly {
@@ -810,7 +810,7 @@ func greyobject(obj, base, off uintptr, hbits heapBits, span *mspan, gcw *gcWork
 // field at byte offset off in obj.
 func gcDumpObject(label string, obj, off uintptr) {
        if obj < mheap_.arena_start || obj >= mheap_.arena_used {
-               print(label, "=", hex(obj), " is not a heap object\n")
+               print(label, "=", hex(obj), " is not in the Go heap\n")
                return
        }
        k := obj >> _PageShift