Conversation
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
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.
QuickJSAsyncRuntime.newContextsilently dropsoptions.contextPointer, while the baseQuickJSRuntime.newContexthonors it:Three call sites pass that option in order to wrap a context the engine already handed us —
executePendingJobs()and the module loader / normalizer callbacks inruntime.ts, all shaped like:On asyncify variants the override therefore creates a brand new
JSContextinstead of wrappingthe existing one:
JS_FreeRuntime, whichasserts
list_empty(&rt->gc_obj_list)— an uncatchable WASMabort(), i.e. untrusted JS cankill the host process.
Relation to #269 and #248
While root-causing #269 (that same assertion, reachable from a plain promise job) we found that
executePendingJobscan reach this fallback with a garbage pointer: the out-parameter view iscreated before
QTS_ExecutePendingJob, and if the job grows the WASM memory the view isdetached, so
typedArray[0]readsundefinedrather than0. ThectxPtr === 0guard missesundefined,contextMap.get(undefined)misses as well — and this override turns that miss into aleaked context.
#248 fixes the detach half of that chain. This PR fixes the other half, which remains reachable
through the module-loader paths even with #248 applied.
For reference, the full chain reproduces deterministically (160k+ objects in a single promise job →
Aborted(Assertion failed: list_empty(&rt->gc_obj_list), at: quickjs.c, JS_FreeRuntime)); with bothhalves fixed, 160k/200k/500k all tear down cleanly. I can share the reproducer if useful.
Tests
No test added: my environment can't run the suite (it needs a full
pnpm install, which fails hereon an unrelated peer-dependency conflict). Happy to add one along the lines of "
newContext({ contextPointer: ptr })returns a context whose
ctx.value === ptron an asyncify runtime" — just tell me which file you'dprefer it in.