Re: [U-Boot] [PATCH] GPT: fix memory leaks identified by Coverity

[Łukasz Majewski]

On 09/25/2017 02:37 AM, ali...@peloton-tech.com wrote:

From: Alison Chaiken 

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 
---
  cmd/gpt.c | 84 +--
  1 file changed, 66 insertions(+), 18 deletions(-)

diff --git a/cmd/gpt.c b/cmd/gpt.c
index d4406e3120..dfa41947e1 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -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, _disk_guid,
   _partitions, _count);
-   if (ret < 0)
-   return ret;
+   if (ret < 0) {
+   del_gpt_info();
+   free(partitions_list);
+   if (ret == -ENOMEM)
+   set_gpt_cleanup(_disk_guid, _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, _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, _partitions) {
@@ -722,33 +755,48 @@ static int 

[U-Boot] [PATCH] GPT: fix memory leaks identified by Coverity

[alison]

From: Alison Chaiken 

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 
---
 cmd/gpt.c | 84 +--
 1 file changed, 66 insertions(+), 18 deletions(-)

diff --git a/cmd/gpt.c b/cmd/gpt.c
index d4406e3120..dfa41947e1 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -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, _disk_guid,
   _partitions, _count);
-   if (ret < 0)
-   return ret;
+   if (ret < 0) {
+   del_gpt_info();
+   free(partitions_list);
+   if (ret == -ENOMEM)
+   set_gpt_cleanup(_disk_guid, _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, _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, _partitions) {
@@ -722,33 +755,48 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, 
char