From 8b2bbb5135dbc17fbfe22ae634bce0d7e34451fa Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Tue, 27 Oct 2015 23:38:01 +0200 Subject: [PATCH] Delay detached_cb to between messages Without this it's possible for the following to happen: - The debug transport gets broken by e.g. a syntax error during message handling. - The detached_cb is called when marking transport bad. The detached callback immediately reattaches. - The debug command processing continues, potentially reading from and writing to the *new connection* which causes the connection to go out of sync. Delaying the detached_cb call to the message loop, right after one message has been fully processed, avoids this issue and keeps the connections cleanly separated. The solution is a bit awkward: duk_debug_do_detach() is split into two. Outside callers (duk_debugger_detach(), duk_heap_free()) still have a single function helper to hide the split. --- src/duk_api_debug.c | 6 +++- src/duk_debugger.c | 76 +++++++++++++++++++++++++++++++-------------- 2 files changed, 57 insertions(+), 25 deletions(-) diff --git a/src/duk_api_debug.c b/src/duk_api_debug.c index 27a32e64..17a5f0c3 100644 --- a/src/duk_api_debug.c +++ b/src/duk_api_debug.c @@ -53,6 +53,10 @@ DUK_EXTERNAL void duk_debugger_attach(duk_context *ctx, const char *str; duk_size_t len; + /* XXX: should there be an error or an automatic detach if + * already attached? + */ + DUK_ASSERT_CTX_VALID(ctx); DUK_ASSERT(read_cb != NULL); DUK_ASSERT(write_cb != NULL); @@ -104,7 +108,7 @@ DUK_EXTERNAL void duk_debugger_detach(duk_context *ctx) { DUK_ASSERT(thr != NULL); DUK_ASSERT(thr->heap != NULL); - /* Can be called muliple times with no harm. */ + /* Can be called multiple times with no harm. */ duk_debug_do_detach(thr->heap); } diff --git a/src/duk_debugger.c b/src/duk_debugger.c index ee702f96..d31a57c8 100644 --- a/src/duk_debugger.c +++ b/src/duk_debugger.c @@ -25,25 +25,27 @@ typedef union { #define DUK__SET_CONN_BROKEN(thr) do { \ /* For now shared handler is fine. */ \ - duk_debug_do_detach((thr)->heap); \ + duk__debug_do_detach1((thr)->heap); \ } while (0) -DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) { - duk_debug_detached_function detach_cb; - void *detach_udata; - - /* Can be called multiple times with no harm. */ +DUK_LOCAL void duk__debug_do_detach1(duk_heap *heap) { + /* Can be called multiple times with no harm. Mark the transport + * bad (dbg_read_cb == NULL) and clear state except for the detached + * callback and the udata field. The detached callback is delayed + * to the message loop so that it can be called between messages; + * this avoids corner cases related to immediate debugger reattach + * inside the detached callback. + */ - detach_cb = heap->dbg_detached_cb; - detach_udata = heap->dbg_udata; + DUK_D(DUK_DPRINT("debugger transport detaching, marking transport broken")); heap->dbg_read_cb = NULL; heap->dbg_write_cb = NULL; heap->dbg_peek_cb = NULL; heap->dbg_read_flush_cb = NULL; heap->dbg_write_flush_cb = NULL; - heap->dbg_detached_cb = NULL; - heap->dbg_udata = NULL; + /* heap->dbg_detached_cb: keep */ + /* heap->dbg_udata: keep */ heap->dbg_processing = 0; heap->dbg_paused = 0; heap->dbg_state_dirty = 0; @@ -62,16 +64,34 @@ DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) { * XXX: clear breakpoint on either attach or detach? */ heap->dbg_breakpoints_active[0] = (duk_breakpoint *) NULL; +} - /* Call the detached callback last, so that if it immediately - * reattaches, the debugger state will be correct. The udata - * pointer used must be prior to NULLing heap->dbg_udata. - */ - if (detach_cb) { - detach_cb(detach_udata); +DUK_LOCAL void duk__debug_do_detach2(duk_heap *heap) { + duk_debug_detached_function detached_cb; + void *detached_udata; + + /* Safe to call multiple times. */ + + detached_cb = heap->dbg_detached_cb; + detached_udata = heap->dbg_udata; + heap->dbg_detached_cb = NULL; + heap->dbg_udata = NULL; + + if (detached_cb) { + /* Careful here: state must be wiped before the call + * so that we can cleanly handle a re-attach from + * inside the callback. + */ + DUK_D(DUK_DPRINT("detached during message loop, delayed call to detached_cb")); + detached_cb(detached_udata); } } +DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) { + duk__debug_do_detach1(heap); + duk__debug_do_detach2(heap); +} + /* * Debug connection peek and flush primitives */ @@ -1427,7 +1447,7 @@ DUK_LOCAL void duk__debug_handle_detach(duk_hthread *thr, duk_heap *heap) { duk_debug_write_eom(thr); DUK_D(DUK_DPRINT("debug connection detached, mark broken")); - DUK__SET_CONN_BROKEN(thr); /* calls detached callback */ + DUK__SET_CONN_BROKEN(thr); } #if defined(DUK_USE_DEBUGGER_DUMPHEAP) @@ -1725,15 +1745,12 @@ DUK_LOCAL void duk__debug_process_message(duk_hthread *thr) { break; } case DUK_DBG_CMD_DETACH: { - /* Important: when the detach handler returns, - * user code may have re-attached so we must not - * skip to EOM because it would happen in the new - * connection. Instead, return to message loop - * and start parsing messages. + /* The actual detached_cb call is postponed to message loop so + * we don't need any special precautions here (just skip to EOM + * on the already closed connection). */ duk__debug_handle_detach(thr, heap); - DUK_D(DUK_DPRINT("returning to message loop after detach, don't skip to EOM")); - return; + break; } #if defined(DUK_USE_DEBUGGER_DUMPHEAP) case DUK_DBG_CMD_DUMPHEAP: { @@ -1888,6 +1905,17 @@ DUK_INTERNAL duk_bool_t duk_debug_process_messages(duk_hthread *thr, duk_bool_t } duk__debug_process_message(thr); + + if (thr->heap->dbg_read_cb == NULL) { + /* Became detached during message handling (perhaps because + * of an error or by an explicit Detach). Call detached + * callback here, between messages, to avoid confusing the + * broken connection and a possible replacement (which may + * be provided by an instant reattach inside the detached + * callback). + */ + duk__debug_do_detach2(thr->heap); + } if (thr->heap->dbg_state_dirty) { /* Executed something that may have affected status, * resend.