diff mbox series

[PATCHv3] cmd/gpt: Address error cases during gpt rename more correctly

Message ID 20200121165338.24874-1-trini@konsulko.com
State Accepted
Commit 5749faa3d6837d6dbaf2119fc3ec49a326690c8f
Headers show
Series [PATCHv3] cmd/gpt: Address error cases during gpt rename more correctly | expand

Commit Message

Tom Rini Jan. 21, 2020, 4:53 p.m. UTC
New analysis by the tool has shown that we have some cases where we
weren't handling the error exit condition correctly.  When we ran into
the ENOMEM case we wouldn't exit the function and thus incorrect things
could happen.  Rework the unwinding such that we don't need a helper
function now and free what we may have allocated.

Fixes: 18030d04d25d ("GPT: fix memory leaks identified by Coverity")
Reported-by: Coverity (CID: 275475, 275476)
Cc: Alison Chaiken <alison at she-devel.com>
Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
Cc: Jordy <jordy at simplyhacker.com>
Signed-off-by: Tom Rini <trini at konsulko.com>
---
Changes in v3:
- Move del_gpt_info() call into the unwind as set_gpt_info() will have
  been called at that point and we need to clear it. (Simon)
Changes in v2:
- Initialize str_disk_guid to NULL to be sure we can test that it has
  been set later on (Simon, Jordy).
---
 cmd/gpt.c | 47 ++++++++++++-----------------------------------
 1 file changed, 12 insertions(+), 35 deletions(-)

Comments

Simon Goldschmidt Jan. 22, 2020, 7:20 a.m. UTC | #1
On Tue, Jan 21, 2020 at 5:53 PM Tom Rini <trini at konsulko.com> wrote:
>
> New analysis by the tool has shown that we have some cases where we
> weren't handling the error exit condition correctly.  When we ran into
> the ENOMEM case we wouldn't exit the function and thus incorrect things
> could happen.  Rework the unwinding such that we don't need a helper
> function now and free what we may have allocated.
>
> Fixes: 18030d04d25d ("GPT: fix memory leaks identified by Coverity")
> Reported-by: Coverity (CID: 275475, 275476)
> Cc: Alison Chaiken <alison at she-devel.com>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> Cc: Jordy <jordy at simplyhacker.com>
> Signed-off-by: Tom Rini <trini at konsulko.com>

Looks good to me.

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>

> ---
> Changes in v3:
> - Move del_gpt_info() call into the unwind as set_gpt_info() will have
>   been called at that point and we need to clear it. (Simon)
> Changes in v2:
> - Initialize str_disk_guid to NULL to be sure we can test that it has
>   been set later on (Simon, Jordy).
> ---
>  cmd/gpt.c | 47 ++++++++++++-----------------------------------
>  1 file changed, 12 insertions(+), 35 deletions(-)
>
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index 0c4349f4b249..964702bad43e 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -633,21 +633,6 @@ 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)
>  {
> @@ -655,7 +640,7 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
>         struct disk_part *curr;
>         disk_partition_t *new_partitions = NULL;
>         char disk_guid[UUID_STR_LEN + 1];
> -       char *partitions_list, *str_disk_guid;
> +       char *partitions_list, *str_disk_guid = NULL;
>         u8 part_count = 0;
>         int partlistlen, ret, numparts = 0, partnum, i = 1, ctr1 = 0, ctr2 = 0;
>
> @@ -697,14 +682,8 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
>         /* 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) {
> -               del_gpt_info();
> -               free(partitions_list);
> -               if (ret == -ENOMEM)
> -                       set_gpt_cleanup(&str_disk_guid, &new_partitions);
> -               else
> -                       goto out;
> -       }
> +       if (ret < 0)
> +               goto out;
>
>         if (!strcmp(subcomm, "swap")) {
>                 if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > PART_NAME_LEN)) {
> @@ -766,14 +745,8 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
>          * 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;
> -       }
> +       if (ret < 0)
> +               goto out;
>
>         debug("Writing new partition table\n");
>         ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
> @@ -795,10 +768,14 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
>         }
>         printf("new partition table with %d partitions is:\n", numparts);
>         print_gpt_info();
> -       del_gpt_info();
>   out:
> -       free(new_partitions);
> -       free(str_disk_guid);
> +       del_gpt_info();
> +#ifdef CONFIG_RANDOM_UUID
> +       if (str_disk_guid)
> +               free(str_disk_guid);
> +#endif
> +       if (new_partitions)
> +               free(new_partitions);
>         free(partitions_list);
>         return ret;
>  }
> --
> 2.17.1
>
Jordy Jan. 22, 2020, 12:39 p.m. UTC | #2
Just caught up with all the patches and the final one indeed looks good to me too!

Kind Regards,

Jordy
> On January 22, 2020 2:20 AM Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> wrote:
> 
>  
> On Tue, Jan 21, 2020 at 5:53 PM Tom Rini <trini at konsulko.com> wrote:
> >
> > New analysis by the tool has shown that we have some cases where we
> > weren't handling the error exit condition correctly.  When we ran into
> > the ENOMEM case we wouldn't exit the function and thus incorrect things
> > could happen.  Rework the unwinding such that we don't need a helper
> > function now and free what we may have allocated.
> >
> > Fixes: 18030d04d25d ("GPT: fix memory leaks identified by Coverity")
> > Reported-by: Coverity (CID: 275475, 275476)
> > Cc: Alison Chaiken <alison at she-devel.com>
> > Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> > Cc: Jordy <jordy at simplyhacker.com>
> > Signed-off-by: Tom Rini <trini at konsulko.com>
> 
> Looks good to me.
> 
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> 
> > ---
> > Changes in v3:
> > - Move del_gpt_info() call into the unwind as set_gpt_info() will have
> >   been called at that point and we need to clear it. (Simon)
> > Changes in v2:
> > - Initialize str_disk_guid to NULL to be sure we can test that it has
> >   been set later on (Simon, Jordy).
> > ---
> >  cmd/gpt.c | 47 ++++++++++++-----------------------------------
> >  1 file changed, 12 insertions(+), 35 deletions(-)
> >
> > diff --git a/cmd/gpt.c b/cmd/gpt.c
> > index 0c4349f4b249..964702bad43e 100644
> > --- a/cmd/gpt.c
> > +++ b/cmd/gpt.c
> > @@ -633,21 +633,6 @@ 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)
> >  {
> > @@ -655,7 +640,7 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
> >         struct disk_part *curr;
> >         disk_partition_t *new_partitions = NULL;
> >         char disk_guid[UUID_STR_LEN + 1];
> > -       char *partitions_list, *str_disk_guid;
> > +       char *partitions_list, *str_disk_guid = NULL;
> >         u8 part_count = 0;
> >         int partlistlen, ret, numparts = 0, partnum, i = 1, ctr1 = 0, ctr2 = 0;
> >
> > @@ -697,14 +682,8 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
> >         /* 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) {
> > -               del_gpt_info();
> > -               free(partitions_list);
> > -               if (ret == -ENOMEM)
> > -                       set_gpt_cleanup(&str_disk_guid, &new_partitions);
> > -               else
> > -                       goto out;
> > -       }
> > +       if (ret < 0)
> > +               goto out;
> >
> >         if (!strcmp(subcomm, "swap")) {
> >                 if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > PART_NAME_LEN)) {
> > @@ -766,14 +745,8 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
> >          * 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;
> > -       }
> > +       if (ret < 0)
> > +               goto out;
> >
> >         debug("Writing new partition table\n");
> >         ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
> > @@ -795,10 +768,14 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
> >         }
> >         printf("new partition table with %d partitions is:\n", numparts);
> >         print_gpt_info();
> > -       del_gpt_info();
> >   out:
> > -       free(new_partitions);
> > -       free(str_disk_guid);
> > +       del_gpt_info();
> > +#ifdef CONFIG_RANDOM_UUID
> > +       if (str_disk_guid)
> > +               free(str_disk_guid);
> > +#endif
> > +       if (new_partitions)
> > +               free(new_partitions);
> >         free(partitions_list);
> >         return ret;
> >  }
> > --
> > 2.17.1
> >
Tom Rini Jan. 31, 2020, 1:40 a.m. UTC | #3
On Tue, Jan 21, 2020 at 11:53:38AM -0500, Tom Rini wrote:

> New analysis by the tool has shown that we have some cases where we
> weren't handling the error exit condition correctly.  When we ran into
> the ENOMEM case we wouldn't exit the function and thus incorrect things
> could happen.  Rework the unwinding such that we don't need a helper
> function now and free what we may have allocated.
> 
> Fixes: 18030d04d25d ("GPT: fix memory leaks identified by Coverity")
> Reported-by: Coverity (CID: 275475, 275476)
> Cc: Alison Chaiken <alison at she-devel.com>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> Cc: Jordy <jordy at simplyhacker.com>
> Signed-off-by: Tom Rini <trini at konsulko.com>
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/cmd/gpt.c b/cmd/gpt.c
index 0c4349f4b249..964702bad43e 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -633,21 +633,6 @@  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)
 {
@@ -655,7 +640,7 @@  static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	struct disk_part *curr;
 	disk_partition_t *new_partitions = NULL;
 	char disk_guid[UUID_STR_LEN + 1];
-	char *partitions_list, *str_disk_guid;
+	char *partitions_list, *str_disk_guid = NULL;
 	u8 part_count = 0;
 	int partlistlen, ret, numparts = 0, partnum, i = 1, ctr1 = 0, ctr2 = 0;
 
@@ -697,14 +682,8 @@  static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	/* 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) {
-		del_gpt_info();
-		free(partitions_list);
-		if (ret == -ENOMEM)
-			set_gpt_cleanup(&str_disk_guid, &new_partitions);
-		else
-			goto out;
-	}
+	if (ret < 0)
+		goto out;
 
 	if (!strcmp(subcomm, "swap")) {
 		if ((strlen(name1) > PART_NAME_LEN) || (strlen(name2) > PART_NAME_LEN)) {
@@ -766,14 +745,8 @@  static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	 * 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;
-	}
+	if (ret < 0)
+		goto out;
 
 	debug("Writing new partition table\n");
 	ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
@@ -795,10 +768,14 @@  static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
 	}
 	printf("new partition table with %d partitions is:\n", numparts);
 	print_gpt_info();
-	del_gpt_info();
  out:
-	free(new_partitions);
-	free(str_disk_guid);
+	del_gpt_info();
+#ifdef CONFIG_RANDOM_UUID
+	if (str_disk_guid)
+		free(str_disk_guid);
+#endif
+	if (new_partitions)
+		free(new_partitions);
 	free(partitions_list);
 	return ret;
 }