runtime: ensure weak handles end up in their own allocation

Currently weak handles are atomic.Uintptr values that may end up in
a tiny block which can cause all sorts of surprising leaks. See #76007
for one example.

This change pads out the underlying allocation of the atomic.Uintptr to
16 bytes to ensure we bypass the tiny allocator, and it gets its own
block. This wastes 8 bytes per weak handle. We could potentially do
better by using the 8 byte noscan size class, but this can be a
follow-up change.

For #76007.

Change-Id: I3ab74dda9bf312ea0007f167093052de28134944
Reviewed-on: https://go-review.googlesource.com/c/go/+/719960
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/runtime/mheap.go b/src/runtime/mheap.go
index 711c7790eb76bb2e5a0e056afeb5670f1cc630a4..3334099092a4c607f4f56036b1c838c4f1f8418c 100644 (file)
--- a/src/runtime/mheap.go
+++ b/src/runtime/mheap.go
@@ -2534,7 +2534,15 @@ func getOrAddWeakHandle(p unsafe.Pointer) *atomic.Uintptr {
        s := (*specialWeakHandle)(mheap_.specialWeakHandleAlloc.alloc())
        unlock(&mheap_.speciallock)
 
-       handle := new(atomic.Uintptr)
+       // N.B. Pad the weak handle to ensure it doesn't share a tiny
+       // block with any other allocations. This can lead to leaks, such
+       // as in go.dev/issue/76007. As an alternative, we could consider
+       // using the currently-unused 8-byte noscan size class.
+       type weakHandleBox struct {
+               h atomic.Uintptr
+               _ [maxTinySize - unsafe.Sizeof(atomic.Uintptr{})]byte
+       }
+       handle := &(new(weakHandleBox).h)
        s.special.kind = _KindSpecialWeakHandle
        s.handle = handle
        handle.Store(uintptr(p))

diff --git a/src/weak/pointer_test.go b/src/weak/pointer_test.go
index da464a8d01f2d010b8d97d52e58729f113e7d71b..5e8b9bef5860e28bfedfa7748c6e8d6294642e11 100644 (file)
--- a/src/weak/pointer_test.go
+++ b/src/weak/pointer_test.go
@@ -16,8 +16,10 @@ import (
 )
 
 type T struct {
-       // N.B. This must contain a pointer, otherwise the weak handle might get placed
-       // in a tiny block making the tests in this package flaky.
+       // N.B. T is what it is to avoid having test values get tiny-allocated
+       // in the same block as the weak handle, but since the fix to
+       // go.dev/issue/76007, this should no longer be possible.
+       // TODO(mknyszek): Consider using tiny-allocated values for all the tests.
        t *T
        a int
        b int
@@ -327,3 +329,37 @@ func TestImmortalPointer(t *testing.T) {
                t.Errorf("immortal weak pointer to %p has unexpected Value %p", want, got)
        }
 }
+
+func TestPointerTiny(t *testing.T) {
+       runtime.GC() // Clear tiny-alloc caches.
+
+       const N = 1000
+       wps := make([]weak.Pointer[int], N)
+       for i := range N {
+               // N.B. *x is just an int, so the value is very likely
+               // tiny-allocated alongside the weak handle, assuming bug
+               // from go.dev/issue/76007 exists.
+               x := new(int)
+               *x = i
+               wps[i] = weak.Make(x)
+       }
+
+       // Get the cleanups to start running.
+       runtime.GC()
+
+       // Expect at least 3/4ths of the weak pointers to have gone nil.
+       //
+       // Note that we provide some leeway since it's possible our allocation
+       // gets grouped with some other long-lived tiny allocation, but this
+       // shouldn't be the case for the vast majority of allocations.
+       n := 0
+       for _, wp := range wps {
+               if wp.Value() == nil {
+                       n++
+               }
+       }
+       const want = 3 * N / 4
+       if n < want {
+               t.Fatalf("not enough weak pointers are nil: expected at least %v, got %v", want, n)
+       }
+}