Some fixes to windbg

* Fix radareorg#10505 * Fix wrong register profile being picked * Fix use-after free(s) and null derefs
GustavoLCR · Jul 27, 2019 · 5948e50 · 5948e50
1 parent fceda4c
commit 5948e50
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 21 deletions.
diff --git a/libr/debug/p/debug_windbg.c b/libr/debug/p/debug_windbg.c
@@ -72,23 +72,28 @@ static RDebugReasonType r_debug_windbg_wait(RDebug *dbg, int pid) {
 		if (dbreak) {
 			dbreak = false;
 			windbg_break (wctx);
-			free (pkt);
+			R_FREE (pkt);
 			continue;
 		}
 		if (ret != KD_E_OK || !pkt) {
 			reason = R_DEBUG_REASON_ERROR;
 			break;
 		}
 		stc = (kd_stc_64 *) pkt->data;
+		dbg->reason.addr = stc->pc;
+		dbg->reason.tid = stc->kthread;
+		dbg->reason.signum = stc->state;
+		windbg_set_cpu (wctx, stc->cpu);
 		if (stc->state == DbgKdExceptionStateChange) {
-			windbg_set_cpu (wctx, stc->cpu);
 			dbg->reason.type = R_DEBUG_REASON_INT;
-			dbg->reason.addr = stc->pc;
-			dbg->reason.tid = stc->kthread;
-			dbg->reason.signum = stc->state;
 			reason = R_DEBUG_REASON_INT;
 			break;
+		} else if (stc->state == DbgKdLoadSymbolsStateChange) {
+			dbg->reason.type = R_DEBUG_REASON_NEW_LIB;
+			reason = R_DEBUG_REASON_NEW_LIB;
+			break;
 		}
+		R_FREE (pkt);
 	}
 	free (pkt);
 	return reason;
@@ -111,14 +116,14 @@ static int r_debug_windbg_attach(RDebug *dbg, int pid) {
 	// Handshake
 	if (!windbg_sync (wctx)) {
 		eprintf ("Could not connect to windbg\n");
-		windbg_ctx_free (wctx);
+		windbg_ctx_free (&desc->data);
 		return false;
 	}
 	if (!windbg_read_ver (wctx)) {
-		windbg_ctx_free (wctx);
+		windbg_ctx_free (&desc->data);
 		return false;
 	}
-
+	dbg->bits = windbg_get_bits (wctx);
 	// Make r_debug_is_dead happy
 	dbg->pid = 0;
 	return true;
@@ -136,6 +141,7 @@ static char *r_debug_windbg_reg_profile(RDebug *dbg) {
 	if (dbg->arch && strcmp (dbg->arch, "x86")) {
 		return NULL;
 	}
+	r_debug_windbg_attach (dbg, 0);
 	if (dbg->bits == R_SYS_BITS_32) {
 #include "native/reg/windows-x86.h"
 	} else if (dbg->bits == R_SYS_BITS_64) {
@@ -150,7 +156,13 @@ static int r_debug_windbg_breakpoint(RBreakpoint *bp, RBreakpointItem *b, bool s
 		return false;
 	}
 	// Use a 32 bit word here to keep this compatible with 32 bit hosts
-	tag = (int *)&b->data;
+	if (!b->data) {
+		b->data = R_NEW0 (int);
+		if (!b->data) {
+			return 0;
+		}
+	}
+	tag = b->data;
 	return windbg_bkpt (wctx, b->addr, set, b->hw, tag);
 }
 

diff --git a/libr/io/p/io_windbg.c b/libr/io/p/io_windbg.c
@@ -93,7 +93,7 @@ static int __read(RIO *io, RIODesc *fd, ut8 *buf, int count) {
 }
 
 static int __close(RIODesc *fd) {
-	windbg_ctx_free (fd->data);
+	windbg_ctx_free (&fd->data);
 	return true;
 }
 

diff --git a/shlr/windbg/windbg.c b/shlr/windbg/windbg.c
@@ -78,6 +78,10 @@ struct _WindCtx {
 	WindProc *target;
 };
 
+int windbg_get_bits(WindCtx *ctx) {
+	return ctx->is_x64 ? R_SYS_BITS_64 : R_SYS_BITS_32;
+}
+
 int windbg_get_cpus(WindCtx *ctx) {
 	if (!ctx) {
 		return -1;
@@ -153,14 +157,14 @@ WindCtx *windbg_ctx_new(void *io_ptr) {
 	return ctx;
 }
 
-void windbg_ctx_free(WindCtx *ctx) {
-	if (!ctx) {
+void windbg_ctx_free(WindCtx **ctx) {
+	if (!ctx || !*ctx) {
 		return;
 	}
-	r_list_free (ctx->plist_cache);
-	r_list_free (ctx->tlist_cache);
-	iob_close (ctx->io_ptr);
-	free (ctx);
+	r_list_free ((*ctx)->plist_cache);
+	r_list_free ((*ctx)->tlist_cache);
+	iob_close ((*ctx)->io_ptr);
+	R_FREE (*ctx);
 }
 
 #define PKT_REQ(p) ((kd_req_t *) (((kd_packet_t *) p)->data))
@@ -212,16 +216,21 @@ int windbg_wait_packet(WindCtx *ctx, const uint32_t type, kd_packet_t **p) {
 	int retries = 10;
 
 	do {
-		if (pkt) free (pkt);
+		if (pkt) {
+			R_FREE (pkt);
+		}
 		// Try to read a whole packet
 		ret = kd_read_packet (ctx->io_ptr, &pkt);
-		if (ret != KD_E_OK) {
+		if (ret != KD_E_OK || !pkt) {
 			break;
 		}
 
 		// eprintf ("Received %08x\n", pkt->type);
 		if (pkt->type != type) {
-			eprintf ("We were not waiting for this... %08x\n", type);
+			eprintf ("We were not waiting for this... %08x\n", pkt->type);
+		}
+		if (pkt->leader == KD_PACKET_DATA) {
+			kd_send_ctrl_packet (ctx->io_ptr, KD_PACKET_TYPE_ACKNOWLEDGE, 0);
 		}
 		if (pkt->leader == KD_PACKET_DATA && pkt->type == KD_PACKET_TYPE_STATE_CHANGE64) {
 			// dump_stc (pkt);
@@ -640,6 +649,10 @@ int windbg_sync(WindCtx *ctx) {
 		return 0;
 	}
 
+	if (ctx->syncd) {
+		return 1;
+	}
+
 	// Send the breakin packet
 	if (iob_write (ctx->io_ptr, (const uint8_t *) "b", 1) != 1) {
 		return 0;
@@ -757,7 +770,7 @@ bool windbg_write_reg(WindCtx *ctx, const uint8_t *buf, int size) {
 
 int windbg_read_reg(WindCtx *ctx, uint8_t *buf, int size) {
 	kd_req_t req;
-	kd_packet_t *pkt;
+	kd_packet_t *pkt = NULL;
 	int ret;
 
 	if (!ctx || !ctx->io_ptr || !ctx->syncd) {

diff --git a/shlr/windbg/windbg.h b/shlr/windbg/windbg.h
@@ -72,6 +72,7 @@ typedef struct {
 } Profile;
 
 // grep -e "^windbg_" shlr/wind/wind.c | sed -e 's/ {$/;/' -e 's/^/int /'
+int windbg_get_bits(WindCtx *ctx);
 ut64 windbg_get_target_base (WindCtx *ctx);
 ut32 windbg_get_target (WindCtx *ctx);
 bool windbg_set_target (WindCtx *ctx, ut32 pid);
@@ -81,7 +82,7 @@ int windbg_get_cpus (WindCtx *ctx);
 bool windbg_set_cpu (WindCtx *ctx, int cpu);
 int windbg_get_cpu (WindCtx *ctx);
 WindCtx * windbg_ctx_new (void *io_ptr);
-void windbg_ctx_free (WindCtx *ctx);
+void windbg_ctx_free (WindCtx **ctx);
 int windbg_wait_packet (WindCtx *ctx, const ut32 type, kd_packet_t **p);
 int windbg_sync (WindCtx *ctx);
 bool windbg_read_ver (WindCtx *ctx);