From bed2b7cf41471e1521af5a83ae28bd643eb3e038 Mon Sep 17 00:00:00 2001 From: Cherry Mui Date: Wed, 9 Nov 2022 10:44:36 -0500 Subject: [PATCH] runtime: delay incrementing freeindex in malloc When the GC is scanning some memory (possibly conservatively), finding a pointer, while concurrently another goroutine is allocating an object at the same address as the found pointer, the GC may see the pointer before the object and/or the heap bits are initialized. This may cause the GC to see bad pointers and possibly crash. To prevent this, we make it that the scanner can only see the object as allocated after the object and the heap bits are initialized. As the scanner uses the freeindex to determine if an object is allocated, we delay the increment of freeindex after the initialization. As currently in some code path finding the next free index and updating the free index to a new slot past it is coupled, this needs a small refactoring. In the new code mspan.nextFreeIndex return the next free index but not update it (although allocCache is updated). mallocgc will update it at a later time. Fixes #54596. Change-Id: I6dd5ccf743f2d2c46a1ed67c6a8237fe09a71260 Reviewed-on: https://go-review.googlesource.com/c/go/+/427619 TryBot-Result: Gopher Robot Run-TryBot: Cherry Mui Reviewed-by: Michael Knyszek --- src/runtime/malloc.go | 40 +++++++++++++++++++++++----------------- src/runtime/mbitmap.go | 17 +++++++++++++---- src/runtime/mcentral.go | 1 - 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/runtime/malloc.go b/src/runtime/malloc.go index 70a13d0576..c7335c55c6 100644 --- a/src/runtime/malloc.go +++ b/src/runtime/malloc.go @@ -813,24 +813,22 @@ retry: // base address for all 0-byte allocations var zerobase uintptr -// nextFreeFast returns the next free object if one is quickly available. -// Otherwise it returns 0. -func nextFreeFast(s *mspan) gclinkptr { +// nextFreeFast returns the next free object if one is quickly available, +// and the corresponding free index. Otherwise it returns 0, 0. +func nextFreeFast(s *mspan) (gclinkptr, uintptr) { theBit := sys.TrailingZeros64(s.allocCache) // Is there a free object in the allocCache? if theBit < 64 { result := s.freeindex + uintptr(theBit) if result < s.nelems { - freeidx := result + 1 - if freeidx%64 == 0 && freeidx != s.nelems { - return 0 - } s.allocCache >>= uint(theBit + 1) - s.freeindex = freeidx + // NOTE: s.freeindex is not updated for now (although allocCache + // is updated). mallocgc will update s.freeindex later after the + // memory is initialized. s.allocCount++ - return gclinkptr(result*s.elemsize + s.base()) + return gclinkptr(result*s.elemsize + s.base()), result } } - return 0 + return 0, 0 } // nextFree returns the next free object from the cached span if one is available. @@ -842,10 +840,10 @@ func nextFreeFast(s *mspan) gclinkptr { // // Must run in a non-preemptible context since otherwise the owner of // c could change. -func (c *mcache) nextFree(spc spanClass) (v gclinkptr, s *mspan, shouldhelpgc bool) { +func (c *mcache) nextFree(spc spanClass) (v gclinkptr, s *mspan, freeIndex uintptr, shouldhelpgc bool) { s = c.alloc[spc] shouldhelpgc = false - freeIndex := s.nextFreeIndex() + freeIndex = s.nextFreeIndex() if freeIndex == s.nelems { // The span is full. if uintptr(s.allocCount) != s.nelems { @@ -953,6 +951,7 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { // In some cases block zeroing can profitably (for latency reduction purposes) // be delayed till preemption is possible; delayedZeroing tracks that state. delayedZeroing := false + var freeidx uintptr if size <= maxSmallSize { if noscan && size < maxTinySize { // Tiny allocator. @@ -1012,9 +1011,10 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { } // Allocate a new maxTinySize block. span = c.alloc[tinySpanClass] - v := nextFreeFast(span) + var v gclinkptr + v, freeidx = nextFreeFast(span) if v == 0 { - v, span, shouldhelpgc = c.nextFree(tinySpanClass) + v, span, freeidx, shouldhelpgc = c.nextFree(tinySpanClass) } x = unsafe.Pointer(v) (*[2]uint64)(x)[0] = 0 @@ -1037,9 +1037,10 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { size = uintptr(class_to_size[sizeclass]) spc := makeSpanClass(sizeclass, noscan) span = c.alloc[spc] - v := nextFreeFast(span) + var v gclinkptr + v, freeidx = nextFreeFast(span) if v == 0 { - v, span, shouldhelpgc = c.nextFree(spc) + v, span, freeidx, shouldhelpgc = c.nextFree(spc) } x = unsafe.Pointer(v) if needzero && span.needzero != 0 { @@ -1051,7 +1052,7 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { // For large allocations, keep track of zeroed state so that // bulk zeroing can be happen later in a preemptible context. span = c.allocLarge(size, noscan) - span.freeindex = 1 + freeidx = 0 span.allocCount = 1 size = span.elemsize x = unsafe.Pointer(span.base()) @@ -1093,6 +1094,11 @@ func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer { // but see uninitialized memory or stale heap bits. publicationBarrier() + // As x and the heap bits are initialized, update + // freeindx now so x is seen by the GC (including + // convervative scan) as an allocated object. + span.updateFreeIndex(freeidx + 1) + // Allocate black during GC. // All slots hold nil so no scanning is needed. // This may be racing with GC so do it atomically if there can be diff --git a/src/runtime/mbitmap.go b/src/runtime/mbitmap.go index dc99ba768b..8fee8262b7 100644 --- a/src/runtime/mbitmap.go +++ b/src/runtime/mbitmap.go @@ -132,7 +132,8 @@ func (s *mspan) refillAllocCache(whichByte uintptr) { } // nextFreeIndex returns the index of the next free object in s at -// or after s.freeindex. +// or after s.freeindex. s.freeindex is not updated (except the full +// span case), but the alloc cache is updated. // There are hardware instructions that can be used to make this // faster if profiling warrants it. func (s *mspan) nextFreeIndex() uintptr { @@ -170,9 +171,18 @@ func (s *mspan) nextFreeIndex() uintptr { } s.allocCache >>= uint(bitIndex + 1) - sfreeindex = result + 1 - if sfreeindex%64 == 0 && sfreeindex != snelems { + // NOTE: s.freeindex is not updated for now (although allocCache + // is updated). mallocgc will update s.freeindex later after the + // memory is initialized. + + return result +} + +// updateFreeIndex updates s.freeindex to sfreeindex, refills +// the allocCache if necessary. +func (s *mspan) updateFreeIndex(sfreeindex uintptr) { + if sfreeindex%64 == 0 && sfreeindex != s.nelems { // We just incremented s.freeindex so it isn't 0. // As each 1 in s.allocCache was encountered and used for allocation // it was shifted away. At this point s.allocCache contains all 0s. @@ -182,7 +192,6 @@ func (s *mspan) nextFreeIndex() uintptr { s.refillAllocCache(whichByte) } s.freeindex = sfreeindex - return result } // isFree reports whether the index'th object in s is unallocated. diff --git a/src/runtime/mcentral.go b/src/runtime/mcentral.go index 3382c54e7f..6621af5f78 100644 --- a/src/runtime/mcentral.go +++ b/src/runtime/mcentral.go @@ -146,7 +146,6 @@ func (c *mcentral) cacheSpan() *mspan { // Check if there's any free space. freeIndex := s.nextFreeIndex() if freeIndex != s.nelems { - s.freeindex = freeIndex sweep.active.end(sl) goto havespan } -- 2.50.0