[JSC] A direct tail call to a host function can return into freed JIT code - #649
Conversation
… code DFG and FTL inline the host call thunk into the caller's own code for a direct call to a NativeExecutable (273947@main). For a tail call that is not safe: the tail call destroys the caller's frame, so while the host function runs nothing on the machine stack refers to the caller's CodeBlock. The conservative stack scan cannot add it to CodeBlockSet::m_currentlyExecuting, which is what keeps executing code alive. A watchpoint fire plus a collection inside the host call then jettisons the caller and frees its machine code. The host call returns into that memory. Emit the inline thunk only for a call that keeps its frame. A direct tail call links through DirectCallLinkInfo again. Its target for a host function is the executable's host call thunk, which lives as long as the VM, and that thunk returns to the caller's caller. * JSTests/stress/direct-tail-call-to-host-function-code-freed-during-call.js: Added. * Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp: (JSC::DFG::SpeculativeJIT::emitCall): * Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked that in both tiers the removed fast path falls through to the pre-existing DirectCallLinkInfo / emitDirectTailCallFastPath code (DFGSpeculativeJIT64.cpp:1071, FTLLowerDFGToB3.cpp:14139) — this is a targeted revert of the tail-call half of 273947@ main, not new codegen. Also confirmed FTL's emitCallTarget lambda is still reached by the non-tail native path at FTLLowerDFGToB3.cpp:14167, so no dead code was introduced.
Extended reasoning...
The fix disables the inline host-call thunk only for the isTail case in both DFG and FTL, letting direct tail calls to native functions fall back to the DirectCallLinkInfo path that predates 273947@ main. I read the surrounding ~140 lines in each file to confirm the fallthrough targets are the unchanged, pre-existing tail-call linking code, and that the FTL emitCallTarget lambda (defined before the isTail branch) is still consumed by the non-tail native path so the removal doesn't leave it dead. The engine change itself is small, well-commented, and mirrored across both tiers as JSC conventions require; the only outstanding issue is the stress test's hardcoded iteration count, already flagged inline.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughDFG and FTL lowerers now route native tail calls through the direct linked path instead of the current CodeBlock host thunk. A stress test verifies behavior when optimized caller code is freed during argument stringification. ChangesHost-function tail-call lifetime
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
JSTests/README.md rule 2. The two configurations pin the tier-up thresholds, so testLoopCount is 3000 in both and each run takes about 11 ms on a build with the fix. Stock jsc still fails both 5 of 5.
main has the same bug fixed another way (#649): a direct tail call to a host function no longer takes the inline call path, and links through the executable's host call thunk, which lives as long as the VM and returns to the caller's caller. It comes with a regression test that frees the caller's code deterministically. This branch kept the inline call and parked the CodeBlock's pointer in a stack slot across it (f66e3bc). Both fix the crash. The inline tail call is about 0.7 ns faster per call (17.6 ns against 18.4 ns); main's version leaves no JIT code running without a frame that names its CodeBlock, which is the assumption CodeBlockSet's currently-executing set states. DFGSpeculativeJIT64.cpp and FTLLowerDFGToB3.cpp are main's.
The preview release autobuild-preview-pr-645-b6d2430a: #645 merged with oven-sh/WebKit main, which brings the fix for a direct tail call to a host function returning into freed JIT code (oven-sh/WebKit#649). Temporary: once #645 is merged this becomes the sha of the autobuild from main.
The preview tag goes away now that the WebKit PR has merged. The new pin is fork main. It also picks up the five other commits that landed there since cf1b36ec8703: oven-sh/WebKit#636, #634, #632, #652 and #646. Bun builds against the new headers with no source change.
main pins 9b02218df662 (#42556), one commit past 3cf2a3dfd259. This branch keeps main's pin and carries only the test.
Problem
b99371011, linux x64 release) crashes 6 of 6 runs on the script in the notes:panic(main thread): Segmentation fault at address 0x0,Illegal instruction at address 0x7f..., orabort() called.BUN_JSC_useJIT=0prints the expected output.NativeExecutable(dfg/DFGSpeculativeJIT64.cpp:1043,ftl/FTLLowerDFGToB3.cpp:14132, added in 273947@main). A tail call destroys the caller's frame, so while the host function runs nothing on the machine stack refers to the caller'sCodeBlock.CodeBlockSet::m_currentlyExecuting. A watchpoint fire plus a collection inside the hook jettisons the caller and frees its code. The host call returns into that memory.Fix
DirectCallLinkInfoagain, as it did before 273947@main.ExecutableBase::generatedJITCodeWithArityCheckForCall(DirectCallLinkInfo::repatchSpeculatively). That thunk lives as long as the VM, and it returns to the caller's caller, not into the caller's code.CodeBlock, and the scan marks it.JSTests/stress/direct-tail-call-to-host-function-code-freed-during-call.js. Stockjscfails it 5 of 5 in both configurations it runs, this build 0 of 5. 379 call, tail-call and inlining stress tests give identical output on both builds.Background
CodeBlockpointer, is gone.CodeBlockSetInlines.hstates the assumption:m_currentlyExecuting"is strongly assuming that this catches all the currently executing CodeBlock".CodeBlockin its executable. The block then lives only as long as the stack keeps it, and its machine code is freed with it.Notes
Bun repro (
bun g.js, release build of bun main09bb5463010/10, 1.4.2 5/6, 1.4.3 canary 6/6, no engine options):What each ingredient does:
hot()is hot enough for DFG and FTL.Bun.inspectis a constant, so the call is aDirectTailCallto aNativeExecutableand the compiler inlines the host call thunk intohot's code. The dump shows the node and the caller's code range.Bun.gcinside the hook materializes a lazy property on theBunobject. The structure transition fires the watchpointhot's code holds for that structure, which jettisonshot.Bun.gc(true)collects. Nothing marks the jettisonedCodeBlock, so the code is freed.churn()makes the JIT reuse the memory.Bun.inspectthen returns into it.Engine-level test:
gc()inside the hook is enough. The test uses--zeroExecutableMemoryOnFree=1, which fills freed code with zeroes, so the return into it crashes every run instead of depending on what reuses the memory. Without that option the same test crashes 2 of 5 on this build and 0 of 5 on an LTO build.--useFTLJIT=0, because the DFG site and the FTL site both emit the thunk. Stockjscfails both.const r = o.f(v); return r + "";) passes on stockjsc, as expected: the caller's frame is still on the stack.Why a given case crashes or not:
CodeBlockpointer when the scan runs. That is a property of the stack layout, so the same script crashes on one build and not on another.CodeBlockis found in a dead stack slot, so the code is not freed. On a crashing run it is not found at all.Bun.inspect,Bun.deepEquals,Bun.deepMatch,Bun.inspect.table,Bun.YAML.stringify,Bun.JSON5.stringify,Bun.TOML.stringify,Bun.gzipSync,Bun.deflateSync,Bun.escapeHTML,Bun.markdown.htmlandBun.indexOfLine, and survives throughBun.stringWidth,Bun.stripANSI,Bun.sliceAnsi,Bun.wrapAnsiandBun.zstdCompressSync. The DFG and FTL dumps of a crashing door and a surviving door are the same, down to theDirectTailCall(<NativeExecutable>, <host function>)node.Stress test sweep on linux x64 (RelWithDebInfo,
--useDollarVM=1, default options): every file inJSTests/stresswhose name matchestail,direct-call,call-link,poly-call,host-call,^dfg-.*call,^ftl-.*call,apply,spread,varargs,bound-functionorinlin, 379 files, same exit status and same output before and after.Bun PR that pins the preview build of this branch and adds the Bun test: to follow.