runtime: properly search for cleanups in cleanup.stop

This change modifies the logic which searches for existing cleanups.
The existing search logic sets the next node to the current node
in certain conditions. This would cause future searches to loop
endlessly. The existing loop could convert non-cleanup specials into
cleanups and cause data corruption.

This also changes where we release the m while we are adding a
cleanup. We are currently holding onto an p-specific gcwork after
releasing the m.

Change-Id: I0ac0b304f40910549c8df114e523c89d9f0d7a75
Reviewed-on: https://go-review.googlesource.com/c/go/+/630278
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Carlos Amedee <carlos@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

diff --git a/src/runtime/mcleanup.go b/src/runtime/mcleanup.go
index db1a6ac67e2125b95d420860a4abbf5701d24203..04d8ff59aab3c1ee226a54a1ebeba5777a55b8f0 100644 (file)
--- a/src/runtime/mcleanup.go
+++ b/src/runtime/mcleanup.go
@@ -131,12 +131,26 @@ func (c Cleanup) Stop() {
 
        iter, exists := span.specialFindSplicePoint(offset, _KindSpecialCleanup)
        if exists {
-               for s := *iter; s != nil && offset == uintptr(s.offset); iter = &s.next {
-                       if (*specialCleanup)(unsafe.Pointer(s)).id == c.id {
+               for {
+                       s := *iter
+                       if s == nil {
+                               // Reached the end of the linked list. Stop searching at this point.
+                               break
+                       }
+                       if offset == uintptr(s.offset) && _KindSpecialCleanup == s.kind &&
+                               (*specialCleanup)(unsafe.Pointer(s)).id == c.id {
+                               // The special is a cleanup and contains a matching cleanup id.
                                *iter = s.next
                                found = s
                                break
                        }
+                       if offset < uintptr(s.offset) || (offset == uintptr(s.offset) && _KindSpecialCleanup < s.kind) {
+                               // The special is outside the region specified for that kind of
+                               // special. The specials are sorted by kind.
+                               break
+                       }
+                       // Try the next special.
+                       iter = &s.next
                }
        }
        if span.specials == nil {

diff --git a/src/runtime/mheap.go b/src/runtime/mheap.go
index 4b9734da5f6465bfdd5481748b83f4f4d885f5c0..4fcfbeca84c0a512a5b805516362dcacb5c4b70f 100644 (file)
--- a/src/runtime/mheap.go
+++ b/src/runtime/mheap.go
@@ -2046,7 +2046,6 @@ func addCleanup(p unsafe.Pointer, f *funcval) uint64 {
 
        mp := acquirem()
        addspecial(p, &s.special, true)
-       releasem(mp)
        // This is responsible for maintaining the same
        // GC-related invariants as markrootSpans in any
        // situation where it's possible that markrootSpans
@@ -2057,6 +2056,7 @@ func addCleanup(p unsafe.Pointer, f *funcval) uint64 {
                // special isn't part of the GC'd heap.
                scanblock(uintptr(unsafe.Pointer(&s.fn)), goarch.PtrSize, &oneptrmask[0], gcw, nil)
        }
+       releasem(mp)
        // Keep f alive. There's a window in this function where it's
        // only reachable via the special while the special hasn't been
        // added to the specials list yet. This is similar to a bug