Fix callbacks that cause goroutine stack to grow

CHANGELOG.md

@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.5.6] - 2026-07-05
+
+### Fixed
+- **Critical: callback stack-move corruption** — when a C callback re-enters Go and grows the goroutine stack (`copystack`), `syscallArgs` on the goroutine stack would move but `syscallN` on g0 still held the old address, writing return values to freed memory. Fix: `syscallArgs` is now heap-allocated via `sync.Pool`, immune to `copystack`. Reverts the unsafe `//go:noescape` optimization from v0.5.4. Discovered and fixed by [@tie](https://github.com/tie) with `TestCallbackGrowStack` reproducer. ([PR #59](https://github.com/go-webgpu/goffi/pull/59))
+- **sret cleanup** — simplified >16B struct return path in `call_unix.go`, removed redundant `sretBuf` variable
+- **Added `TestStructReturn24B`** — end-to-end test for >16B struct return via sret hidden pointer
+
 ## [0.5.5] - 2026-06-15
 
 ### Fixed

README.md

@@ -174,11 +174,11 @@ variadic arg type slices.
 
 | Benchmark | Time | Allocations |
 |-----------|------|-------------|
-| Empty function (`getpid`) | 88 ns | 0 allocs (Unix) / 2 allocs (Windows) |
-| Integer argument (`abs`) | 114 ns | 0 allocs (Unix) / 3 allocs (Windows) |
-| String processing (`strlen`) | 98 ns | 0 allocs (Unix) / 3 allocs (Windows) |
+| Empty function (`getpid`) | 88 ns | 0 allocs (steady state) |
+| Integer argument (`abs`) | 114 ns | 0 allocs (steady state) |
+| String processing (`strlen`) | 98 ns | 0 allocs (steady state) |
 
-Since v0.5.4, `//go:noescape` on `runtime_cgocall` keeps `syscallArgs` on the goroutine stack — true zero-allocation FFI on Unix platforms. Windows uses `syscall.SyscallN` (runtime-managed).
+`syscallArgs` is heap-allocated via `sync.Pool` for callback safety (goroutine stack may move during C→Go callbacks). Pool reuse gives 0 allocs/op in steady state.
 
 At 60 FPS with ~50 FFI calls per frame, overhead is **5 µs per frame** — 0.03% of the 16.6 ms budget. Unmeasurable in profiling.
 

docs/ARCHITECTURE.md

@@ -73,7 +73,7 @@ ffi.PrepareCallInterface(cif, types.DefaultCall,
 2. Marks goroutine as "in syscall" — allows GC to proceed
 3. Calls our assembly wrapper
 
-Since v0.5.4, the `runtime_cgocall` linkname declaration has `//go:noescape`, keeping `syscallArgs` on the goroutine stack (zero heap allocations). All ABI-boundary structs use `structs.HostLayout` (Go 1.23+) to guarantee C-compatible memory layout.
+Since v0.5.6, `syscallArgs` is heap-allocated via `sync.Pool` — goroutine stacks may move during C→Go callbacks (`copystack`), and assembly on g0 holds the args pointer across the call. All ABI-boundary structs use `structs.HostLayout` (Go 1.23+) to guarantee C-compatible memory layout.
 4. Restores Go stack on return
 
 We access it via `//go:linkname`:

docs/PERFORMANCE.md

@@ -37,7 +37,7 @@
 - **Minimum FFI overhead**: ~88 ns (empty function)
 - **Typical overhead**: ~100-115 ns (with arguments)
 - **Overhead ratio**: ~400-500x vs direct Go call
-- **Allocations**: 2-3 per call (runtime.cgocall internals). Since v0.5.4, `//go:noescape` on `runtime_cgocall` keeps `syscallArgs` on the goroutine stack, eliminating the primary per-call heap allocation on Unix platforms.
+- **Allocations**: 0 in steady state. `syscallArgs` is heap-allocated via `sync.Pool` for callback safety (goroutine stack may move during C→Go callbacks). Pool reuse eliminates per-call allocations after warmup.
 
 ### 2. One-Time Costs
 

ffi/callback_test.go

@@ -7,6 +7,8 @@ import (
 	"sync"
 	"testing"
 	"unsafe"
+
+	"github.com/go-webgpu/goffi/types"
 )
 
 const callbackFloatRegCount = 8
@@ -734,3 +736,64 @@ func BenchmarkCallbackFloat(b *testing.B) {
 		callbackWrap(args)
 	}
 }
+
+// growStack makes the calling goroutine's stack grow, and so move (copystack), by
+// recursing with frames big enough to matter. It returns 0. The //go:noinline
+// keeps every frame real so the recursion actually uses up stack.
+//
+//go:noinline
+func growStack(n int) int {
+	if n == 0 {
+		return 0
+	}
+	var pad [4096]byte
+	pad[0] = byte(n)
+	pad[len(pad)-1] = byte(n)
+	return int(pad[0]) - int(pad[len(pad)-1]) + growStack(n-1)
+}
+
+// TestCallbackGrowStack guards against a callback from C into Go losing its
+// argument or return value when the goroutine stack moves during the call.
+//
+// CallNFloat builds the argument and return block, and syscallN (on g0) writes
+// the C return value back into it after the call. If that block sits on the
+// goroutine stack and the callback grows the stack, the stack moves and
+// syscallN's write lands at the old, pre-move address, so the return value is
+// lost.
+//
+// The test makes this deterministic instead of relying on process-global warmup
+// state. It runs on a fresh goroutine with a small starting stack and forces a
+// large stack growth from inside the callback, right while syscallN is holding
+// the pre-move address. growStack adds 0, so passing arg=42 must yield 42 back.
+func TestCallbackGrowStack(t *testing.T) {
+	u := types.UInt64TypeDescriptor
+	var cif types.CallInterface
+	if err := PrepareCallInterface(&cif, types.DefaultCall, u, []*types.TypeDescriptor{u}); err != nil {
+		t.Fatal(err)
+	}
+	cb := NewCallback(func(arg uintptr) uintptr {
+		// About 2 MiB of stack growth, enough to force a copystack mid-call.
+		// growStack returns 0.
+		return arg + uintptr(growStack(512))
+	})
+
+	done := make(chan uintptr, 1)
+	go func() {
+		arg, ret := uintptr(42), uintptr(0)
+		// cb is the trampoline's code address as a uintptr. Reinterpret its bits as
+		// an unsafe.Pointer rather than converting directly: a direct unsafe.Pointer(cb)
+		// trips vet's unsafeptr guard against conversions that hide a heap address from
+		// the GC, and the trampoline is code that is never GC-managed and never moved.
+		fn := *(*unsafe.Pointer)(unsafe.Pointer(&cb))
+		if err := CallFunction(&cif, fn, unsafe.Pointer(&ret), []unsafe.Pointer{unsafe.Pointer(&arg)}); err != nil {
+			t.Error(err)
+			done <- ^uintptr(0)
+			return
+		}
+		done <- ret
+	}()
+
+	if got := <-done; got != 42 {
+		t.Fatalf("callback returned %d, want 42 (argument or return value lost when the stack moved during the call)", got)
+	}
+}

ffi/ffi.go

@@ -248,6 +248,7 @@ func PrepareVariadicCallInterface(
 //   - All argument pointers must remain valid during the call
 //   - Return value buffer must be large enough for the result type
 //   - Use runtime.KeepAlive() if needed to prevent premature GC of arguments
+//   - Use runtime.Pinner to pin pointers under a moving GC
 func CallFunctionContext(
 	ctx context.Context,
 	cif *types.CallInterface,

ffi/struct_e2e_test.go

@@ -531,3 +531,47 @@ func TestCallbackStructArgWithScalar(t *testing.T) {
 		t.Errorf("expected %#v %d, received %#v %d", expected, extra, receivedArg1, receivedArg2)
 	}
 }
+
+// TestStructReturn24B exercises the sret path for a struct larger than 16 bytes.
+// The caller provides a buffer in rvalue, goffi points the hidden return pointer
+// (RDI on amd64, X8 on arm64) at it, and the callee writes the struct directly
+// into that buffer. This covers the >16B struct return ABI, which the other
+// struct-return tests (all <= 16 bytes) do not reach.
+func TestStructReturn24B(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Windows struct returns use a separate ABI path (call_windows.go), not covered here")
+	}
+	requireStructLib(t)
+
+	sym, err := GetSymbol(structTestLib, "return_struct_24")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tripleI64 := &types.TypeDescriptor{
+		Kind:      types.StructType,
+		Size:      24,
+		Alignment: 8,
+		Members: []*types.TypeDescriptor{
+			types.SInt64TypeDescriptor,
+			types.SInt64TypeDescriptor,
+			types.SInt64TypeDescriptor,
+		},
+	}
+
+	var cif types.CallInterface
+	if err := PrepareCallInterface(&cif, types.DefaultCall, tripleI64, nil); err != nil {
+		t.Fatal(err)
+	}
+
+	type TripleI64 struct{ A, B, C int64 }
+	var result TripleI64
+	if err := CallFunction(&cif, sym, unsafe.Pointer(&result), nil); err != nil {
+		t.Fatal(err)
+	}
+
+	want := TripleI64{11, 22, 33}
+	if result != want {
+		t.Fatalf("return_struct_24() = %+v, want %+v", result, want)
+	}
+}

ffi/testdata/structtest.c

@@ -86,6 +86,15 @@ struct return_pair_i64 return_struct_2ints(int64_t a, int64_t b) {
     return s;
 }
 
+// 24-byte struct (> 16 bytes) returned by value through the sret ABI: the caller
+// passes a hidden destination pointer (RDI on SysV AMD64, X8 on AAPCS64) and the
+// callee writes the struct into it. The Go test calls this with a real rvalue
+// buffer and checks the returned fields. triple_i64 is defined above.
+struct triple_i64 return_struct_24(void) {
+    struct triple_i64 s = {.a = 11, .b = 22, .c = 33};
+    return s;
+}
+
 // Variadic: sum N int64_t values.
 // Prototype: int64_t sum_variadic(int64_t count, ...)
 // nfixedargs = 1 (only 'count' is fixed).

internal/arch/amd64/call_unix.go

@@ -73,14 +73,9 @@ func (i *Implementation) Execute(
 	// Detect sret: struct > 16 bytes requires hidden first argument in RDI.
 	// The caller's rvalue buffer is passed as the first integer argument and
 	// callee writes the return value directly into it.
-	sretBuf := unsafe.Pointer(nil)
-	if cif.ReturnType.Kind == types.StructType && cif.ReturnType.Size > 16 {
-		if rvalue != nil {
-			sretBuf = rvalue
-		} else {
-			sretBuf = unsafe.Pointer(&[128]byte{})
-		}
-		addInt(uintptr(sretBuf))
+	sret := cif.ReturnType.Kind == types.StructType && cif.ReturnType.Size > 16
+	if sret {
+		addInt(uintptr(rvalue))
 	}
 
 	// Map arguments to registers or stack
@@ -222,10 +217,10 @@ func (i *Implementation) Execute(
 	ret, r2, fret, fret2 := gosyscall.CallNFloat(uintptr(fn), gpr, sse, stackArgs, numStack)
 
 	runtime.KeepAlive(avalue)
-	runtime.KeepAlive(sretBuf)
+	runtime.KeepAlive(rvalue)
 
 	// If sret, the callee wrote directly into rvalue — no further copy needed.
-	if sretBuf != nil {
+	if sret {
 		return nil
 	}
 

internal/arch/arm64/call_arm64.go

@@ -197,7 +197,7 @@ func (i *Implementation) Execute(
 
 	// Determine if we need to pass X8 for large struct return (sret)
 	var r8 uintptr
-	if cif.Flags&types.ReturnViaPointer != 0 && rvalue != nil {
+	if cif.Flags&types.ReturnViaPointer != 0 {
 		// For sret, pass rvalue pointer in X8 - callee writes directly to it
 		r8 = uintptr(rvalue)
 	}

internal/syscall/syscall_arm64.go

@@ -6,13 +6,18 @@ package syscall
 
 import (
 	"structs"
+	"sync"
 	"unsafe"
 )
 
 //go:linkname runtime_cgocall runtime.cgocall
-//go:noescape
 func runtime_cgocall(fn uintptr, arg unsafe.Pointer) int32
 
+// syscallArgsPool recycles the per-call argument/return block. The block must
+// live in non-moving memory (see callNFloat): pooling keeps it heap-resident and
+// reused, so the move-safe path costs no per-call allocation in steady state.
+var syscallArgsPool = sync.Pool{New: func() any { return new(syscallArgs) }}
+
 // syscallArgs matches the layout expected by syscallN assembly.
 // AAPCS64 uses X0-X7 (8 GPRs) and D0-D7 (8 FPRs) for arguments.
 // Args 9+ (integer) or FP overflow are placed on the stack.
@@ -63,7 +68,18 @@ func CallNFloat(fn uintptr, gpr [8]uintptr, fpr [8]uint64, stackArgs [7]uintptr,
 }
 
 func callNFloat(fn uintptr, gpr [8]uintptr, fpr [8]uint64, stackArgs [7]uintptr, numStack int, r8 uintptr) (r1 uintptr, r2 uintptr, fret [4]uint64) {
-	args := syscallArgs{
+	// args holds both the call arguments and the C function's return values.
+	// syscallN runs on g0 and writes the return values back into args after the
+	// call returns. That call can run a callback that re-enters Go and grows this
+	// goroutine's stack, which moves it. If args lived on the stack it would move
+	// too, syscallN's write would land at the old address, and the first such call
+	// would lose its return value. So args has to live in non-moving memory: we
+	// take it from the heap-backed pool. The heap does not move on today's Go
+	// (non-compacting GC, confirmed through Go 1.27). If a compacting GC ever
+	// lands, add runtime.Pinner here.
+	args := syscallArgsPool.Get().(*syscallArgs)
+	defer syscallArgsPool.Put(args)
+	*args = syscallArgs{
 		fn: fn,
 		a1: gpr[0], a2: gpr[1], a3: gpr[2], a4: gpr[3],
 		a5: gpr[4], a6: gpr[5], a7: gpr[6], a8: gpr[7],
@@ -87,7 +103,9 @@ func callNFloat(fn uintptr, gpr [8]uintptr, fpr [8]uint64, stackArgs [7]uintptr,
 		r8: r8, // X8 for large struct returns
 	}
 	_ = numStack // informational; assembly always pushes all 7 stack slots
-	runtime_cgocall(syscallNABI0, unsafe.Pointer(&args))
+
+	runtime_cgocall(syscallNABI0, unsafe.Pointer(args))
+
 	r1 = args.r1
 	r2 = args.r2
 	fret[0] = uint64(args.fr1)

internal/syscall/syscall_unix_amd64.go

@@ -6,13 +6,18 @@ package syscall
 
 import (
 	"structs"
+	"sync"
 	"unsafe"
 )
 
 //go:linkname runtime_cgocall runtime.cgocall
-//go:noescape
 func runtime_cgocall(fn uintptr, arg unsafe.Pointer) int32
 
+// syscallArgsPool recycles the per-call argument/return block. The block must
+// live in non-moving memory (see CallNFloat): pooling keeps it heap-resident and
+// reused, so the move-safe path costs no per-call allocation in steady state.
+var syscallArgsPool = sync.Pool{New: func() any { return new(syscallArgs) }}
+
 // syscallArgs matches the layout expected by syscallN assembly.
 // Supports up to 15 total arguments (6 GP registers + 9 stack slots),
 // matching purego's syscall15Args layout.
@@ -54,7 +59,18 @@ var syscallNABI0 uintptr
 //   - f1: XMM0 float return value (bit pattern)
 //   - f2: XMM1 second float return value — for {SSE, SSE} 9-16B struct returns (e.g. NSPoint)
 func CallNFloat(fn uintptr, gpr [6]uintptr, sse [8]float64, stackArgs [9]uintptr, numStack int) (r1 uintptr, r2 uintptr, f1 float64, f2 float64) {
-	args := syscallArgs{
+	// args holds both the call arguments and the C function's return values.
+	// syscallN runs on g0 and writes the return values back into args after the
+	// call returns. That call can run a callback that re-enters Go and grows this
+	// goroutine's stack, which moves it. If args lived on the stack it would move
+	// too, syscallN's write would land at the old address, and the first such call
+	// would lose its return value. So args has to live in non-moving memory: we
+	// take it from the heap-backed pool. The heap does not move on today's Go
+	// (non-compacting GC, confirmed through Go 1.27). If a compacting GC ever
+	// lands, add runtime.Pinner here.
+	args := syscallArgsPool.Get().(*syscallArgs)
+	defer syscallArgsPool.Put(args)
+	*args = syscallArgs{
 		fn: fn,
 		a1: gpr[0], a2: gpr[1], a3: gpr[2],
 		a4: gpr[3], a5: gpr[4], a6: gpr[5],
@@ -79,7 +95,9 @@ func CallNFloat(fn uintptr, gpr [6]uintptr, sse [8]float64, stackArgs [9]uintptr
 		f8: *(*uintptr)(unsafe.Pointer(&sse[7])),
 	}
 	_ = numStack // numStack is informational; assembly always pushes all 9 slots
-	runtime_cgocall(syscallNABI0, unsafe.Pointer(&args))
+
+	runtime_cgocall(syscallNABI0, unsafe.Pointer(args))
+
 	r1 = args.r1
 	r2 = args.r2
 	f1 = *(*float64)(unsafe.Pointer(&args.f1))