fix(ffi): externalize typed array backing stores to prevent GC heap corruption

[robobun]

Summary

• Fixes a crash where passing a small Buffer to an FFI function and overflowing it corrupts JSC's GC heap, causing segfaults in unrelated operations
• Externalizes typed array backing stores before extracting pointers for FFI by calling JSC's possiblySharedBuffer() / slowDownAndWasteMemory()
• Adds regression test for FFI small buffer externalization

Problem

When Buffer.alloc(N) creates a small buffer, JSC uses "FastTypedArray" mode where data is stored inline in the GC heap cell. If an FFI function writes past the buffer's end (e.g., writing a 192-byte XEvent into a 96-byte buffer), it corrupts JSC's GC metadata, causing crashes like:

panic(main thread): Segmentation fault at address 0x1F4
JSC::LocalAllocator::tryAllocateIn (LocalAllocator.cpp:229)
JSC::MarkedSpace::beginMarking (MarkedSpace.cpp:443)

Fix

Before extracting a typed array's pointer for FFI, call possiblySharedBuffer() which internally calls slowDownAndWasteMemory() for FastTypedArray mode. This moves data from the GC heap to separately-allocated external memory. For already-external arrays, it's a no-op.

Applied in all FFI pointer extraction paths:

• JSVALUE_TO_TYPED_ARRAY_VECTOR in FFI.h (TCC-compiled wrappers via dlopen)
• ptrWithoutTypeChecks in FFIObject.zig (fast path for ptr())
• ptr_ in FFIObject.zig (slow path)

Test plan

• New regression test: test/regression/issue/23446.test.ts (5 tests, 10038 assertions)
• Existing FFI tests pass: cc.test.ts, ffi-error-messages.test.ts, addr32.test.ts
• Verified original reproduction no longer corrupts GC heap (crash moves from GC allocator to Gigacage allocator)

Closes #23446

🤖 Generated with Claude Code

[robobun]

^{Updated 2:24 PM PT - Mar 18th, 2026}

❌ Your commit d406d5ab has 2 failures in Build #40043 (All Failures):

• test/integration/next-pages/test/dev-server.test.ts - code 1 on 🍎 14 aarch64
• vendor/elysia/test/response/stream.test.ts - code 1 on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 28259

That installs a local version of the PR into your bun-28259 executable, so you can run:

bun-28259 --bun

[coderabbitai]

Walkthrough

Added a new FFI entrypoint, Bun__FFI__ensureExternalBackingStore, and wired it through headers, Zig bindings, and C++ implementation to ensure typed-array backing stores are externalized before exposing pointers to FFI; added a regression test exercising FFI small-buffer externalization and GC resilience for issue #23446.

Changes

Cohort / File(s)	Summary
FFI Header & Macros src/bun.js/api/FFI.h	Declare new imported symbol Bun__FFI__ensureExternalBackingStore(EncodedJSValue) and update JSVALUE_TO_TYPED_ARRAY_VECTOR to delegate to it instead of using fixed-offset pointer arithmetic.
Zig bindings / API integration src/bun.js/api/FFIObject.zig, src/bun.js/api/ffi.zig	Add extern declaration for Bun__FFI__ensureExternalBackingStore in Zig, register the symbol in Offsets and CompilerRT.inject, and modify ptrWithoutTypeChecks / ptr_ flows to consult the new function and use an external pointer when provided.
C++ implementation src/bun.js/bindings/ffi.cpp	Add extern "C" void* Bun__FFI__ensureExternalBackingStore(JSC::EncodedJSValue) that decodes the JS value, checks/casts to JSArrayBufferView, invokes externalization via possiblySharedBuffer() and returns the backing pointer or nullptr.
Regression tests test/regression/issue/23446.test.ts	Add tests that build a small C library and exercise FFI reads/writes on small buffers, stress repeated allocations with forced GC, and verify data integrity after externalization; tests are skipped on unsupported environments.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: externalizing typed array backing stores to prevent GC heap corruption in FFI operations.
Description check	✅ Passed	The PR description is comprehensive, covering the problem, root cause analysis, the fix approach, implementation details, and test verification.
Linked Issues check	✅ Passed	The PR directly addresses issue #23446 by implementing the core objective: externalizing typed array backing stores before FFI pointer extraction to prevent GC heap corruption.
Out of Scope Changes check	✅ Passed	All changes are focused on the FFI externalization fix: implementation in FFI.h, FFIObject.zig, ffi.zig, ffi.cpp, plus a comprehensive regression test.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📝 Coding Plan

• Generate coding plan for human review comments

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

The approach looks correct — possiblySharedBuffer() is the standard JSC API for externalizing FastTypedArray inline storage, and it's already used elsewhere in the codebase. However, this touches FFI/GC interaction across C++, Zig, and TCC-compiled C, which warrants a human familiar with JSC internals confirming the fix.

Extended reasoning...

Overview

This PR fixes a GC heap corruption crash when small Buffer objects (stored inline as JSC FastTypedArrays) are passed to FFI functions. The fix calls possiblySharedBuffer() on typed array views before extracting their data pointer, which forces JSC to externalize inline storage. Changes span 5 files: a new C++ function in ffi.cpp, modifications to FFI.h (TCC-compiled C), FFIObject.zig (fast and slow pointer extraction paths), ffi.zig (TCC symbol injection), and a new regression test.

Security risks

This is a security improvement. The original code allowed FFI buffer overflows to corrupt JSC GC metadata (inline heap cells). After this fix, data is stored in external allocations, so overflows affect separately-allocated memory instead of GC internals. No new attack surface is introduced.

Level of scrutiny

Medium-high. While the fix uses well-established JSC APIs (possiblySharedBuffer() / vector()) already used in 5+ other places in the codebase, the change modifies a core assumption about how FFI extracts pointers from typed arrays. The interaction between TCC-compiled wrappers, Zig fast paths, and JSC GC internals is subtle enough that a human familiar with these systems should confirm the approach — particularly that externalizing at pointer-extraction time (rather than buffer creation) is the right layer.

Other factors

• The PR has a claude label indicating AI authorship with no human review yet
• The regression test is thorough (5 tests, GC stress testing with 5000 iterations)
• No CODEOWNERS cover these paths
• possiblySharedBuffer() for already-external arrays is documented as a no-op, so performance impact should be minimal for the common case

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/regression/issue/23446.test.ts`:
- Around line 7-10: The skip predicate passed to describe.skipIf is redundant
because isFFIUnavailable is defined as isWindows && isArm64 and therefore
already covered by checking isWindows; update the call to describe.skipIf to
remove the redundant "|| isFFIUnavailable" term so the predicate becomes
describe.skipIf(isASAN || isWindows)("FFI small buffer externalization", ...) —
adjust the predicate expression where describe.skipIf is used and keep the
isFFIUnavailable const only if still needed elsewhere (or remove its declaration
if unused).
- Around line 28-53: The test uses an order-dependent fixture by initializing
lib inside it("setup") (using cc and tempDirWithFiles) and then relying on that
shared lib in later tests; make setup deterministic by moving
initialization/cleanup into beforeAll/afterAll (create dir and assign lib via cc
with the same symbols), or convert each dependent test to be self-contained by
calling tempDirWithFiles and cc inside each test before assertions; ensure any
teardown mirrors creation so lib is never undefined when tests run individually.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b64ae8ea-9ca6-4a25-ba2f-8bdb133d1dfb

📥 Commits

Reviewing files that changed from the base of the PR and between 8aeb2c2 and 874870c.

📒 Files selected for processing (1)

• test/regression/issue/23446.test.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/regression/issue/23446.test.ts`:
- Around line 52-54: The test's afterAll handler calls lib.close() without
guarding against lib being undefined if beforeAll failed; update the afterAll
block to check that lib is defined (e.g., if (lib) or using optional chaining
like lib?.close()) before calling close so it won't throw when lib was never
initialized; reference the afterAll callback and the lib variable when making
the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 434696c0-79ac-467f-b99f-682429a6eb59

📥 Commits

Reviewing files that changed from the base of the PR and between 874870c and 5c7151f.

📒 Files selected for processing (1)

• test/regression/issue/23446.test.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Add defensive null check in afterAll

If beforeAll fails (e.g., cc() throws during compilation), lib remains undefined and lib.close() will throw, potentially masking the original failure.

🛡️ Suggested fix

  afterAll(() => {
-   lib.close();
+   lib?.close();
  });

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	afterAll(() => {
	lib.close();
	});
	afterAll(() => {
	lib?.close();
	});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/regression/issue/23446.test.ts` around lines 52 - 54, The test's
afterAll handler calls lib.close() without guarding against lib being undefined
if beforeAll failed; update the afterAll block to check that lib is defined
(e.g., if (lib) or using optional chaining like lib?.close()) before calling
close so it won't throw when lib was never initialized; reference the afterAll
callback and the lib variable when making the change.

[claude]

🟡 Nit: JSArrayBufferView__offsetOfVector is now dead code. The #define is still injected into TCC-compiled code via CompilerRT.define() (line 2401), but after this PR changed JSVALUE_TO_TYPED_ARRAY_VECTOR to call Bun__FFI__ensureExternalBackingStore, no code in FFI.h references it. Consider removing the define injection and the corresponding Offsets struct field as cleanup.

Extended reasoning...

What the bug is

The CompilerRT.define() function in ffi.zig (line 2401) still injects a #define JSArrayBufferView__offsetOfVector into TCC-compiled code. However, the only consumer of this define was the old JSVALUE_TO_TYPED_ARRAY_VECTOR implementation which directly read the vector pointer at this offset:

// OLD:
return *(void**)((char*)val.asPtr + JSArrayBufferView__offsetOfVector);
// NEW:
return Bun__FFI__ensureExternalBackingStore(val);

Grepping FFI.h for JSArrayBufferView__offsetOfVector returns zero matches, confirming it is unused.

Why this happens

This PR correctly changed all typed array pointer extraction paths to go through Bun__FFI__ensureExternalBackingStore to prevent GC heap corruption. The new function is properly injected via CompilerRT.inject(). However, the old JSArrayBufferView__offsetOfVector define was not removed from CompilerRT.define(), and the corresponding field in the Offsets struct and its initialization in ffi.cpp remain.

Impact assessment

This is harmless dead code:

• An unused #define has zero runtime cost (it is a preprocessor symbol that never gets expanded)
• The Offsets struct field initialization is a single u32 read alongside other still-needed fields, so the overhead is negligible
• It does not cause incorrect behavior, crashes, or any observable issue

One consideration: these defines are technically in scope for user C code compiled via cc(), so a user could theoretically reference JSArrayBufferView__offsetOfVector directly. However, this would be relying on an undocumented internal detail, and the offset alone is no longer sufficient to safely extract a typed array vector (which is precisely what this PR fixes).

Concrete walkthrough

1. User calls cc({ source: "test.c", symbols: {...} })
2. CompilerRT.define(state) runs, injecting #define JSArrayBufferView__offsetOfVector <value> into the TCC compilation context
3. The user's code and FFI.h are compiled together
4. No code in FFI.h references JSArrayBufferView__offsetOfVector — JSVALUE_TO_TYPED_ARRAY_VECTOR now calls Bun__FFI__ensureExternalBackingStore instead
5. The define sits unused in the preprocessor symbol table

Suggested fix

Remove the JSArrayBufferView__offsetOfVector entry from the defineSymbolsComptime call in CompilerRT.define(). Optionally, also remove the JSArrayBufferView__offsetOfVector field from the Offsets struct and its initialization in ffi.cpp, though leaving it is equally harmless.