Merge branch 'fix-currpc-setjmp-sync'

Fix a thr->ptr_curr_pc reference in the error unwind path which could lead
to a dereference of thr->ptr_curr_pc when it pointed to an already unwound
C stack frame.

doc/execution.rst

@@ -228,14 +228,31 @@ However, the pointer needs to be synced (copied back) when:
 * The current activation changes, i.e. a new function call is made.
 
 * When an error is about to be thrown, to ensure any longjmp handlers
-  will see correct PC values.
+  will see correct PC values in activations.
 
 * When the executor interrupt is entered; in particular, the debugger
-  must see an up-to-date state.
+  must see an up-to-date state in activations.
 
 * When a ``goto restart_execution;`` occurs in bytecode dispatch, which
   happens for multiple opcodes.
 
+Care must be taken *not* to sync when ``thr->ptr_curr_pc`` is no longer
+pointing to the topmost activation and/or when the C stack frame pointed
+to may no longer exist.  The current policy is to:
+
+* Sync PC on function calls, also backup/restore ``thr->ptr_curr_pc`` on
+  calls.
+
+* Sync PC before a longjmp, often a bit earlier to ensure stacktraces
+  come out right.
+
+* Never sync or otherwise access ``thr->ptr_curr_pc`` in the setjmp
+  catcher and unwind code paths.  This is to ensure we never dereference
+  a ``thr->ptr_curr_pc`` no longer related to the topmost activation or
+  pointing to an unwound C stack frame.  (The ``thr->ptr_curr_pc`` is
+  not currently NULLed so it's intentionally dangling and must not be
+  dereferenced incorrectly.)
+
 Syncing the pointer back unnecessarily or multiple times is safe, however.
 
 Function bytecode is behind a stable pointer, so there are no realloc or

src/duk_js_executor.c

@@ -2057,8 +2057,9 @@ DUK_INTERNAL void duk_js_execute_bytecode(duk_hthread *exec_thr) {
 		DUK_ASSERT(entry_thread != NULL);
 		thr = entry_thread->heap->curr_thread;
 
-		DUK_ASSERT(thr->callstack_top > 0);  /* we were executing bytecode */
-		duk_hthread_sync_currpc(thr);
+		/* Don't sync curr_pc when unwinding: with recursive executor
+		 * calls thr->ptr_curr_pc may be dangling.
+		 */
 
 		/* XXX: signalling the need to shrink check (only if unwound) */
 

tests/ecmascript/test-bug-currpc-valgrind-gh294.js

@@ -0,0 +1,27 @@
+/*
+ *  Memory unsafety issue when developing GH-294.
+ */
+
+/*===
+f
+g
+Error: myerror
+===*/
+
+function f() {
+    print('f');
+    function g() {
+        print('g');
+        throw new Error('myerror');
+    }
+    g();
+}
+
+try {
+    // Calling as f() does not trigger the error, but calling through
+    // apply() does.  The internal difference is that apply() goes through
+    // duk_handle_call().
+    f.apply();
+} catch (e) {
+    print(e);
+}