Skip to content

Commit 491f84c

Browse files
committed
Fix dangling ptr in thread termination (GH-1845)
1 parent 2efbee8 commit 491f84c

File tree

1 file changed

+26
-6
lines changed

1 file changed

+26
-6
lines changed

src-input/duk_js_executor.c

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1641,18 +1641,38 @@ DUK_LOCAL duk_small_uint_t duk__handle_return(duk_hthread *thr, duk_activation *
16411641

16421642
resumer = thr->resumer;
16431643

1644-
/* Share yield longjmp handler. */
1645-
DUK_ASSERT(thr->valstack_top - 1 >= thr->valstack_bottom);
1646-
duk_hthread_activation_unwind_norz(resumer);
1647-
duk__handle_yield(thr, resumer, thr->valstack_top - 1);
1644+
/* Share yield longjmp handler.
1645+
*
1646+
* This sequence of steps is a bit fragile (see GH-1845):
1647+
* - We need the return value from 'thr' (resumed thread) value stack.
1648+
* The termination unwinds its value stack, losing the value.
1649+
* - We need a refcounted reference for 'thr', which may only exist
1650+
* in the caller value stack. We can't unwind or reconfigure the
1651+
* caller's value stack without potentially freeing 'thr'.
1652+
*
1653+
* Current approach is to capture the 'thr' return value and store
1654+
* a reference to 'thr' in the caller value stack temporarily. This
1655+
* keeps 'thr' reachable until final yield/return handling which
1656+
* removes the references atomatically.
1657+
*/
16481658

1649-
duk_hthread_terminate(thr); /* updates thread state, minimizes its allocations */
1650-
DUK_ASSERT(thr->state == DUK_HTHREAD_STATE_TERMINATED);
1659+
DUK_ASSERT(thr->valstack_top - 1 >= thr->valstack_bottom);
1660+
duk_hthread_activation_unwind_norz(resumer); /* May remove last reference to 'thr', but is NORZ. */
1661+
duk_push_tval(resumer, thr->valstack_top - 1); /* Capture return value, side effect free. */
1662+
duk_push_hthread(resumer, thr); /* Make 'thr' reachable again, before side effects. */
16511663

1664+
duk_hthread_terminate(thr); /* Updates thread state, minimizes its allocations. */
16521665
thr->resumer = NULL;
16531666
DUK_HTHREAD_DECREF(thr, resumer);
1667+
DUK_ASSERT(thr->state == DUK_HTHREAD_STATE_TERMINATED);
1668+
16541669
resumer->state = DUK_HTHREAD_STATE_RUNNING;
16551670
DUK_HEAP_SWITCH_THREAD(thr->heap, resumer);
1671+
1672+
DUK_ASSERT(resumer->valstack_top - 2 >= resumer->valstack_bottom);
1673+
duk__handle_yield(thr, resumer, resumer->valstack_top - 2);
1674+
thr = NULL; /* 'thr' invalidated by call */
1675+
16561676
#if 0
16571677
thr = resumer; /* not needed */
16581678
#endif

0 commit comments

Comments
 (0)