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

[Tom Rini]

On Tue, Jan 21, 2020 at 08:33:04AM +0100, Simon Goldschmidt wrote:
> On Mon, Jan 20, 2020 at 11: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>
> >
> > ---
> > 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 | 37 +++++++++----------------------------
> >  1 file changed, 9 insertions(+), 28 deletions(-)
> >
> > diff --git a/cmd/gpt.c b/cmd/gpt.c
> > index 0c4349f4b249..c0e3c5161789 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;
> >
> > @@ -699,11 +684,7 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
> >                            &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;
> > +               goto out;
> >         }
> >
> >         if (!strcmp(subcomm, "swap")) {
> > @@ -768,11 +749,7 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
> >          */
> >         if (ret < 0) {
> >                 del_gpt_info();
> > -               free(partitions_list);
> > -               if (ret == -ENOMEM)
> > -                       set_gpt_cleanup(&str_disk_guid, &new_partitions);
> > -               else
> > -                       goto out;
> > +               goto out;
> >         }
> >
> >         debug("Writing new partition table\n");
> > @@ -797,8 +774,12 @@ static int do_rename_gpt_parts(struct blk_desc *dev_desc, char *subcomm,
> >         print_gpt_info();
> >         del_gpt_info();
> >   out:
> 
> OK, so the freeing below looks good to me now. However, what worries me is that
> when reaching 'out:' via different code flows, del_gpt_info is not always
> called. I think that could lead to memory leaks when calling this code again, as
> get_gpt_info just reinitializes the list head without checking if there's
> actually something allocated in the list.
> 
> Maybe we should move the last call to del_gpt_info below 'out:' so that it is
> always called?

I think that's safe to do, yes.  v3 coming up..

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200121/284686e2/attachment.sig>