diff --git a/RELEASES.rst b/RELEASES.rst index 5852e68f05..fcc35b08c8 100644 --- a/RELEASES.rst +++ b/RELEASES.rst @@ -3281,6 +3281,9 @@ Planned * Include only when compiling as C++ with C++ exception support enabled (GH-1838, GH-1839) +* Fix potential dangling pointer use in Duktape thread termination handling; + the dangling pointer could cause unsafe memory behavior (GH-1845, GH-1868) + * Fix performance.now() property attributes to 'wec' (earlier 'wc') (GH-1821) * Fix debugger StepOver behavior when a tailcall happens in a nested diff --git a/src-input/duk_js_executor.c b/src-input/duk_js_executor.c index e3b94052fc..b28a77c81e 100644 --- a/src-input/duk_js_executor.c +++ b/src-input/duk_js_executor.c @@ -1644,18 +1644,38 @@ DUK_LOCAL duk_small_uint_t duk__handle_return(duk_hthread *thr, duk_activation * resumer = thr->resumer; - /* Share yield longjmp handler. */ - DUK_ASSERT(thr->valstack_top - 1 >= thr->valstack_bottom); - duk_hthread_activation_unwind_norz(resumer); - duk__handle_yield(thr, resumer, thr->valstack_top - 1); + /* Share yield longjmp handler. + * + * This sequence of steps is a bit fragile (see GH-1845): + * - We need the return value from 'thr' (resumed thread) value stack. + * The termination unwinds its value stack, losing the value. + * - We need a refcounted reference for 'thr', which may only exist + * in the caller value stack. We can't unwind or reconfigure the + * caller's value stack without potentially freeing 'thr'. + * + * Current approach is to capture the 'thr' return value and store + * a reference to 'thr' in the caller value stack temporarily. This + * keeps 'thr' reachable until final yield/return handling which + * removes the references atomatically. + */ - duk_hthread_terminate(thr); /* updates thread state, minimizes its allocations */ - DUK_ASSERT(thr->state == DUK_HTHREAD_STATE_TERMINATED); + DUK_ASSERT(thr->valstack_top - 1 >= thr->valstack_bottom); + duk_hthread_activation_unwind_norz(resumer); /* May remove last reference to 'thr', but is NORZ. */ + duk_push_tval(resumer, thr->valstack_top - 1); /* Capture return value, side effect free. */ + duk_push_hthread(resumer, thr); /* Make 'thr' reachable again, before side effects. */ + duk_hthread_terminate(thr); /* Updates thread state, minimizes its allocations. */ thr->resumer = NULL; DUK_HTHREAD_DECREF(thr, resumer); + DUK_ASSERT(thr->state == DUK_HTHREAD_STATE_TERMINATED); + resumer->state = DUK_HTHREAD_STATE_RUNNING; DUK_HEAP_SWITCH_THREAD(thr->heap, resumer); + + DUK_ASSERT(resumer->valstack_top - 2 >= resumer->valstack_bottom); + duk__handle_yield(thr, resumer, resumer->valstack_top - 2); + thr = NULL; /* 'thr' invalidated by call */ + #if 0 thr = resumer; /* not needed */ #endif