Repository navigation
[JSC] The conservative scan does not read stack words ASan has poisoned - #673
Merged
Merged
Conversation
Under ASan an instrumented frame has a redzone between its locals. No code may write one, so whatever an earlier frame left in that stack memory stays there for as long as the frame is live, and sanitizeStack only zeroes below the stack pointer. The conservative scan read it as a root. MicrotaskQueue::drainImpl's frame is live for the whole of a module's top-level-await body. With clang 23 its ASan frame has microtaskCallCache at 32..488 and task at 560..600, and a test's last Subprocess cell sat at 512: no incoming edge and no root entry in a heap snapshot (Strong handles are reported), one stack word holding its address, in that redzone. The cell was marked at every collection and its finalizer never ran. Which cell lands in which redzone depends on frame layout, hence on the compiler. A word ASan reports poisoned (a redzone, a local past its scope, the unused capacity of an annotated container) cannot hold a live value, and a pointer needs all eight bytes addressable, so ASan builds skip such words: in the scan loop for a stack read in place, and in copyMemory for another thread's stack, whose copy has no poison of its own. MicrotaskCallCache already zeroes its storage for the same reason; this covers the memory no constructor can reach. Builds without ASan compile to the same code.
|
Preview build of 0e72202: |
|
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 (3)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. WalkthroughThe change adds ASan-aware poisoned-word detection for conservative scanning. Conservative root scans skip poisoned words, and copied CPU-register words are zeroed when poisoned. ChangesConservative scan handling
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The ASan conservative-scan handling is covered across in-place and copied-stack paths, with no unresolved merge-blocking concern 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Sep 16, 2026
…n has poisoned oven-sh/WebKit#673. On the x64-asan lane the last object a test created was never collected (terminal.test.ts here; serve-pending-promise-abort-leak on main): a stale cell pointer sat in the ASan redzone between two locals of MicrotaskQueue::drainImpl's frame, which is live for a module's whole top-level-await body, and the conservative scan read it as a root. No-Verification-Needed: version bump
robobun
added a commit
that referenced
this pull request
Sep 16, 2026
…ng to occupy A microtask checkpoint is where a turn of the embedder's event loop resumes an async function: a timer or an I/O callback settles a promise, and the queue is drained. Every turn does that from the same stack depth, so the frames of the checkpoint (MicrotaskQueue::drain*(), runInternalMicrotask(), the job's, the entry to JS) are at the same addresses in every turn. They stay for as long as the turn's JS runs, and they lie over whatever ran at that depth since the last checkpoint: that checkpoint's own frames, or the embedder's. What they do not write is in reach of the conservative scan of every collection made from under them, and sanitizeStackForVM(), which such a collection calls, clears only what is below the frame that calls it. #673 took the words that ASan poisons (redzones, locals out of scope) out of the scan. This is about the words it cannot take out: spill slots and locals of a live frame that this job's path through the function does not write. Builds without ASan have only those, and ASan builds have them too. runInternalMicrotask() is one switch over every kind of job, with callMicrotask() inlined into several arms, and a job uses one arm. Seen as: what a finished async function held is not collected by collections made from later turns. async function makeGarbage() { /* two objects in a list, one await per object */ } await makeGarbage(); for (;;) { await turn(); fullGC(); } // turn(): a promise that a timer resolves jsc shell of c281568 (linux amd64, lto): 32 turns until the objects are collected (the loop tiers up and the frames change), 60 of 60 turns not collected with --useJIT=0. Bun at that WebKit (release, x86_64): not collected in 8 of 8 turns when the timer is Bun.sleep(1); Bun's debug ASan build, which has #673: not collected in 8 of 8 turns for the module shape with any timer. A heap snapshot has the objects held by `list`, held by the lexical environment of the finished function's JSAsyncFunctionGenerator, which has no incoming edge and no root. In a debugger, the one word of the scanned span that holds the generator's address is at rbp-64 of runInternalMicrotask()'s frame (272 bytes), under drainWithUseCallOnEachMicrotask() (624 bytes). The job that runs in the later turns (AsyncModuleExecutionResume for the module, AsyncFunctionResume when the loop is in an async function of its own) does not write that slot. Which shape shows it moves with the compiler: the same script with setTimeout() kept the objects with clang 21 and does not with clang 23. Earlier sightings of this frame in Bun's leak tests: oven-sh/bun#37853 (the same generator word), oven-sh/bun#41607 (darwin arm64, release). So a checkpoint that has jobs to run first clears MicrotaskQueue::stackBytesClearedForCheckpoint bytes of the stack below performMicrotaskCheckpoint(), before the first of those frames exists: 2 KB in a release build (from there to the JS frame is 1.5 KB for x86_64), 32 KB with assertions or ASan (12.6 KB with both). It does that with a function that is never inlined and whose frame is an array of that size, which it zeroes with memset(): that frame lies exactly where the caller's next callee has its own, on every ABI, and the compiler probes the stack for it where a platform needs that. The array's address goes through an empty asm statement. Without it the stores are dead to the compiler (zeroBytes() and secureZeroBytes() both compile to a bare `ret` here: the memory clobber of secureZeroSpan() does not keep stores to a local whose address does not escape). callMicrotask() asserts that it runs within seven eighths of the window below the checkpoint's MicrotaskCallCache, which is a local of drainImpl(). Not sanitizeStackForVM() at the checkpoint: it clears from where it was last called, and nothing need have called it from below the checkpoint since. The jsc shell does not change with it (JSLock::didAcquireLock() resets VM::m_lastStackTop for every task). Not sanitizeStackForVMImpl() with m_lastStackTop lowered to the bottom of the window, which was the first version: its loop for x86_64 stores 8 bytes per iteration, and a checkpoint with one job went from 106 ns to 195 ns. Cost, Xeon 8375C, clang 23: `promise.then(noop); drainMicrotasks()` 5 M times in the jsc shell, 101 ns per iteration without the clear and 122 ns with it. Bun, 1 M turns of `await new Promise(r => setImmediate(r))`: 2531 ms without and 2547 ms with (medians of 11, same binary, the option). 20 M awaits in one checkpoint: 602 ms and 602 ms. An empty checkpoint clears nothing. Once per checkpoint and not once per job, which would cost every job that much: what one job leaves is still there for the later jobs of the same checkpoint, and is gone at the next one. Not when the window would reach below the soft stack limit. An embedder that holds the API lock for the life of its thread can get much of this from a sanitizeStackForVM() per event loop turn instead (oven-sh/bun#37853): in Bun that collects the same six cases at the same cost. It depends on something having sanitized from below the stale word since it was written, clears per turn and not per checkpoint, and covers the embedder's own frames as well. The two do not exclude each other. Options::clearStackForMicrotaskCheckpoint (default on) is for comparisons. Tests: JSTests/modules/microtask-checkpoint-clears-its-stack.js (an async function, then the module) and JSTests/stress/microtask-checkpoint-clears-its-stack.js (a promise reaction, then an async generator, each followed by an async function). Both fail in the shell of c281568 and with --clearStackForMicrotaskCheckpoint=0, and pass in the 28 configurations that run-javascriptcore-tests runs them in (release, x86_64).
robobun
added a commit
that referenced
this pull request
Sep 16, 2026
…ng to occupy A microtask checkpoint is where a turn of the embedder's event loop resumes an async function: a timer or an I/O callback settles a promise, and the queue is drained. Every turn does that from the same stack depth, so the frames of the checkpoint (MicrotaskQueue::drain*(), runInternalMicrotask(), the job's, the entry to JS) are at the same addresses in every turn. They stay for as long as the turn's JS runs, and they lie over whatever ran at that depth since the last checkpoint: that checkpoint's own frames, or the embedder's. What they do not write is in reach of the conservative scan of every collection made from under them, and sanitizeStackForVM(), which such a collection calls, clears only what is below the frame that calls it. #673 took the words that ASan poisons (redzones, locals out of scope) out of the scan. This is about the words it cannot take out: spill slots and locals of a live frame that this job's path through the function does not write. Builds without ASan have only those, and ASan builds have them too. runInternalMicrotask() is one switch over every kind of job, with callMicrotask() inlined into several arms, and a job uses one arm. Seen as: what a finished async function held is not collected by collections made from later turns. async function makeGarbage() { /* two objects in a list, one await per object */ } await makeGarbage(); for (;;) { await turn(); fullGC(); } // turn(): a promise that a timer resolves jsc shell of c281568 (linux amd64, lto): 32 turns until the objects are collected (the loop tiers up and the frames change), 60 of 60 turns not collected with --useJIT=0. Bun at that WebKit (release, x86_64): not collected in 8 of 8 turns when the timer is Bun.sleep(1); Bun's debug ASan build, which has #673: not collected in 8 of 8 turns for the module shape with any timer. A heap snapshot has the objects held by `list`, held by the lexical environment of the finished function's JSAsyncFunctionGenerator, which has no incoming edge and no root. In a debugger, the one word of the scanned span that holds the generator's address is at rbp-64 of runInternalMicrotask()'s frame (272 bytes), under drainWithUseCallOnEachMicrotask() (624 bytes). The job that runs in the later turns (AsyncModuleExecutionResume for the module, AsyncFunctionResume when the loop is in an async function of its own) does not write that slot. Which shape shows it moves with the compiler: the same script with setTimeout() kept the objects with clang 21 and does not with clang 23. Earlier sightings of this frame in Bun's leak tests: oven-sh/bun#37853 (the same generator word), oven-sh/bun#41607 (darwin arm64, release). So a checkpoint that has jobs to run first clears MicrotaskQueue::stackBytesClearedForCheckpoint bytes of the stack below performMicrotaskCheckpoint(), before the first of those frames exists: 2 KB in a release build (from there to the JS frame is 1.5 KB for x86_64), 32 KB with assertions or ASan (12 KB with both). It does that with a function that is never inlined and whose frame is an array of that size, which it zeroes with memset(): that frame lies exactly where the caller's next callee has its own, on every ABI, and the compiler probes the stack for it where a platform needs that. The array's address goes through an empty asm statement. Without it the stores are dead to the compiler (zeroBytes() and secureZeroBytes() both compile to a bare `ret` here: the memory clobber of secureZeroSpan() does not keep stores to a local whose address does not escape). callMicrotask() asserts that it runs within seven eighths of the window below the checkpoint's MicrotaskCallCache, which is a local of drainImpl(). Not sanitizeStackForVM() at the checkpoint: it clears from where it was last called, and nothing need have called it from below the checkpoint since. The jsc shell does not change with it (JSLock::didAcquireLock() resets VM::m_lastStackTop for every task). Not sanitizeStackForVMImpl() with m_lastStackTop lowered to the bottom of the window, which was the first version: its loop for x86_64 stores 8 bytes per iteration, and a checkpoint with one job went from 106 ns to 195 ns. Cost, Xeon 8375C, clang 23: `promise.then(noop); drainMicrotasks()` 5 M times in the jsc shell, 101 ns per iteration without the clear and 122 ns with it. Bun, 1 M turns of `await new Promise(r => setImmediate(r))`: 2531 ms without and 2547 ms with (medians of 11, same binary, the option). 20 M awaits in one checkpoint: 602 ms and 602 ms. An empty checkpoint clears nothing. Once per checkpoint and not once per job, which would cost every job that much: what one job leaves is still there for the later jobs of the same checkpoint, and is gone at the next one. Not when the window would reach below the soft stack limit. An embedder that holds the API lock for the life of its thread can get much of this from a sanitizeStackForVM() per event loop turn instead (oven-sh/bun#37853): in Bun that collects the same six cases at the same cost. It depends on something having sanitized from below the stale word since it was written, clears per turn and not per checkpoint, and covers the embedder's own frames as well. The two do not exclude each other. Options::clearStackForMicrotaskCheckpoint (default on) is for comparisons. Tests: JSTests/modules/microtask-checkpoint-clears-its-stack.js (an async function, then the module) and JSTests/stress/microtask-checkpoint-clears-its-stack.js (a promise reaction, then an async generator, each followed by an async function). Both fail in the shell of c281568 and with --clearStackForMicrotaskCheckpoint=0, and pass in the 28 configurations that run-javascriptcore-tests runs them in (release, x86_64).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ASan builds only. Builds without ASan compile to the same code.
What was wrong
Two Bun tests on the x64-asan lane create N objects, drop them, and wait for N finalizers; N−1 ran and the last object never did:
test/js/bun/http/serve-pending-promise-abort-leak.test.ts— already red on Bun main (clang 21)test/js/bun/terminal/terminal.test.ts— red on Upgrade LLVM 21.1.8 → 23.1.1 and Rust nightly to 2026-09-15 bun#42851 (clang 23 + current main), 4/4 attempts, deterministic locallySame signature both times, and which test is hit moves with the compiler and with unrelated commits.
What holds the object
Standalone repro of the terminal test, release+asan, paused while the object is retained:
generateHeapSnapshotForDebugging): the survivingTerminal's only incoming edge isSubprocess.terminal; thatSubprocesscell has no incoming edge and no root entry. Strong handles are in that snapshot's root list (431 of them), so it is not the wrapper's ownJSRef.Subprocesscell's address. Walking the frame-pointer chain puts it in the frame ofJSC::MicrotaskQueue::drainWithUseCallOnEachMicrotask, atrbp-0xd0.2 32 456 22 microtaskCallCache:176 560 40 8 task:182—microtaskCallCacheat 32..488,taskat 560..600. The word is at offset 512: the redzone between them.No code may write a redzone, so whatever an earlier frame left in that stack memory stays for as long as the frame is live, and
sanitizeStackonly zeroes belowsp.drainImpl's frame is live for the whole of a module's top-level-await body, so the word was a root at every collection.MicrotaskCallCachealready zeroes its storage for exactly this reason ("an entry built over a slot an earlier frame left a cell pointer in…"). A redzone is the part of the frame no constructor can reach.The change
A word ASan reports poisoned — a redzone, a local past its scope, the unused capacity of an annotated container — cannot hold a live value, and a pointer needs all eight bytes addressable. So
isPoisonedForConservativeScan():ConservativeRoots::genericAddSpanfor a stack scanned in place;MachineThreads'copyMemorystore 0 for a poisoned source word, because when the collector thread has the conn it scans a copy of the mutator's stack, and the copy has no poison of its own.This is not a scrub and does not re-time anything: nothing is zeroed on the stack, and non-ASan builds are untouched.
Soundness
Could a live root be in poisoned memory? Looked for one and did not find it: JIT/LLInt/wasm frames are not instrumented and never poisoned; spills and callee-saves lie outside ASan's instrumented alloca; use-after-scope poison only covers locals the language already ended (a value still live is in a register, scanned separately);
Vector's container annotation unpoisons a slot before writing it;-fsanitize-address-use-after-return=never, so no fake stacks; every other caller ofadd()passes fully addressable memory.__asan_region_is_poisonedtakes no lock, so it is safe incopyMemorywhile threads are suspended.Verified
Bun
release-asanbuilt against this tree (clang 23.1.1):terminal.test.tsserve-pending-promise-abort-leak.test.tsPlus streams, serve, websocket, worker_threads, spawn, shell, ffi, AsyncLocalStorage, inspect, bundler (~2100 tests): zero AddressSanitizer reports, and the only failures are ones the unpatched binary has too (a root-only port test, an FTL test that times out under local ASan).