Merge pull request #610 from svaarala/debugger-getbytecode-optional-arg

Add option callstack level or object argument to GetBytecode

RELEASES.rst

@@ -1391,6 +1391,9 @@ Planned
 
 * Add sizeof void pointer to the BasicInfo debugger command (GH-611)
 
+* Extend debugger GetBytecode command to accept an optional callstack level
+  or direct heap object argument (GH-610)
+
 * A DukLuv-based JSON debug proxy is now included in the dist package;
   it should allow much easier and more flexible packaging of a JSON debug
   proxy into a debug client (GH-590)
@@ -1425,6 +1428,10 @@ Planned
   the debug transport write callback after it had already returned an error
   (GH-599)
 
+* Fix debugger PutVar command bug where a failure to read the PutVar variable
+  value (e.g. due to a transport detach) could lead to memory unsafe behavior
+  (GH-610)
+
 * Portability improvement for Atari Mint: avoid fmin/fmax (GH-556)
 
 * Change OS string (visible in Duktape.env) from "ios" to "osx" for non-phone

doc/debugger.rst

@@ -770,6 +770,8 @@ types in the text)::
     <buf: field name>      e.g. <buf: buffer data>
     <ptr: field name>      e.g. <ptr: prototype pointer>
     <tval: field name>     e.g. <tval: eval result>
+    <obj: field name>      e.g. <obj: target>
+    <heapptr: field name>  e.g. <heapptr: target>
 
 When a field does not relate to an Ecmascript value exactly, e.g. the field
 is a debugger control field, typing can be loose.  For example, a boolean
@@ -1847,19 +1849,43 @@ GetBytecode request (0x21)
 
 Format::
 
-    REQ <int: 0x21> EOM
+    REQ <int: 0x21> [<int: level> OR <obj: target> OR <heapptr: target>] EOM
     REP <int: numconsts> (<tval: const>){numconsts}
         <int: numfuncs> (<tval: func>){numfuncs}
         <str: bytecode> EOM
 
-Example::
+Example without argument, gets bytecode for current function::
 
     REQ 33 EOM
     REP 2 "foo" "bar" 0  "...bytecode..." EOM
 
-Bytecode endianness is target specific so the debug client needs to get
+Callstack level can be given explicitly, for example -3 is the third callstack
+level counting from callstack top::
+
+    REQ 33 -3 EOM
+    REP 2 "foo" "bar" 0  "...bytecode..." EOM
+
+An explicit Ecmascript function object can also be given using an "object" or
+"heapptr" dvalue::
+
+    REQ 33 {"type":"object","class":6,"pointer":"00000000014839e0"} EOM
+    REP 2 "foo" "bar" 0  "...bytecode..." EOM
+
+An error reply is returned if:
+
+* The argument exists but has an invalid type or points to a target value
+  which is not an Ecmascript function.
+
+* Callstack entry doesn't exist or isn't an Ecmascript activation.
+
+Notes:
+
+* Bytecode endianness is target specific so the debug client needs to get
   target endianness and interpret the bytecode based on that.
 
+* Minor change from Duktape 1.4.0: when the callstack entry doesn't exist
+  Duktape 1.5.x and above will return an error rather than an empty result.
+
 .. note:: This command is somewhat incomplete at the moment and may be modified
    once the best way to do this in the debugger UI has been figured out.
 

src/duk_debugger.c

@@ -425,7 +425,7 @@ DUK_LOCAL duk_double_t duk__debug_read_double_raw(duk_hthread *thr) {
 	return du.d;
 }
 
-DUK_INTERNAL void duk_debug_read_tval(duk_hthread *thr) {
+DUK_INTERNAL duk_tval *duk_debug_read_tval(duk_hthread *thr) {
 	duk_context *ctx = (duk_context *) thr;
 	duk_uint8_t x;
 	duk_uint_t t;
@@ -439,16 +439,16 @@ DUK_INTERNAL void duk_debug_read_tval(duk_hthread *thr) {
 		t = (duk_uint_t) (x - 0xc0);
 		t = (t << 8) + duk_debug_read_byte(thr);
 		duk_push_uint(ctx, (duk_uint_t) t);
-		return;
+		goto return_ptr;
 	}
 	if (x >= 0x80) {
 		duk_push_uint(ctx, (duk_uint_t) (x - 0x80));
-		return;
+		goto return_ptr;
 	}
 	if (x >= 0x60) {
 		len = (duk_uint32_t) (x - 0x60);
 		duk__debug_read_hstring_raw(thr, len);
-		return;
+		goto return_ptr;
 	}
 
 	switch (x) {
@@ -491,20 +491,27 @@ DUK_INTERNAL void duk_debug_read_tval(duk_hthread *thr) {
 		duk_push_number(ctx, d);
 		break;
 	}
-	case 0x1b:
-		/* XXX: not needed for now, so not implemented */
-		DUK_D(DUK_DPRINT("reading object values unimplemented"));
-		goto fail;
+	case 0x1b: {
+		duk_heaphdr *h;
+		duk_debug_skip_byte(thr);
+		h = (duk_heaphdr *) duk__debug_read_pointer_raw(thr);
+		duk_push_heapptr(thr, (void *) h);
+		break;
+	}
 	case 0x1c: {
 		void *ptr;
 		ptr = duk__debug_read_pointer_raw(thr);
 		duk_push_pointer(thr, ptr);
 		break;
 	}
-	case 0x1d:
-		/* XXX: not needed for now, so not implemented */
+	case 0x1d: {
+		/* XXX: Not needed for now, so not implemented.  Note that
+		 * function pointers may have different size/layout than
+		 * a void pointer.
+		 */
 		DUK_D(DUK_DPRINT("reading lightfunc values unimplemented"));
 		goto fail;
+	}
 	case 0x1e: {
 		duk_heaphdr *h;
 		h = (duk_heaphdr *) duk__debug_read_pointer_raw(thr);
@@ -516,11 +523,13 @@ DUK_INTERNAL void duk_debug_read_tval(duk_hthread *thr) {
 		goto fail;
 	}
 
-	return;
+ return_ptr:
+	return DUK_GET_TVAL_NEGIDX(thr, -1);
 
  fail:
 	DUK_D(DUK_DPRINT("debug connection error: failed to decode tval"));
 	DUK__SET_CONN_BROKEN(thr, 1);
+	return NULL;
 }
 
 /*
@@ -1217,7 +1226,6 @@ DUK_LOCAL void duk__debug_handle_get_var(duk_hthread *thr, duk_heap *heap) {
 }
 
 DUK_LOCAL void duk__debug_handle_put_var(duk_hthread *thr, duk_heap *heap) {
-	duk_context *ctx = (duk_context *) thr;
 	duk_hstring *str;
 	duk_tval *tv;
 	duk_int32_t level;
@@ -1227,9 +1235,11 @@ DUK_LOCAL void duk__debug_handle_put_var(duk_hthread *thr, duk_heap *heap) {
 
 	str = duk_debug_read_hstring(thr);  /* push to stack */
 	DUK_ASSERT(str != NULL);
-	duk_debug_read_tval(thr);           /* push to stack */
-	tv = duk_get_tval(ctx, -1);
-	DUK_ASSERT(tv != NULL);
+	tv = duk_debug_read_tval(thr);
+	if (tv == NULL) {
+		/* detached */
+		return;
+	}
 	if (duk_debug_peek_byte(thr) != DUK_DBG_MARKER_EOM) {
 		level = duk_debug_read_int(thr);  /* optional callstack level */
 		if (level >= 0 || -level > (duk_int32_t) thr->callstack_top) {
@@ -1469,13 +1479,19 @@ DUK_LOCAL void duk__debug_handle_apprequest(duk_hthread *thr, duk_heap *heap) {
 		 * then call the request callback to process the request.
 		 */
 		while (duk_debug_peek_byte(thr) != DUK_DBG_MARKER_EOM) {
+			duk_tval *tv;
 			if (!duk_check_stack(ctx, 1)) {
 				DUK_D(DUK_DPRINT("failed to allocate space for request dvalue(s)"));
 				goto fail;
 			}
-			duk_debug_read_tval(thr);  /* push to stack */
+			tv = duk_debug_read_tval(thr);  /* push to stack */
+			if (tv == NULL) {
+				/* detached */
+				return;
+			}
 			nvalues++;
 		}
+		DUK_ASSERT(duk_get_top(ctx) == old_top + nvalues);
 
 		/* Request callback should push values for reply to client onto valstack */
 		DUK_D(DUK_DPRINT("calling into AppRequest request_cb with nvalues=%ld, old_top=%ld, top=%ld",
@@ -1690,28 +1706,52 @@ DUK_LOCAL void duk__debug_handle_dump_heap(duk_hthread *thr, duk_heap *heap) {
 
 DUK_LOCAL void duk__debug_handle_get_bytecode(duk_hthread *thr, duk_heap *heap) {
 	duk_activation *act;
-	duk_hcompiledfunction *fun;
+	duk_hcompiledfunction *fun = NULL;
 	duk_size_t i, n;
 	duk_tval *tv;
 	duk_hobject **fn;
+	duk_int32_t level = -1;
+	duk_uint8_t ibyte;
 
 	DUK_UNREF(heap);
 
 	DUK_D(DUK_DPRINT("debug command GetBytecode"));
 
-	duk_debug_write_reply(thr);
-	if (thr->callstack_top == 0) {
-		fun = NULL;
+	ibyte = duk_debug_peek_byte(thr);
+	if (ibyte != DUK_DBG_MARKER_EOM) {
+		tv = duk_debug_read_tval(thr);
+		if (tv == NULL) {
+			/* detached */
+			return;
+		}
+		if (DUK_TVAL_IS_OBJECT(tv)) {
+			/* tentative, checked later */
+			fun = (duk_hcompiledfunction *) DUK_TVAL_GET_OBJECT(tv);
+			DUK_ASSERT(fun != NULL);
+		} else if (DUK_TVAL_IS_NUMBER(tv)) {
+			level = (duk_int32_t) DUK_TVAL_GET_NUMBER(tv);
 		} else {
-		act = thr->callstack + thr->callstack_top - 1;
+			DUK_D(DUK_DPRINT("invalid argument to GetBytecode: %!T", tv));
+			goto fail_args;
+		}
+	}
+
+	if (fun == NULL) {
+		if (level >= 0 || -level > (duk_int32_t) thr->callstack_top) {
+			DUK_D(DUK_DPRINT("invalid callstack level for GetBytecode"));
+			goto fail_level;
+		}
+		act = thr->callstack + thr->callstack_top + level;
 		fun = (duk_hcompiledfunction *) DUK_ACT_GET_FUNC(act);
-		if (!DUK_HOBJECT_IS_COMPILEDFUNCTION((duk_hobject *) fun)) {
-			fun = NULL;
 	}
+
+	if (fun == NULL || !DUK_HOBJECT_IS_COMPILEDFUNCTION((duk_hobject *) fun)) {
+		DUK_D(DUK_DPRINT("invalid argument to GetBytecode: %!O", fun));
+		goto fail_args;
 	}
-	DUK_ASSERT(fun == NULL || DUK_HOBJECT_IS_COMPILEDFUNCTION((duk_hobject *) fun));
+	DUK_ASSERT(fun != NULL && DUK_HOBJECT_IS_COMPILEDFUNCTION((duk_hobject *) fun));
 
-	if (fun != NULL) {
+	duk_debug_write_reply(thr);
 	n = DUK_HCOMPILEDFUNCTION_GET_CONSTS_COUNT(heap, fun);
 	duk_debug_write_int(thr, (duk_int32_t) n);
 	tv = DUK_HCOMPILEDFUNCTION_GET_CONSTS_BASE(heap, fun);
@@ -1719,7 +1759,6 @@ DUK_LOCAL void duk__debug_handle_get_bytecode(duk_hthread *thr, duk_heap *heap)
 		duk_debug_write_tval(thr, tv);
 		tv++;
 	}
-
 	n = DUK_HCOMPILEDFUNCTION_GET_FUNCS_COUNT(heap, fun);
 	duk_debug_write_int(thr, (duk_int32_t) n);
 	fn = DUK_HCOMPILEDFUNCTION_GET_FUNCS_BASE(heap, fun);
@@ -1727,16 +1766,19 @@ DUK_LOCAL void duk__debug_handle_get_bytecode(duk_hthread *thr, duk_heap *heap)
 		duk_debug_write_hobject(thr, *fn);
 		fn++;
 	}
-
 	duk_debug_write_string(thr,
 	                       (const char *) DUK_HCOMPILEDFUNCTION_GET_CODE_BASE(heap, fun),
 	                       (duk_size_t) DUK_HCOMPILEDFUNCTION_GET_CODE_SIZE(heap, fun));
-	} else {
-		duk_debug_write_int(thr, 0);
-		duk_debug_write_int(thr, 0);
-		duk_debug_write_cstring(thr, "");
-	}
 	duk_debug_write_eom(thr);
+	return;
+
+ fail_args:
+	duk_debug_write_error_eom(thr, DUK_DBG_ERR_UNKNOWN, "invalid argument");
+	return;
+
+ fail_level:
+	duk_debug_write_error_eom(thr, DUK_DBG_ERR_NOTFOUND, "invalid callstack level");
+	return;
 }
 
 /* Process one debug message.  Automatically restore value stack top to its

src/duk_debugger.h

@@ -61,7 +61,7 @@ DUK_INTERNAL_DECL duk_hstring *duk_debug_read_hstring(duk_hthread *thr);
 /* XXX: exposed duk_debug_read_pointer */
 /* XXX: exposed duk_debug_read_buffer */
 /* XXX: exposed duk_debug_read_hbuffer */
-DUK_INTERNAL_DECL void duk_debug_read_tval(duk_hthread *thr);
+DUK_INTERNAL_DECL duk_tval *duk_debug_read_tval(duk_hthread *thr);
 
 DUK_INTERNAL_DECL void duk_debug_write_bytes(duk_hthread *thr, const duk_uint8_t *data, duk_size_t length);
 DUK_INTERNAL_DECL void duk_debug_write_byte(duk_hthread *thr, duk_uint8_t x);