bun:ffi: copy the TinyCC diagnostic into the error cc() throws - #38062
bun:ffi: copy the TinyCC diagnostic into the error cc() throws#38062robobun wants to merge 5 commits into
Conversation
The Step::Failed message was wrapped into the Error instance through an untagged ZigString, which does not copy, and the buffer was freed together with the CompileC when cc() returned. Tag it UTF-8 so toErrorInstance copies it, and drop the leading-byte skipping in both TinyCC error callbacks, which was working around the symptom of that free.
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: fix and test pushed (head 8a306cf), waiting on CI. Reproduced on x64 before the change in two ways:
With the change, Review so far: the Mach-O underscore in the expected unresolved-reference name (e80b5c4) and the relative-path case pinning the callback change (8a306cf) came out of it; the overlap with #31451 and the sibling symbol-name sites left unchanged are described in the PR body. |
|
Updated 12:53 PM PT - Aug 13th, 2026
❌ @robobun, your commit 8a306cf has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38062That installs a local version of the PR into your bun-38062 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the init → init_utf8 switch makes toErrorInstance copy the transient Step::Failed buffer instead of wrapping it with createWithoutCopying, fixing the UAF.
What was reviewed:
- Verified in
helpers.hthatgetErrorInstance→toStringusesStringImpl::createWithoutCopyingfor untagged strings and copies for the UTF-8 tag;init_utf8sets that tag. - Confirmed line 1223 is the only site that reads
Step::Failed { msg }and hands it to JS —link_symbols/CFunctiongo throughcreate_jsc_ffi_functionand don't touch this path. - The removed leading-byte-skip loops were a workaround for the same symptom (mimalloc free-list pointer written into the freed block), applied at the wrong point in time; removing them is correct.
- Test spawns a fixture covering both callback paths (
CompileCunresolved ref,Functionwrapper clash) and asserts the exact TinyCC text; failure mode without the fix is documented for both release and ASan.
Extended reasoning...
Overview
Two-file change: src/runtime/ffi/ffi_body.rs (three hunks) and test/js/bun/ffi/cc.test.ts (one new spawned-fixture test). The Rust change is a one-token fix — ZigString::init(msg) → ZigString::init_utf8(msg) at the site where cc() throws a per-symbol wrapper compile failure — plus removal of two identical "skip leading non-printable bytes" loops in the TinyCC error callbacks that were a misplaced workaround for this bug's symptom.
Correctness of the fix
I traced the C++ side: ZigString::to_error_instance → getErrorInstance → Zig::toString (helpers.h:76). For an untagged pointer it returns StringImpl::createWithoutCopying (line 116), which borrows the bytes; for a UTF-8-tagged pointer it goes through fromUTF8ReplacingInvalidSequences, which allocates a copy. init_utf8 sets bit 61 (mark_utf8), so the message is now copied before compile_c (which owns the Function and thus the Step::Failed Box<[u8]>) is dropped on return. The PR's mimalloc-free-list explanation of the 8 garbage bytes matches the observed corruption exactly.
I checked for sibling sites: function.compile() is called only from the cc() path (line 1211), and line 1223 is the only consumer of Step::Failed { msg }. link_symbols and create_cfunction use create_jsc_ffi_function instead and never reach this. The removed byte-skipping loops in handle_compilation_error / handle_tcc_error ran before the buffer was freed, so they never stripped anything and would have eaten a leading non-ASCII path byte — deleting them is right.
Security risks
None. This is an error-path string-lifetime fix in the FFI cc() failure branch; no new inputs are trusted, no permissions or crypto touched. The change strictly improves memory safety (removes a heap UAF flagged by ASan).
Level of scrutiny
Low-to-medium. It's native code with a memory-safety implication, but the mechanism is fully understood, the fix is the tree's documented idiom for transient buffers (JSGlobalObject::create_error_instance uses the same tag for the same reason), and the diff is tiny and mechanical.
Other factors
The test is well-constructed per REVIEW.md: spawned fixture (crash-isolated), drains stdout/stderr/exited concurrently, asserts a combined object so a crashed child shows stderr, exercises both TinyCC callback owners, and the clash.c case (user defines JSFunctionCall) is a clever way to force the wrapper compile — the only Function-path failure reachable from JS. The PR description demonstrates fails-without / passes-with on both release and ASan builds. The test not being gated on isASAN is intentional and stated to pass; the setjmp/longjmp ASan conflict noted on other tests applies to in-process syntax-error paths, not these link/redefinition errors.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — the init → init_utf8 change correctly routes toString through fromUTF8ReplacingInvalidSequences (which copies) instead of createWithoutCopying, and the removed leading-byte loops were dead. A human look would still be worthwhile: the find-duplicate-prs bot flagged #31451 as fixing the same UAF at getErrorInstance for every caller, which is a layering call a maintainer should make, and comment-cop has an open flag on the 3-line comment at ffi_body.rs:1224.
What was reviewed:
- Verified in
helpers.hthat untaggedZigString→getErrorInstanceusescreateWithoutCopyingwhile the UTF-8 tag copies viafromUTF8ReplacingInvalidSequences. - Checked the deleted "garbage-skipping" loops ran before the free that produced the garbage, so they never did anything and would have eaten a leading non-ASCII byte.
- New test spawns a subprocess (so an ASan crash fails the test rather than the runner), covers both the
CompileCand wrapperFunctionpaths, and the non-ASCII case pins the UTF-8 decode.
Extended reasoning...
Overview
Two-file change: src/runtime/ffi/ffi_body.rs swaps ZigString::init(msg) for ZigString::init_utf8(msg) at the one site that throws a per-symbol wrapper compile diagnostic, and deletes two leading-byte-skipping loops from the TinyCC error callbacks. test/js/bun/ffi/cc.test.ts gains a spawned-fixture test covering both TinyCC failure paths (user-source link error and wrapper-compile clash) plus a non-ASCII diagnostic.
Security risks
None. This is an error-message lifetime fix on the cc() failure path; no new inputs are trusted and no security-gated code is touched.
Level of scrutiny
High — this is a use-after-free fix in native code. I traced the mechanism through src/jsc/bindings/helpers.h: getErrorInstance calls toString(ZigString), which for an untagged string returns WTF::StringImpl::createWithoutCopying over the borrowed bytes; the UTF-8 tag routes to WTF::String::fromUTF8ReplacingInvalidSequences, which allocates its own storage. msg is owned by the Function inside the local compile_c, which drops when bun_ffi_cc returns, so the untagged path is a genuine UAF and the tagged path is not. The removed loops in handle_tcc_error / handle_compilation_error ran during the callback, before the buffer they copied into is freed, so the "garbage" they were written to strip could never have been present at that point — dead code whose only effect was to eat a legitimate leading byte outside 0x21..0x7e.
Other factors
Two open automated comments push this to human review. First, find-duplicate-prs flagged #31451 as fixing the same class at Zig::getErrorInstance (copy untagged strings), which would subsume this call-site fix and also cover the AsyncModule.rs sibling the PR description explicitly leaves for later; per REVIEW.md's "fix bugs at the layer that owns the violated invariant" and "fix the whole class in the same PR", a maintainer should pick the layer. Second, comment-cop flagged the 3-line comment above the fix; I read it as explaining a non-obvious copy semantic rather than justifying a workaround, but it is an outstanding flag. The test itself follows harness conventions (tempDir, await using spawn, drains both pipes concurrently, asserts a combined object with exitCode last) and is not gated on isASAN — the author reports it passes under ASan, and the failure paths exercised go through the error callback rather than TinyCC's setjmp/longjmp. CI failures on the first commit (require-cache.test.ts, bake/deinitialization.test.ts) are unrelated to FFI.
|
On the #31451 overlap: that PR makes
The two changes touch different files and are independent, so either can land first. The |
There was a problem hiding this comment.
LGTM — the macOS underscore concern from my earlier review is addressed in e80b5c4 via symbolPrefix.
What was reviewed:
- Verified in
helpers.hthat untaggedZigString→toStringusesStringImpl::createWithoutCopyingwhile the UTF-8-tagged path copies viafromUTF8ReplacingInvalidSequences, soinit_utf8fixes both the UAF and the Latin-1 mojibake. - Confirmed the removed leading-byte-skip loops were dead: the "garbage" was mimalloc's free-list pointer written into the freed buffer after the callback had already copied it.
- Test follows harness conventions (spawned fixture,
tempDir, concurrent pipe drain, combined-object assertion) and now branches onisMacOSfor the Mach-O_prefix in both the asm label and theunresolvedexpectation.
Extended reasoning...
Overview
The PR changes one call site in src/runtime/ffi/ffi_body.rs from ZigString::init(msg) to ZigString::init_utf8(msg) when throwing a wrapper-compilation diagnostic from cc(), removes two dead leading-byte-skip loops in the TinyCC error callbacks, and adds a spawned-subprocess test to test/js/bun/ffi/cc.test.ts covering both diagnostic paths (CompileC and Function) plus a non-ASCII token.
Security risks
None. This is an error-message construction path; the only data flow is TinyCC diagnostic text → JS Error.message. The change moves from a borrowing string wrapper to a copying one, which strictly reduces the memory-safety surface (fixes a heap-use-after-free that ASan catches on the unfixed build).
Level of scrutiny
Medium. It's native memory-safety code, but the change is a one-token substitution to the tree's established idiom for transient-buffer error messages, verified against src/jsc/bindings/helpers.h:76-118: untagged strings hit StringImpl::createWithoutCopying (line 116), UTF-8-tagged strings hit fromUTF8ReplacingInvalidSequences (line 95, a copy). The dead-loop removal is well-justified — the garbage bytes are the allocator's free-list link written after the callback returns, so the loops could never have removed it, and would have incorrectly stripped a leading non-ASCII filename byte.
Other factors
My earlier inline finding (macOS leading_underscore making the exact-string unresolved assertion fail) was addressed in commit e80b5c4: the test now defines symbolPrefix = isMacOS ? "_" : "" and interpolates it into both the asm label and the expected unresolved reference to '...' string. The bug-hunting system found nothing on this revision. The test is hermetic (no network, subprocess-isolated, using tempDir), drains stdout/stderr/exited concurrently, and asserts a combined object so a crashed fixture surfaces stderr in the diff. The PR description demonstrates the test fails on the unfixed build in both release (garbage prefix + mojibake) and ASan (heap-use-after-free) modes.
There was a problem hiding this comment.
LGTM — the macOS underscore issue from my earlier pass is addressed in e80b5c4 via the shared symbolPrefix.
What was reviewed:
- Verified in
helpers.hthat untaggedZigString→toStringhitsStringImpl::createWithoutCopying(line 116) while UTF-8-tagged hitsfromUTF8ReplacingInvalidSequences(line 95), soinit_utf8both copies and decodes as claimed. - Checked that the removed leading-byte loops are pinned by the
" relative.c"case (leading space survives), and that both callbacks now store the bytes verbatim. - Confirmed the test covers both throw sites (
CompileC::deferred_errorsheader path andFunction::Step::Failedbare path), the<string>prefix the UAF overwrote, and UTF-8 round-trip.
Extended reasoning...
Overview
Two files: src/runtime/ffi/ffi_body.rs changes one token at the Step::Failed throw site (ZigString::init → init_utf8) and deletes two identical leading-byte-skipping loops from the TinyCC error callbacks; test/js/bun/ffi/cc.test.ts adds a spawned-fixture describe block covering four diagnostic shapes. Net Rust change is one substitution plus ~22 deleted lines.
Security risks
None. The change tightens a use-after-free on an error path (diagnostic message text) and fixes a Latin-1/UTF-8 decoding mismatch. No new input surface, no auth/crypto/permissions.
Level of scrutiny
Native memory safety in FFI is the most-blocked category, so I traced the mechanism rather than trusting the description: Zig::toString at helpers.h:115-116 wraps untagged bytes with createWithoutCopying, and Step::Failed { msg } is a Box<[u8]> owned by Function inside the compile_c local — dropped when bun_ffi_cc returns the exception. init_utf8 routes through helpers.h:95 (fromUTF8ReplacingInvalidSequences), which allocates a fresh WTFStringImpl. The removed loops were a symptom-level workaround that could never fire (mimalloc writes the free-list link after the callback has copied the bytes out) and would have corrupted diagnostics beginning with a space or non-ASCII path byte; the new relative.c case pins that removing them preserves the leading byte.
Other factors
My previous review found the macOS leading_underscore mismatch in the unresolved assertion; e80b5c4 addresses it by deriving both the expected symbol name and the asm label from one isMacOS ? "_" : "" prefix, keeping the assertion exact per platform rather than loosening to a regex. The comment-cop note was addressed in 72c1b86 (one-line comment). All inline threads are resolved. The test follows harness conventions (subprocess, tempDir, drains all pipes, asserts a combined object with stderr visible on failure) and does not gate on isASAN — the author reports it passes under the ASan debug build, which is the point (it is the ASan repro for the UAF). CI is the final arbiter on the Windows leading-space filename and macOS lanes, but nothing here warrants holding for a human.
1 participant
Problem
cc()compiles around a symbol fails in TinyCC, the Error thrown to JS has the first 8 bytes of its message replaced by garbage, e.g.error: @nÁÈ[or: unresolved reference to 'memmove'for TinyCC'stcc: error: unresolved reference to 'memmove'(seen on the Linux aarch64 lanes of bun:ffi: treat arguments not passed to a cc() symbol as undefined instead of reading past the call frame #38014; any wrapper diagnostic does it, on every platform).AddressSanitizer: heap-use-after-freereading the message (JSC::stringCopySameType<-JSON.stringify), freed inFFI::bun_ffi_ccat the end ofcc().src/runtime/ffi/ffi_body.rs:1235(before this change) threwZigString::init(msg).to_error_instance(...). For an untaggedZigString,toErrorInstancebuilds the message with the non-copyingZig::toString(src/jsc/bindings/helpers.h,getErrorInstance), so the Error's.messagepoints straight at theStep::Failedbuffer. That buffer is owned by theFunctioninside theCompileClocal, which is dropped whencc()returns the exception, before JS can read it. The 8 garbage bytes are the allocator's free-list link written at the start of the freed block; the rest of the message survives, which is why the text after the first 8 bytes was intact in every sample.initreads the bytes as Latin-1, but TinyCC's text is UTF-8 (it quotes source tokens), so a diagnostic mentioning a non-ASCII name comes out as mojibake ((got 'ñ')for(got 'ñ')).Function::handle_tcc_error,CompileC::handle_compilation_error) also skipped leading bytes outside0x21..0x7e. The loop dates from 5e270f9 (2022), where the callback had been storing TinyCC's own buffer (freed by TinyCC right after the callback) and the same commit fixed that by copying; the loop runs on the freshly delivered text, so it never had garbage to remove. Wrapper diagnostics always start with<string>:ortcc:, so there it does nothing; aCompileCdiagnostic starts with the source path exactly as the user passed it, so there it eats the first byte of a relative path that starts with a space or a non-ASCII character (ñsyntax.c:1: error: ...arrives assyntax.c:1: error: ...).Fix
ZigString::init_utf8(msg).to_error_instance(...). A UTF-8-taggedZigStringis decoded as UTF-8 and copied bytoErrorInstance, so the Error owns a correctly decoded message. This is the tree's idiom for a message in a transient buffer (JSGlobalObject::create_error_instance, "Ensure we clone it"). EveryStep::Failedmessage goes through this one site, so the staticfail()messages are covered too.getErrorInstancecopy for every caller; that removes the use after free for this site too, but not the Latin-1 decoding or the loops, and it has no test for this path. The two changes are independent (different files) and both stand with the other applied.ZigString::init(function_name...)sites that name the JS functions and keys in the success paths ofcc(),dlopen()andlinkSymbols()(ffi_body.rs~1234, ~1577, ~1663, plus theputcalls next to them). They share the Latin-1 half of the defect (a non-ASCII symbol name comes back asañadir) but not the lifetime half, and need their own tests across the three entry points; filed separately.test/js/bun/ffi/cc.test.ts, "TinyCC diagnostics thrown by cc()". One spawned fixture exercises the paths and prints the messages:CompileCpath), expected exactly1 errors while compiling <file>\ntcc: error: unresolved reference to 'bun_test_missing_symbol'\n(on macOS TinyCC stores and reports C symbols with a_prefix, so the expected name carries it there); this one passes before and after and pins the format of the untouched formatting path;relative.c(CompileCpath), expected exactly1 errors while compiling relative.c\n relative.c:1: error: ';' expected (got 'b')\n; before the change the second line loses its leading space (a non-ASCII name shows the same thing but cannot be opened through TinyCC's narrowopen()on Windows, hence the space);JSFunctionCall, which collides with the wrapper's entry point so the user's C compiles and the wrapper does not (Functionpath), expected<string>:<line>: error: ... 'JSFunctionCall';y ñ(asm labels are taken verbatim, so on macOS the label itself carries the_), so the wrapper's declaration of it is a syntax error quoting the non-ASCII token (Functionpath), expected<string>:<line>: error: ... 'ñ'; this still distinguishesinit_utf8frominitonce Copy the message string when constructing Error/AggregateError from a ZigString #31451 copies untagged strings, since that copy is Latin-1.AMM:371: error: incompatible types for redefinition of 'JSFunctionCall'andPm.M:371: error: ';' expected (got 'ñ')(the 8-byte<string>prefix is exactly what gets overwritten), and the relative case loses its leading space, so the test fails on all three.bun bd(ASan): the fixture dies withheap-use-after-free, freed atFFI::bun_ffi_cc(ffi_body.rs:1271, the drop ofcompile_c), so the test fails.bun bd test test/js/bun/ffi/cc.test.tspasses, including under ASan (the syntax-error cases go through TinyCC's longjmp and are clean there); the rest oftest/js/bun/ffi/is unchanged.Background
cc()compiles the user's C into one TinyCC state (CompileC::compile), then, for each requested symbol, generates a small C wrapper (Function::print_source_code, which declares the user's function by name and definesJSFunctionCall) and compiles it from a string in a second TinyCC state (Function::compile). TinyCC delivers diagnostics through the error callback registered on each state. The user-source state's callback collects them andcc()formats them into a fresh string under aN errors while compiling <file>header (that path copies, and was only affected by the loop); the wrapper state's callback stores the text inStep::Failed { msg }, whichcc()throws as-is (the path fixed here).ZigStringis a borrowed pointer+length with flag bits in the pointer.toErrorInstancedecodes and copies the bytes when the UTF-8 flag is set; an untagged string is treated as Latin-1 and wrapped withStringImpl::createWithoutCopying, which is meant for static text.initleaves the string untagged;init_utf8sets the flag.nextpointer into the first 8 bytes of the block and leaves the rest alone. The values seen in CI (for example90 4b 7c 4e 0d 05 00 00, a pointer in the address range mimalloc reserves) are such pointers.Related sites (not changed here)
src/jsc/AsyncModule.rsbuilds two errors from a localVecthrough the same untaggedto_error_instance; Copy the message string when constructing Error/AggregateError from a ZigString #31451 is the fix for those.ffi_body.rsdescribed under Fix; filed separately.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/cc.test.ts