From: Carlos Amedee <carlos@golang.org>
Date: Wed, 20 Nov 2024 22:20:41 +0000 (-0500)
Subject: runtime: properly search for cleanups in cleanup.stop
X-Git-Tag: go1.24rc1~87
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=7f049eac1b9378ecc4dddd43ebedeae0916c0606;p=gostls13.git

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 db1a6ac67e..04d8ff59aa 100644
--- 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 4b9734da5f..4fcfbeca84 100644
--- 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