bun:ffi: stop allocating TinyCC JIT memory from the CRT heap on Windows - #32013
bun:ffi: stop allocating TinyCC JIT memory from the CRT heap on Windows#32013robobun wants to merge 3 commits into
Conversation
|
Updated 8:05 AM PT - Jul 7th, 2026
❌ @Jarred-Sumner, your commit 9a17f14 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32013That installs a local version of the PR into your bun-32013 --bun |
|
Status: diff is ready; no CI run has shown a failure related to it. Build 69805 (sha 9a17f14, Jarred's rebase onto current main + comment trim), final: 285 jobs passed, 1 failed. Binary size delta is +0.0 KB on every target vs main canary #69776, matching the fact that the only code change is inside The single failed job is one darwin-14-aarch64 test shard with two flaky tests:
Both fail identically on other unrelated PR branches today (builds 69752, 69781, 69787, 69797), and neither has any connection to Windows-only TinyCC JIT memory allocation, which is dead code on darwin. Run history: 61473 (f81b81e) red only on the then repo-wide bunx.test.ts breakage, since skipped on main by #32042; 61646 (main-merge) red only on expired build agents; 61698 (retrigger) green except one bootstrap-dead agent + 2 expired darwin shards; 69805 as above. Needs a maintainer to merge over the darwin flakes or retry that shard (the Buildkite token here is read-only). |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughWindows runtime execution in TinyCC now uses platform-specific memory management ( ChangesWindows Runtime Fixes for TinyCC
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Additional field evidence: Sentry group BUN-3K30 (https://bun-p9.sentry.io/issues/?query=issue%3ABUN-3K30) is the same crash seen from bun 1.4.x. Sentry split it into a separate group because the 1.4.x Rust-port frames ( I re-audited |
Backport upstream tinycc mob 6728a64f1b (win32: use VirtualAlloc for run memory) and 601a088214 (win32: improve tccrun protection diagnostics) as a vendor patch. The pinned tinycc commit mallocs JIT run memory from the CRT heap, makes pages inside that heap block executable with VirtualProtect, and registers SEH unwind tables pointing into it with RtlAddFunctionTable, ignoring the return value. Windows x64 processes using bun:ffi crash with an access violation inside ntdll under RtlAddFunctionTable during a later FFI compile. With the patch, run memory comes from a dedicated VirtualAlloc region, is released with VirtualFree after RtlDeleteFunctionTable, and RtlAddFunctionTable failures surface as compile errors. Non-Windows paths are unchanged (new branches are ifdef _WIN32).
f975687 to
9a17f14
Compare
|
Heads-up: #29476 bumps |
|
Verified: The only So whichever PR lands second, Agreed the cleanest end-state is landing both in oven-sh/tinycc and bumping past it, at which point the |
There was a problem hiding this comment.
No issues found, but this changes JIT executable-memory allocation and SEH unwind-table handling in vendored C — worth a human confirming the backport matches upstream 6728a64f1b/601a088214 against the pinned fork before merge.
Extended reasoning...
Overview
This PR adds patches/tinycc/tccrun.c.patch and registers it in scripts/build/deps/tinycc.ts. The patch backports two upstream TinyCC commits so that on Windows, JIT run memory is allocated via VirtualAlloc/VirtualFree instead of the CRT heap, and RtlAddFunctionTable failures are surfaced as errors. All new code is behind #ifdef _WIN32; non-Windows paths are byte-identical.
Security risks
The change governs allocation and page-protection of executable JIT memory and registration of SEH unwind tables — memory-safety-critical territory. The direction of the change is strictly safer (dedicated VirtualAlloc region instead of page-protecting CRT heap blocks, error-checking a previously unchecked RtlAddFunctionTable), and it is a verbatim backport of upstream fixes. I don't see new attack surface, but the domain itself is sensitive enough that a human should sign off.
Level of scrutiny
Medium-high. It's a small, focused patch to a vendored dependency using the established patches/ mechanism, and Windows CI exercises the new path via the existing bun:ffi suite. However, it modifies executable-memory lifecycle in C, has no deterministic regression test (justified: heap-state-dependent crash with no reliable repro), and the correctness hinges on the patch context matching the pinned oven-sh/tinycc fork commit exactly.
Other factors
The bug-hunting system found no issues. CI is green on Windows including the FFI suite. The PR description is unusually thorough (root-cause analysis, upstream commit references, alternative-explanation audit). A maintainer has already engaged with the branch. Given all that, this is close to approvable — I'm deferring only because vendored-C changes to JIT memory allocation fall outside what I'd auto-approve without a human glance at the upstream diff.
|
Re the bot's verification request: applying the two upstream commits' curl -sL https://raw.githubusercontent.com/oven-sh/tinycc/12882eee/tccrun.c | tee a.c > tccrun.c \
&& for sha in 6728a64f1b 601a088214; do gh api repos/TinyCC/tinycc/commits/$sha \
--jq '.files[]|select(.filename=="tccrun.c")|.patch' | patch -s a.c; done \
&& git apply --no-index patches/tinycc/tccrun.c.patch \
&& diff a.c tccrun.c && echo IDENTICALOutput here: |
|
Per-issue verification of the find-issues bot's list (1 candidate)
Fixes block: empty. No issue in the bot's list has an empirically proven FIXES verdict, so nothing should be added on that basis. Note on the existing Not repeated here: robobun's earlier comments on this PR verify patch mechanics (clean apply at both |
|
Agreed, and taken the conservative option: the PR body now says "Should address #31941" with an explicit note that the link rests on causal reasoning rather than an observed before/after, so merging will not auto-close it. The two Sentry groups (BUN-2V2K, BUN-3K30) are where the field signal will show up; if #31941's reporter confirms on a release carrying this patch, it can be closed then. |
|
Closing in favor of #33653, which bumps Verified
The crash analysis and Sentry group references (BUN-2V2K, BUN-3K30, #31941) have been carried over into #33653's description. |
Crash
Windows x64 processes using
bun:ffisegfault inside ntdll while compiling symbols, always with this stack (symbolicated from bun 1.3.14):Fault addresses vary per event and look heap-like (0x1CB99EF6BB8, 0x189937629B8, 0x2C426471E98, ...). Crash reporting has 12,000+ events across bun 1.3.8 through 1.3.14 (report ID BUN-2V2K), 100% Windows, spread over many distinct machines and apps. There is no known deterministic reproduction; the crash depends on accumulated heap state.
Cause
The pinned oven-sh/tinycc commit allocates JIT "run memory" with
tcc_malloc, i.e. from the CRT heap, which on Windows is the NT process heap. It then:VirtualProtects the code pages toPAGE_EXECUTE_READWRITE, andRtlAddFunctionTable, ignoring the return value.So executable JIT pages and registered unwind data live inside pages owned by the NT process heap. In bun that heap is used almost exclusively by TinyCC and by ntdll itself (bun's own allocations go through mimalloc, JSC's through bmalloc), which is why corrupted heap or unwind-table state surfaces precisely at the next
RtlAddFunctionTablecall: it allocates its dynamic-table node from the process heap and updates the process-wide table list.Upstream TinyCC identified and fixed exactly this:
RtlAddFunctionTablefailure as an error instead of silently keeping a never-registered table pointer (previously that pointer was later handed toRtlDeleteFunctionTable).The pinned fork commit predates both.
Fix
Backport the two upstream commits verbatim as
patches/tinycc/tccrun.c.patch, registered inscripts/build/deps/tinycc.ts(the existing vendor-patch mechanism) with a note to drop the patch onceTINYCC_COMMITadvances past an upstream merge containing 601a088214:VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE)region, released withVirtualFree(MEM_RELEASE)afterRtlDeleteFunctionTable(same unregister-before-free order as before).RtlAddFunctionTablefailure now fails the FFI compile with "RtlAddFunctionTable failed".VirtualProtectfailure gets a Windows-specific diagnostic.Non-Windows behavior is byte-identical: every new branch is
#ifdef _WIN32.Other explanations audited and ruled out on current code: registration/unregistration are strictly paired (
run_function_tableis set once intcc_relocate_exand deleted only intcc_run_freebefore the memory is released, and bun frees TCC states only throughtcc_delete), so there is no code path that frees a registered table's memory without unregistering it. Concurrent Worker compiles hitting TinyCC globals remain theoretically possible but cannot account for the event volume.Verification
bun bdre-fetches the pinned tinycc commit, applies the patch cleanly (the patch-content hash invalidates the cached source), and builds.bun bd test test/js/bun/ffi/matches main (the twoffi.test.jserrors about/tmp/bun-ffi-test.dylibare pre-existing; they reproduce with the released bun).JSCallback+CFunctioncompile/call/close loop runs clean under the ASAN debug build.#ifdef _WIN32branches and run the existingbun:ffisuite through the new allocation path.Why there is no new test
The changed code is Windows-only vendored C, dead on Linux, and the crash has no deterministic repro (heap-state dependent). No test can fail before and pass after this change on Linux, and a Windows stress test would not reliably fail on the unfixed build either. The existing Windows FFI tests exercise the new path on every CI run.
Related issue
Should address #31941 (not auto-closing: the link is causal reasoning, not an observed before/after; please reopen if it recurs on a release containing this patch).
That report is the same memory class seen from the other side: a Windows x64 standalone executable doing sustained FFI polling crashes after ~2 hours while executing the compiled trampoline (
JSFFIFunction::trampolinecalling into an anonymous executable region), with the same heap-range fault addresses (0x1A5F8AF0108, 0x210D7F40108) as the compile-time crashes above. The workload churns the CRT heap heavily around the live JIT block (libuv allocates from it; the session had 1,484 spawns and 12k fetches). With this patch the JIT code no longer lives in heap pages at all. The issue's 2-hour repro has not been run against the patched build; if the crash recurs on a release containing this fix, the issue should be reopened.