From: Antonio Borneo <borneo.antonio@gmail.com>
Date: Sat, 19 Feb 2022 15:16:31 +0000 (+0100)
Subject: gdb_server: fix double free
X-Git-Tag: v0.12.0-rc1~318
X-Git-Url: https://review.openocd.org/gitweb?p=openocd.git;a=commitdiff_plain;h=bc50b8f1b299991851d4ff3eb30f583ed6aa530b;ds=sidebyside

gdb_server: fix double free

Commit 6541233aa78d ("Combine register lists of smp targets.")
unconditionally assigns the output pointers of the function
smp_reg_list_noread(), even if the function fails and returns
error.
This causes a double free from the caller, that has assigned NULL
to the pointers to simplify the error handling.

Use local variables in smp_reg_list_noread() and assign the output
pointers only on success.

Change-Id: Ic0fd2f26520566cf322f0190780e15637c01cfae
Fixes: 6541233aa78d ("Combine register lists of smp targets.")
Reported-by: Michele Bisogno <michele.bisogno.ct@renesas.com>
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Reviewed-on: https://review.openocd.org/c/openocd/+/6852
Tested-by: jenkins
Reviewed-by: Michele Bisogno <michele.bisogno.ct@renesas.com>
Reviewed-by: Tim Newsome <tim@sifive.com>
---

diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c
index 95720e5616..f8a1aac83a 100644
--- a/src/server/gdb_server.c
+++ b/src/server/gdb_server.c
@@ -2272,12 +2272,12 @@ static int smp_reg_list_noread(struct target *target,
 				combined_list_size, REG_CLASS_ALL);
 
 	unsigned int combined_allocated = 256;
-	*combined_list = malloc(combined_allocated * sizeof(struct reg *));
-	if (*combined_list == NULL) {
+	struct reg **local_list = malloc(combined_allocated * sizeof(struct reg *));
+	if (!local_list) {
 		LOG_ERROR("malloc(%zu) failed", combined_allocated * sizeof(struct reg *));
 		return ERROR_FAIL;
 	}
-	*combined_list_size = 0;
+	unsigned int local_list_size = 0;
 
 	struct target_list *head;
 	foreach_smp_target(head, target->smp_targets) {
@@ -2286,7 +2286,7 @@ static int smp_reg_list_noread(struct target *target,
 		int result = target_get_gdb_reg_list_noread(head->target, &reg_list,
 				&reg_list_size, reg_class);
 		if (result != ERROR_OK) {
-			free(*combined_list);
+			free(local_list);
 			return result;
 		}
 		for (int i = 0; i < reg_list_size; i++) {
@@ -2296,8 +2296,8 @@ static int smp_reg_list_noread(struct target *target,
 				/* Nested loop makes this O(n^2), but this entire function with
 				 * 5 RISC-V targets takes just 2ms on my computer. Fast enough
 				 * for me. */
-				for (int j = 0; j < *combined_list_size; j++) {
-					struct reg *b = (*combined_list)[j];
+				for (unsigned int j = 0; j < local_list_size; j++) {
+					struct reg *b = local_list[j];
 					if (!strcmp(a->name, b->name)) {
 						found = true;
 						if (a->size != b->size) {
@@ -2305,7 +2305,7 @@ static int smp_reg_list_noread(struct target *target,
 									"target, but %d bits on another target.",
 									a->name, a->size, b->size);
 							free(reg_list);
-							free(*combined_list);
+							free(local_list);
 							return ERROR_FAIL;
 						}
 						break;
@@ -2313,16 +2313,16 @@ static int smp_reg_list_noread(struct target *target,
 				}
 				if (!found) {
 					LOG_DEBUG("[%s] %s not found in combined list", target_name(target), a->name);
-					if (*combined_list_size >= (int) combined_allocated) {
+					if (local_list_size >= combined_allocated) {
 						combined_allocated *= 2;
-						*combined_list = realloc(*combined_list, combined_allocated * sizeof(struct reg *));
-						if (*combined_list == NULL) {
+						local_list = realloc(local_list, combined_allocated * sizeof(struct reg *));
+						if (!local_list) {
 							LOG_ERROR("realloc(%zu) failed", combined_allocated * sizeof(struct reg *));
 							return ERROR_FAIL;
 						}
 					}
-					(*combined_list)[*combined_list_size] = a;
-					(*combined_list_size)++;
+					local_list[local_list_size] = a;
+					local_list_size++;
 				}
 			}
 		}
@@ -2336,12 +2336,12 @@ static int smp_reg_list_noread(struct target *target,
 		int result = target_get_gdb_reg_list_noread(head->target, &reg_list,
 				&reg_list_size, reg_class);
 		if (result != ERROR_OK) {
-			free(*combined_list);
+			free(local_list);
 			return result;
 		}
-		for (int i = 0; i < *combined_list_size; i++) {
+		for (unsigned int i = 0; i < local_list_size; i++) {
 			bool found = false;
-			struct reg *a = (*combined_list)[i];
+			struct reg *a = local_list[i];
 			for (int j = 0; j < reg_list_size; j++) {
 				struct reg *b = reg_list[j];
 				if (b->exist && !strcmp(a->name, b->name)) {
@@ -2358,6 +2358,8 @@ static int smp_reg_list_noread(struct target *target,
 		free(reg_list);
 	}
 
+	*combined_list = local_list;
+	*combined_list_size = local_list_size;
 	return ERROR_OK;
 }