From: Russ Cox <rsc@golang.org>
Date: Wed, 20 May 2015 02:58:10 +0000 (-0400)
Subject: runtime: fix callwritebarrier
X-Git-Tag: go1.5beta1~500
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=001438bdfec2b97e04a053abfbe42efe499f78e5;p=gostls13.git

runtime: fix callwritebarrier

Given a call frame F of size N where the return values start at offset R,
callwritebarrier was instructing heapBitsBulkBarrier to scan the block
of memory [F+R, F+R+N). It should only scan [F+R, F+N). The extra N-R
bytes scanned might lead into the next allocated block in memory.
Because the scan was consulting the heap bitmap for type information,
scanning into the next block normally "just worked" in the sense of
not crashing.

Scanning the extra N-R bytes of memory is a problem mainly because
it causes the GC to consider pointers that might otherwise not be
considered, leading it to retain objects that should actually be freed.
This is very difficult to detect.

Luckily, juju turned up a case where the heap bitmap and the memory
were out of sync for the block immediately after the call frame, so that
heapBitsBulkBarrier saw an obvious non-pointer where it expected a
pointer, causing a loud crash.

Why is there a non-pointer in memory that the heap bitmap records as
a pointer? That is more difficult to answer. At least one way that it
could happen is that allocations containing no pointers at all do not
update the heap bitmap. So if heapBitsBulkBarrier walked out of the
current object and into a no-pointer object and consulted those bitmap
bits, it would be misled. This doesn't happen in general because all
the paths to heapBitsBulkBarrier first check for the no-pointer case.
This may or may not be what happened, but it's the only scenario
I've been able to construct.

I tried for quite a while to write a simple test for this and could not.
It does fix the juju crash, and it is clearly an improvement over the
old code.

Fixes #10844.

Change-Id: I53982c93ef23ef93155c4086bbd95a4c4fdaac9a
Reviewed-on: https://go-review.googlesource.com/10317
Reviewed-by: Austin Clements <austin@google.com>
---

diff --git a/src/runtime/mbarrier.go b/src/runtime/mbarrier.go
index 77b50095a0..53a0a00ae7 100644
--- a/src/runtime/mbarrier.go
+++ b/src/runtime/mbarrier.go
@@ -195,7 +195,7 @@ func callwritebarrier(typ *_type, frame unsafe.Pointer, framesize, retoffset uin
 	if !writeBarrierEnabled || typ == nil || typ.kind&kindNoPointers != 0 || framesize-retoffset < ptrSize || !inheap(uintptr(frame)) {
 		return
 	}
-	heapBitsBulkBarrier(uintptr(add(frame, retoffset)), framesize)
+	heapBitsBulkBarrier(uintptr(add(frame, retoffset)), framesize-retoffset)
 }
 
 //go:nosplit
diff --git a/src/runtime/mbitmap.go b/src/runtime/mbitmap.go
index 39bb4217b3..b20908fb49 100644
--- a/src/runtime/mbitmap.go
+++ b/src/runtime/mbitmap.go
@@ -356,6 +356,12 @@ func (h heapBits) setCheckmarked(size uintptr) {
 // calling memmove(p, src, size). This function is marked nosplit
 // to avoid being preempted; the GC must not stop the goroutine
 // betwen the memmove and the execution of the barriers.
+//
+// The heap bitmap is not maintained for allocations containing
+// no pointers at all; any caller of heapBitsBulkBarrier must first
+// make sure the underlying allocation contains pointers, usually
+// by checking typ.kind&kindNoPointers.
+//
 //go:nosplit
 func heapBitsBulkBarrier(p, size uintptr) {
 	if (p|size)&(ptrSize-1) != 0 {
diff --git a/src/runtime/mheap.go b/src/runtime/mheap.go
index a610da2e47..04fa050bc5 100644
--- a/src/runtime/mheap.go
+++ b/src/runtime/mheap.go
@@ -168,7 +168,9 @@ func recordspan(vh unsafe.Pointer, p unsafe.Pointer) {
 
 // inheap reports whether b is a pointer into a (potentially dead) heap object.
 // It returns false for pointers into stack spans.
+// Non-preemptible because it is used by write barriers.
 //go:nowritebarrier
+//go:nosplit
 func inheap(b uintptr) bool {
 	if b == 0 || b < mheap_.arena_start || b >= mheap_.arena_used {
 		return false