GPT: fix memory leaks identified by Coverity

PATCH Superseded alison at peloton-tech.com 4 participants 7 messages Sep 25, 2017 – Oct 7, 2017

lore

Share & tools

lore

alison at peloton-tech.com

author

2017-09-25 00:37 UTC #0 lore raw

PATCH 1/1 GPT: fix memory leaks identified by Coverity

From: Alison Chaiken <[email protected]>

Create a common exit for most of the error handling code in
do_rename_gpt_parts. Delete the list elements in disk_partitions
before calling INIT_LIST_HEAD from get_gpt_info() a second time.

The SIZEOF_MISMATCH error is not addressed, since that problem was already fixed by "GPT: incomplete initialization in allocate_disk_part".

Signed-off-by Alison Chaiken <[email protected]>

---
 cmd/gpt.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 66 insertions(+), 18 deletions(-)

cmd/gpt.c +66 ~~-18~~

index d4406e3120..dfa41947e1 100644
@@ -633,6 +633,21 @@ static int do_disk_guid(struct blk_desc *dev_desc, char * const namestr)
 }
 
 #ifdef CONFIG_CMD_GPT_RENAME
+
+/* There are 3 malloc() calls in set_gpt_info() and there is no info about which
+ * failed.
+ */
+static void set_gpt_cleanup(char **str_disk_guid,
+			    disk_partition_t **partitions)
+{
+#ifdef CONFIG_RANDOM_UUID
+	if (str_disk_guid)
+		free(str_disk_guid);
+#endif
+	if (partitions)
+		free(partitions);
+}
+
 static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 			       char *name1, char *name2)
 {

@@ -651,19 +666,26 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	ret = get_disk_guid(dev_desc, disk_guid);
 	if (ret < 0)
 		return ret;
+	/* Allocates disk_partitions, requiring matching call to del_gpt_info()
+	 * if successful.
+	 */
 	numparts = get_gpt_info(dev_desc);
 	if (numparts <=  0)
 		return numparts ? numparts : -ENODEV;
 
 	partlistlen = calc_parts_list_len(numparts);
 	partitions_list = malloc(partlistlen);
-	if (partitions_list == NULL)
+	if (!partitions_list) {
+		del_gpt_info();
 		return -ENOMEM;
+	}
 	memset(partitions_list, '\0', partlistlen);
 
 	ret = create_gpt_partitions_list(numparts, disk_guid, partitions_list);
-	if (ret < 0)
+	if (ret < 0) {
+		free(partitions_list);
 		return ret;
+	}
 	/*
 	 * Uncomment the following line to print a string that 'gpt write'
 	 * or 'gpt verify' will accept as input.

@@ -671,15 +693,23 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	debug("OLD partitions_list is %s with %u chars\n", partitions_list,
 	      (unsigned)strlen(partitions_list));
 
+	/* set_gpt_info allocates new_partitions and str_disk_guid */
 	ret = set_gpt_info(dev_desc, partitions_list, &str_disk_guid,
 			   &new_partitions, &part_count);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		del_gpt_info();
+		free(partitions_list);
+		if (ret == -ENOMEM)
+			set_gpt_cleanup(&str_disk_guid, &new_partitions);
+		else
+			goto out;
+	}
 
 	if (!strcmp(subcomm, "swap")) {
 		if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > PART_NAME_LEN)) {
 			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		list_for_each(pos, &disk_partitions) {
 			curr = list_entry(pos, struct disk_part, list);

@@ -693,21 +723,24 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 		}
 		if ((ctr1 + ctr2 < 2) || (ctr1 != ctr2)) {
 			printf("Cannot swap partition names except in pairs.\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 	} else { /* rename */
 		if (strlen(name2) > PART_NAME_LEN) {
 			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		partnum = (int)simple_strtol(name1, NULL, 10);
 		if ((partnum < 0) || (partnum > numparts)) {
 			printf("Illegal partition number %s\n", name1);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		ret = part_get_info(dev_desc, partnum, new_partitions);
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		/* U-Boot partition numbering starts@1 */
 		list_for_each(pos, &disk_partitions) {

@@ -722,33 +755,48 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 
 	ret = create_gpt_partitions_list(numparts, disk_guid, partitions_list);
 	if (ret < 0)
-		return ret;
+		goto out;
 	debug("NEW partitions_list is %s with %u chars\n", partitions_list,
 	      (unsigned)strlen(partitions_list));
 
 	ret = set_gpt_info(dev_desc, partitions_list, &str_disk_guid,
 			   &new_partitions, &part_count);
-	if (ret < 0)
-		return ret;
+	/* Even though valid pointers are here passed into set_gpt_info(),
+	 * it mallocs again, and there's no way to tell which failed.
+	 */
+	if (ret < 0) {
+		del_gpt_info();
+		free(partitions_list);
+		if (ret == -ENOMEM)
+			set_gpt_cleanup(&str_disk_guid, &new_partitions);
+		else
+			goto out;
+	}
 
 	debug("Writing new partition table\n");
 	ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
 	if (ret < 0) {
 		printf("Writing new partition table failed\n");
-		return ret;
+		goto out;
 	}
 
 	debug("Reading back new partition table\n");
+	/* Empty the existing disk_partitions list, as otherwise the memory in
+	 * the original list is unreachable.
+	 */
+	del_gpt_info();
 	numparts = get_gpt_info(dev_desc);
-	if (numparts <=  0)
-		return numparts ? numparts : -ENODEV;
+	if (numparts <=  0) {
+		ret = numparts ? numparts : -ENODEV;
+		goto out;
+	}
 	printf("new partition table with %d partitions is:\n", numparts);
 	print_gpt_info();
-
 	del_gpt_info();
-	free(partitions_list);
-	free(str_disk_guid);
+ out:
 	free(new_partitions);
+	free(str_disk_guid);
+	free(partitions_list);
 	return ret;
 }
 #endif

Signature · 1 line

2.14.1

Łukasz Majewski

2017-09-25 08:36 UTC #1 lore raw

PATCH 1/1 GPT: fix memory leaks identified by Coverity

Quoted 132 lines · alison at peloton-tech.com

From: Alison Chaiken <[email protected]>

Create a common exit for most of the error handling code in
do_rename_gpt_parts. Delete the list elements in disk_partitions
before calling INIT_LIST_HEAD from get_gpt_info() a second time.

The SIZEOF_MISMATCH error is not addressed, since that problem was already fixed by "GPT: incomplete initialization in allocate_disk_part".

Signed-off-by Alison Chaiken <[email protected]>

---
  cmd/gpt.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++--------------
  1 file changed, 66 insertions(+), 18 deletions(-)

cmd/gpt.c +43 ~~-10~~

index d4406e3120..dfa41947e1 100644
@@ -633,6 +633,21 @@ static int do_disk_guid(struct blk_desc *dev_desc, char * const namestr)
  }
 
  #ifdef CONFIG_CMD_GPT_RENAME
+
+/* There are 3 malloc() calls in set_gpt_info() and there is no info about which
+ * failed.
+ */
+static void set_gpt_cleanup(char **str_disk_guid,
+			    disk_partition_t **partitions)
+{
+#ifdef CONFIG_RANDOM_UUID
+	if (str_disk_guid)
+		free(str_disk_guid);
+#endif
+	if (partitions)
+		free(partitions);
+}
+
  static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
  			       char *name1, char *name2)
  {

@@ -651,19 +666,26 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
  	ret = get_disk_guid(dev_desc, disk_guid);
  	if (ret < 0)
  		return ret;
+	/* Allocates disk_partitions, requiring matching call to del_gpt_info()
+	 * if successful.
+	 */
  	numparts = get_gpt_info(dev_desc);
  	if (numparts <=  0)
  		return numparts ? numparts : -ENODEV;
 
  	partlistlen = calc_parts_list_len(numparts);
  	partitions_list = malloc(partlistlen);
-	if (partitions_list == NULL)
+	if (!partitions_list) {
+		del_gpt_info();
  		return -ENOMEM;
+	}
  	memset(partitions_list, '\0', partlistlen);
 
  	ret = create_gpt_partitions_list(numparts, disk_guid, partitions_list);
-	if (ret < 0)
+	if (ret < 0) {
+		free(partitions_list);
  		return ret;
+	}
  	/*
  	 * Uncomment the following line to print a string that 'gpt write'
  	 * or 'gpt verify' will accept as input.

@@ -671,15 +693,23 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
  	debug("OLD partitions_list is %s with %u chars\n", partitions_list,
  	      (unsigned)strlen(partitions_list));
 
+	/* set_gpt_info allocates new_partitions and str_disk_guid */
  	ret = set_gpt_info(dev_desc, partitions_list, &str_disk_guid,
  			   &new_partitions, &part_count);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		del_gpt_info();
+		free(partitions_list);
+		if (ret == -ENOMEM)
+			set_gpt_cleanup(&str_disk_guid, &new_partitions);
+		else
+			goto out;
+	}
 
  	if (!strcmp(subcomm, "swap")) {
  		if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > PART_NAME_LEN)) {
  			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
  		}
  		list_for_each(pos, &disk_partitions) {
  			curr = list_entry(pos, struct disk_part, list);

@@ -693,21 +723,24 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
  		}
  		if ((ctr1 + ctr2 < 2) || (ctr1 != ctr2)) {
  			printf("Cannot swap partition names except in pairs.\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
  		}
  	} else { /* rename */
  		if (strlen(name2) > PART_NAME_LEN) {
  			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
  		}
  		partnum = (int)simple_strtol(name1, NULL, 10);
  		if ((partnum < 0) || (partnum > numparts)) {
  			printf("Illegal partition number %s\n", name1);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
  		}
  		ret = part_get_info(dev_desc, partnum, new_partitions);
  		if (ret < 0)
-			return ret;
+			goto out;
 
  		/* U-Boot partition numbering starts at 1 */
  		list_for_each(pos, &disk_partitions) {

@@ -722,33 +755,48 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 
  	ret = create_gpt_partitions_list(numparts, disk_guid, partitions_list);
  	if (ret < 0)
-		return ret;
+		goto out;

cmd/gpt.c static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm, · line 766

Quoted 10 lines

cmd/gpt.c +3 -2

  	debug("NEW partitions_list is %s with %u chars\n", partitions_list,
  	      (unsigned)strlen(partitions_list));
 
  	ret = set_gpt_info(dev_desc, partitions_list, &str_disk_guid,
  			   &new_partitions, &part_count);
-	if (ret < 0)
-		return ret;
+	/* Even though valid pointers are here passed into set_gpt_info(),
+	 * it mallocs again, and there's no way to tell which failed.
+	 */

Just cosmetic: the multi line comments shall be:

	/*
	 *
	 *
	 */

Quoted 33 lines

cmd/gpt.c +17 -4

+	if (ret < 0) {
+		del_gpt_info();
+		free(partitions_list);
+		if (ret == -ENOMEM)
+			set_gpt_cleanup(&str_disk_guid, &new_partitions);
+		else
+			goto out;
+	}
 
  	debug("Writing new partition table\n");
  	ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
  	if (ret < 0) {
  		printf("Writing new partition table failed\n");
-		return ret;
+		goto out;
  	}
 
  	debug("Reading back new partition table\n");
+	/* Empty the existing disk_partitions list, as otherwise the memory in
+	 * the original list is unreachable.
+	 */
+	del_gpt_info();
  	numparts = get_gpt_info(dev_desc);
-	if (numparts <=  0)
-		return numparts ? numparts : -ENODEV;
+	if (numparts <=  0) {
+		ret = numparts ? numparts : -ENODEV;
+		goto out;
+	}
  	printf("new partition table with %d partitions is:\n", numparts);
  	print_gpt_info();
-

cmd/gpt.c static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm, · line 802

Quoted 10 lines

cmd/gpt.c +3 -2

  	del_gpt_info();
-	free(partitions_list);
-	free(str_disk_guid);
+ out:
  	free(new_partitions);
+	free(str_disk_guid);
+	free(partitions_list);
  	return ret;
  }
  #endif

Reviewed-by Lukasz Majewski <[email protected]>

Signature · 9 lines

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

alison at peloton-tech.com

author

2017-09-26 14:39 UTC #2 lore raw

PATCH v2 1/1 GPT: fix memory leaks identified by Coverity

From: Alison Chaiken <[email protected]>

Create a common exit for most of the error handling code in
do_rename_gpt_parts. Delete the list elements in disk_partitions
before calling INIT_LIST_HEAD from get_gpt_info() a second time.

The SIZEOF_MISMATCH error is not addressed, since that problem was already fixed by "GPT: incomplete initialization in allocate_disk_part".

Signed-off-by Alison Chaiken <[email protected]>

v2: Fix comment-formatting problems in v1.

---
 cmd/gpt.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 18 deletions(-)

cmd/gpt.c +69 ~~-18~~

index d4406e3120..9e04affc06 100644
@@ -633,6 +633,21 @@ static int do_disk_guid(struct blk_desc *dev_desc, char * const namestr)
 }
 
 #ifdef CONFIG_CMD_GPT_RENAME
+/*
+ * There are 3 malloc() calls in set_gpt_info() and there is no info about which
+ * failed.
+ */
+static void set_gpt_cleanup(char **str_disk_guid,
+			    disk_partition_t **partitions)
+{
+#ifdef CONFIG_RANDOM_UUID
+	if (str_disk_guid)
+		free(str_disk_guid);
+#endif
+	if (partitions)
+		free(partitions);
+}
+
 static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 			       char *name1, char *name2)
 {

@@ -651,19 +666,27 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	ret = get_disk_guid(dev_desc, disk_guid);
 	if (ret < 0)
 		return ret;
+	/*
+	 * Allocates disk_partitions, requiring matching call to del_gpt_info()
+	 * if successful.
+	 */
 	numparts = get_gpt_info(dev_desc);
 	if (numparts <=  0)
 		return numparts ? numparts : -ENODEV;
 
 	partlistlen = calc_parts_list_len(numparts);
 	partitions_list = malloc(partlistlen);
-	if (partitions_list == NULL)
+	if (!partitions_list) {
+		del_gpt_info();
 		return -ENOMEM;
+	}
 	memset(partitions_list, '\0', partlistlen);
 
 	ret = create_gpt_partitions_list(numparts, disk_guid, partitions_list);
-	if (ret < 0)
+	if (ret < 0) {
+		free(partitions_list);
 		return ret;
+	}
 	/*
 	 * Uncomment the following line to print a string that 'gpt write'
 	 * or 'gpt verify' will accept as input.

@@ -671,15 +694,23 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	debug("OLD partitions_list is %s with %u chars\n", partitions_list,
 	      (unsigned)strlen(partitions_list));
 
+	/* set_gpt_info allocates new_partitions and str_disk_guid */
 	ret = set_gpt_info(dev_desc, partitions_list, &str_disk_guid,
 			   &new_partitions, &part_count);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		del_gpt_info();
+		free(partitions_list);
+		if (ret == -ENOMEM)
+			set_gpt_cleanup(&str_disk_guid, &new_partitions);
+		else
+			goto out;
+	}
 
 	if (!strcmp(subcomm, "swap")) {
 		if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > PART_NAME_LEN)) {
 			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		list_for_each(pos, &disk_partitions) {
 			curr = list_entry(pos, struct disk_part, list);

@@ -693,21 +724,24 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 		}
 		if ((ctr1 + ctr2 < 2) || (ctr1 != ctr2)) {
 			printf("Cannot swap partition names except in pairs.\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 	} else { /* rename */
 		if (strlen(name2) > PART_NAME_LEN) {
 			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		partnum = (int)simple_strtol(name1, NULL, 10);
 		if ((partnum < 0) || (partnum > numparts)) {
 			printf("Illegal partition number %s\n", name1);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		ret = part_get_info(dev_desc, partnum, new_partitions);
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		/* U-Boot partition numbering starts@1 */
 		list_for_each(pos, &disk_partitions) {

@@ -722,33 +756,50 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 
 	ret = create_gpt_partitions_list(numparts, disk_guid, partitions_list);
 	if (ret < 0)
-		return ret;
+		goto out;
 	debug("NEW partitions_list is %s with %u chars\n", partitions_list,
 	      (unsigned)strlen(partitions_list));
 
 	ret = set_gpt_info(dev_desc, partitions_list, &str_disk_guid,
 			   &new_partitions, &part_count);
-	if (ret < 0)
-		return ret;
+	/*
+	 * Even though valid pointers are here passed into set_gpt_info(),
+	 * it mallocs again, and there's no way to tell which failed.
+	 */
+	if (ret < 0) {
+		del_gpt_info();
+		free(partitions_list);
+		if (ret == -ENOMEM)
+			set_gpt_cleanup(&str_disk_guid, &new_partitions);
+		else
+			goto out;
+	}
 
 	debug("Writing new partition table\n");
 	ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
 	if (ret < 0) {
 		printf("Writing new partition table failed\n");
-		return ret;
+		goto out;
 	}
 
 	debug("Reading back new partition table\n");
+	/*
+	 * Empty the existing disk_partitions list, as otherwise the memory in
+	 * the original list is unreachable.
+	 */
+	del_gpt_info();
 	numparts = get_gpt_info(dev_desc);
-	if (numparts <=  0)
-		return numparts ? numparts : -ENODEV;
+	if (numparts <=  0) {
+		ret = numparts ? numparts : -ENODEV;
+		goto out;
+	}
 	printf("new partition table with %d partitions is:\n", numparts);
 	print_gpt_info();
-
 	del_gpt_info();
-	free(partitions_list);
-	free(str_disk_guid);
+ out:
 	free(new_partitions);
+	free(str_disk_guid);
+	free(partitions_list);
 	return ret;
 }
 #endif

Signature · 1 line

2.14.1

Bin Meng

2017-09-26 14:40 UTC #3 lore raw

PATCH v2 1/1 GPT: fix memory leaks identified by Coverity

Hi,

Quoted 6 lines · On Tue, Sep 26, 2017 at 10:39 PM, <[email protected]> wrote:

From: Alison Chaiken <[email protected]>
Create a common exit for most of the error handling code in
do_rename_gpt_parts. Delete the list elements in disk_partitions
before calling INIT_LIST_HEAD from get_gpt_info() a second time.

Quoted 7 lines

The SIZEOF_MISMATCH error is not addressed, since that problem was already fixed by "GPT: incomplete initialization in allocate_disk_part".
Signed-off-by Alison Chaiken <[email protected]>
v2: Fix comment-formatting problems in v1.

Please move the change log below ---

Quoted 2 lines

---

Regards,
Bin

alison at peloton-tech.com

author

2017-09-26 14:42 UTC #4 lore raw

PATCH v2 1/1 GPT: fix memory leaks identified by Coverity

In reply to Łukasz Majewski

From: Alison Chaiken <[email protected]>

Create a common exit for most of the error handling code in
do_rename_gpt_parts. Delete the list elements in disk_partitions
before calling INIT_LIST_HEAD from get_gpt_info() a second time.

The SIZEOF_MISMATCH error is not addressed, since that problem was already fixed by "GPT: incomplete initialization in allocate_disk_part".

Signed-off-by Alison Chaiken <[email protected]>

---

v2: Fix comment-formatting problems in v1.

 cmd/gpt.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 18 deletions(-)

cmd/gpt.c +69 ~~-18~~

index d4406e3120..9e04affc06 100644
@@ -633,6 +633,21 @@ static int do_disk_guid(struct blk_desc *dev_desc, char * const namestr)
 }
 
 #ifdef CONFIG_CMD_GPT_RENAME
+/*
+ * There are 3 malloc() calls in set_gpt_info() and there is no info about which
+ * failed.
+ */
+static void set_gpt_cleanup(char **str_disk_guid,
+			    disk_partition_t **partitions)
+{
+#ifdef CONFIG_RANDOM_UUID
+	if (str_disk_guid)
+		free(str_disk_guid);
+#endif
+	if (partitions)
+		free(partitions);
+}
+
 static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 			       char *name1, char *name2)
 {

@@ -651,19 +666,27 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	ret = get_disk_guid(dev_desc, disk_guid);
 	if (ret < 0)
 		return ret;
+	/*
+	 * Allocates disk_partitions, requiring matching call to del_gpt_info()
+	 * if successful.
+	 */
 	numparts = get_gpt_info(dev_desc);
 	if (numparts <=  0)
 		return numparts ? numparts : -ENODEV;
 
 	partlistlen = calc_parts_list_len(numparts);
 	partitions_list = malloc(partlistlen);
-	if (partitions_list == NULL)
+	if (!partitions_list) {
+		del_gpt_info();
 		return -ENOMEM;
+	}
 	memset(partitions_list, '\0', partlistlen);
 
 	ret = create_gpt_partitions_list(numparts, disk_guid, partitions_list);
-	if (ret < 0)
+	if (ret < 0) {
+		free(partitions_list);
 		return ret;
+	}
 	/*
 	 * Uncomment the following line to print a string that 'gpt write'
 	 * or 'gpt verify' will accept as input.

@@ -671,15 +694,23 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	debug("OLD partitions_list is %s with %u chars\n", partitions_list,
 	      (unsigned)strlen(partitions_list));
 
+	/* set_gpt_info allocates new_partitions and str_disk_guid */
 	ret = set_gpt_info(dev_desc, partitions_list, &str_disk_guid,
 			   &new_partitions, &part_count);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		del_gpt_info();
+		free(partitions_list);
+		if (ret == -ENOMEM)
+			set_gpt_cleanup(&str_disk_guid, &new_partitions);
+		else
+			goto out;
+	}
 
 	if (!strcmp(subcomm, "swap")) {
 		if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > PART_NAME_LEN)) {
 			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		list_for_each(pos, &disk_partitions) {
 			curr = list_entry(pos, struct disk_part, list);

@@ -693,21 +724,24 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 		}
 		if ((ctr1 + ctr2 < 2) || (ctr1 != ctr2)) {
 			printf("Cannot swap partition names except in pairs.\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 	} else { /* rename */
 		if (strlen(name2) > PART_NAME_LEN) {
 			printf("Names longer than %d characters are truncated.\n", PART_NAME_LEN);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		partnum = (int)simple_strtol(name1, NULL, 10);
 		if ((partnum < 0) || (partnum > numparts)) {
 			printf("Illegal partition number %s\n", name1);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		ret = part_get_info(dev_desc, partnum, new_partitions);
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		/* U-Boot partition numbering starts@1 */
 		list_for_each(pos, &disk_partitions) {

@@ -722,33 +756,50 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 
 	ret = create_gpt_partitions_list(numparts, disk_guid, partitions_list);
 	if (ret < 0)
-		return ret;
+		goto out;
 	debug("NEW partitions_list is %s with %u chars\n", partitions_list,
 	      (unsigned)strlen(partitions_list));
 
 	ret = set_gpt_info(dev_desc, partitions_list, &str_disk_guid,
 			   &new_partitions, &part_count);
-	if (ret < 0)
-		return ret;
+	/*
+	 * Even though valid pointers are here passed into set_gpt_info(),
+	 * it mallocs again, and there's no way to tell which failed.
+	 */
+	if (ret < 0) {
+		del_gpt_info();
+		free(partitions_list);
+		if (ret == -ENOMEM)
+			set_gpt_cleanup(&str_disk_guid, &new_partitions);
+		else
+			goto out;
+	}
 
 	debug("Writing new partition table\n");
 	ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
 	if (ret < 0) {
 		printf("Writing new partition table failed\n");
-		return ret;
+		goto out;
 	}
 
 	debug("Reading back new partition table\n");
+	/*
+	 * Empty the existing disk_partitions list, as otherwise the memory in
+	 * the original list is unreachable.
+	 */
+	del_gpt_info();
 	numparts = get_gpt_info(dev_desc);
-	if (numparts <=  0)
-		return numparts ? numparts : -ENODEV;
+	if (numparts <=  0) {
+		ret = numparts ? numparts : -ENODEV;
+		goto out;
+	}
 	printf("new partition table with %d partitions is:\n", numparts);
 	print_gpt_info();
-
 	del_gpt_info();
-	free(partitions_list);
-	free(str_disk_guid);
+ out:
 	free(new_partitions);
+	free(str_disk_guid);
+	free(partitions_list);
 	return ret;
 }
 #endif

Signature · 1 line

2.14.1

Tom Rini

2017-09-26 14:44 UTC #5 lore raw

PATCH v2 1/1 GPT: fix memory leaks identified by Coverity

Quoted 11 lines · alison at peloton-tech.com

From: Alison Chaiken <[email protected]>
Create a common exit for most of the error handling code in
do_rename_gpt_parts. Delete the list elements in disk_partitions
before calling INIT_LIST_HEAD from get_gpt_info() a second time.
The SIZEOF_MISMATCH error is not addressed, since that problem was already fixed by "GPT: incomplete initialization in allocate_disk_part".
Signed-off-by Alison Chaiken <[email protected]>

Reported-by Coverity (CID: 167222, 167235, 167237)

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170926/07b6d614/attachment.sig>

Tom Rini

2017-10-07 13:08 UTC #6 lore raw

PATCH v2 1/1 GPT: fix memory leaks identified by Coverity

In reply to alison at peloton-tech.com

Quoted 6 lines · alison at peloton-tech.com

From: Alison Chaiken <[email protected]>
Create a common exit for most of the error handling code in
do_rename_gpt_parts. Delete the list elements in disk_partitions
before calling INIT_LIST_HEAD from get_gpt_info() a second time.

Quoted 7 lines

The SIZEOF_MISMATCH error is not addressed, since that problem was already fixed by "GPT: incomplete initialization in allocate_disk_part".
Signed-off-by Alison Chaiken <[email protected]>
Reported-by Coverity (CID: 167222, 167235, 167237)
Reviewed-by Tom Rini <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171007/f5f537c0/attachment.sig>