On Tue, Jan 21, 2020 at 5:53 PM Tom Rini <tr...@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 <ali...@she-devel.com> > Cc: Simon Goldschmidt <simon.k.r.goldschm...@gmail.com> > Cc: Jordy <jo...@simplyhacker.com> > Signed-off-by: Tom Rini <tr...@konsulko.com>
Looks good to me. Reviewed-by: Simon Goldschmidt <simon.k.r.goldschm...@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 >