UAF of a suspended coroutine referenced only via a closure#1569
Conversation
|
@bnoordhuis Here is the context for the GC-stress PR. When I added it to txiki.js one of my tests started to fail. This is the smallest possible repro. I have an AI generated fix but I'm not super-confident about its correctness here. |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
I don't know about this approach. Apart from all the other changes, enlarging JSVarRef is undesirable.
It's only possible when the async function's promise is not awaited, right? What about putting all live promises on a linked list and keep them artificially alive during GC?
Pro: also a useful tool for debugging promise leaks on context destruction.
Con: adds a O(n) step to the GC phase.
Another approach/hack: artificially increase the promise's reference count by one when it's created, then decrement it when it's awaited, then'd, etc. In other words, prevent it from getting reclaimed unless it's consumed by something else.
| // A closure capturing a coroutine local holds an *open* var_ref into the | ||
| // coroutine's frame. Open var_refs are not GC objects and are not marked by | ||
| // the closure, so the edge closure -> open var_ref -> value is invisible to | ||
| // the cycle collector. An ordinary function's frame is a live C-stack root, but | ||
| // a suspended coroutine's frame is a heap GC object, so if the only thing | ||
| // keeping it reachable is such an escaped closure, the whole still-live | ||
| // coroutine used to be collected -- and resuming it, or running the closure, | ||
| // then touched freed memory. |
There was a problem hiding this comment.
Is this comment LLM-generated? I know how the underlying mechanism works but the comment is worded in a way that still confuses me...
|
Thanks for taking a look! Those sound like good ideas, I'll take a look! |
A suspended coroutine (async function, generator or async generator) keeps its activation frame in a heap-allocated GC object. When a closure captures one of the coroutine's locals it holds an *open* var_ref that points into that frame. Open var_refs were not GC objects and were not traced by their holders, so the edge closure -> var_ref -> coroutine frame was invisible to the cycle collector: a coroutine reachable only through such a closure was collected while still live, and resuming it (or running the closure) then touched freed memory. Make an open var_ref that captures a coroutine local a GC object holding a counted reference to the owning coroutine (JSStackFrame.cur_gc_obj) and mark it from every var_ref holder, so the collector sees the edge and keeps the suspended coroutine (and the frame the var_ref points into) alive.
6469041 to
46a3d0f
Compare
|
@bnoordhuis I did another pass (cleanup pending). The new approach doesn't extend the size of I couldn't get your proposal(s) to work because the bug also affects 2 more case without promises involved: plain generators and the mapped arguments fast path. |
| (e.g. mapped `arguments`) must stay non-coro even though cur_gc_obj is | ||
| later set. Fits existing padding, so JSVarRef does not grow. Cleared | ||
| when the var_ref is detached; the ref is released on detach/free. */ | ||
| uint8_t is_coro; |
There was a problem hiding this comment.
Isn't this implied by !var_ref->detached && var_ref->stack_frame->cur_gc_obj != NULL? If that's the case, it'd be better to remove is_coro and have something like bool js_is_coro(JSVarRef *vr).
| if (JS_GC_TYPE(coro) == JS_GC_OBJ_TYPE_ASYNC_FUNCTION) | ||
| js_async_function_free(rt, (JSAsyncFunctionData *)coro); | ||
| else /* generator / async generator: a plain JS object */ | ||
| JS_FreeValueRT(rt, JS_MKPTR(JS_TAG_OBJECT, (JSObject *)coro)); |
There was a problem hiding this comment.
Maybe assert that JS_GC_TYPE(coro) == JS_GC_OBJ_TYPE_JS_OBJECT here? Or turn the if/else into a switch statement with a default: abort(); clause?
2 participants
No description provided.