Fix two QuickJS GC use-after-frees (handle-wrap teardown + async-generator closures)#101
Merged
Conversation
EdgeJS QuickJS aborted ("double free / heap corruption") under allocation
pressure (e.g. running Etherpad, or rapid process signal-listener churn).
ASAN traced it to a use-after-free: a handle wrap's uv close callback
(OnClosed) runs a JS "onclose" callback mid-teardown; that JS re-enters
the GC, whose cycle collector runs the wrap's napi finalizer, which frees
the wrap out from under the still-executing close callback.
Root cause was structural: each handle wrap (signal/udp/process/...) hand-
rolled its own OnClosed + finalizer over a shared helper *toolbox*, and the
implementations drifted -- signal_wrap never guarded the re-entrant
finalize that the others happened to avoid.
This centralizes the dangerous teardown into two shared helpers,
EdgeHandleWrapCompleteClose and EdgeHandleWrapFinalizeNative, which own the
ordering once: an in_close_callback guard makes the finalizer defer freeing
while a close is on the stack, and deletion happens exactly once afterwards.
signal_wrap now delegates its OnClosed/finalizer to them, so the bug cannot
recur there and new wraps can adopt the same safe path instead of
re-deriving it.
Verified with an ASAN build: a minimal signal-churn + gc() repro and real
SIGUSR2 delivery both pass; the signal UAF no longer reproduces.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Migrate the remaining handle wraps -- process, udp, fs-event, fs StatWatcher, and MessagePort -- off their hand-rolled OnClose/finalizer implementations and onto EdgeHandleWrapCompleteClose / EdgeHandleWrapFinalizeNative (added with the signal fix). This removes the duplicated, drift-prone teardown logic that caused the signal use-after- free: the re-entrancy guard and deletion ordering now live in one place for every wrap. Each wrap now provides only a small deleter (its async-destroy / queued- message / owner-ref cleanup + delete) and thin OnClose/finalizer trampolines. The per-wrap EdgeHandleWrapHoldWrapperRef "pin" is dropped: it is redundant with EdgeRegisterActiveHandle's strong keepalive_ref (the real keep-alive while a handle is active) and, unlike the guard, did not actually protect against the QuickJS cycle collector. Verified under ASAN: signal/process/udp/MessagePort churn + gc() repros are all clean, and the signalwrap/statwatcher/pipewrap/tcpwrap Node async-hook tests pass. (test-worker-message-port still fails on a pre-existing ArrayBuffer-transfer-detach gap, unrelated to handle teardown.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings in wasmerio/napi#36 -> wasmerio/quickjs#6, porting the CVE-2023-48184 / bellard#156 fix to our quickjs-ng tree. The cycle collector no longer frees a Promise resolving function still referenced by a suspended async generator's closure var_ref (heap-use-after-free in gc_decref_child during a later GC). Surfaces in async-heavy apps iterating streams (Etherpad, undici); reproduces with `for await (const x of socket)` under GC pressure. This complements the handle-wrap teardown fix already in this branch; both are QuickJS GC use-after-free fixes. NOTE: not final until wasmerio/quickjs#6 and wasmerio/napi#36 merge; update the napi pointer to the merged commit afterward. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Arshia001
changed the title
Pulls in the quickjs fix (via napi 8bfea8b -> quickjs 14bf204) for a heap-use-after-free where open closure var_refs could outlive the async frame they point into when an incomplete async function/generator is freed during cycle collection. Reproduced deterministically by test-stream-iterator-helpers-test262-tests; ASAN-clean after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Two independent QuickJS cycle-GC use-after-free fixes found while bringing up async-heavy framework tests (Etherpad). No framework-test changes are included here.
1. Handle-wrap finalize/close re-entrancy (this repo,
src/)A napi external finalizer could run re-entrantly during a libuv close callback and free a handle wrap mid-teardown (originally seen as a signal-wrap double-free at shutdown). Fixed by routing all handle wraps through a single centralized close/finalize teardown with an
in_close_callbackguard, so finalize and close can't free out from under each other.cafec1dfFix signal-wrap GC use-after-free via centralized handle teardown43767ecdRoute all handle wraps through the shared close/finalize teardown2. Async function/generator closure cycle-GC UAF (submodule bump)
The cycle collector could free a Promise resolving function still referenced by a suspended async generator's closure var_ref → heap-use-after-free in
gc_decref_childduring a later GC. It surfaces in async-heavy apps iterating streams (Etherpad, undici) and reproduces deterministically withfor await (const x of socket)under GC pressure.Root cause: this is CVE-2023-48184 / bellard/quickjs#156 ("incorrect GC of async functions with closures"), fixed in bellard via commit
7414e5f(2024-01-13) but never adopted by quickjs-ng (the fork we vendor still ships the oldasync_func_mark/JSAsyncFunctionDatadesign onmaster). The fix ports that restructuring to our quickjs-ng tree, adapted to quickjs-ng's array-basedsf->var_refs[].Follow-up (regression in the port): the ported design stores a frame's
var_refs[]array inside itsarg_bufallocation, which opened a new cycle-teardown ordering hole — when an incomplete async generator is collected as part of a cycle,async_func_free_frame()freesarg_bufwhile open closure var_refs still point into it, so a laterfree_var_ref()writes into freed memory (heap-UAF,corrupted size vs. prev_size). This reproduced deterministically viatest-stream-iterator-helpers-test262-tests(the CI failure on this PR) and is fixed by neutralizing still-open var_refs into detached/empty refs before the frame is freed. Verified: 1/1 crash → 30/30 pass + ASAN-clean.5e33a307Bump napi → Bump quickjs: cycle-GC use-after-free fix for async closures napi#36 → Fix cycle-GC use-after-free of async functions/generators with closures quickjs#6 (async-closure cycle-GC fix + the follow-up ordering fix)The
napisubmodule pointer here references the unmerged quickjs fix via the PR branch. This PR is not final until:napipointer in this PR is updated to that merged napi commit.Dependency chain: wasmerio/quickjs#6 → wasmerio/napi#36 → this PR.
🤖 Generated with Claude Code