| Message ID | 20191108162452.24963-1-trini@konsulko.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Goldschmidt |
| Headers | show |
| Series | [U-Boot] cmd/gpt: Address error cases during gpt rename more correctly | expand |
+ Jordy, who just found a bug here... Am 08.11.2019 um 17:24 schrieb Tom Rini: > 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@she-devel.com> > Signed-off-by: Tom Rini <trini@konsulko.com> > --- > cmd/gpt.c | 35 ++++++++--------------------------- > 1 file changed, 8 insertions(+), 27 deletions(-) > > diff --git a/cmd/gpt.c b/cmd/gpt.c > index 0c4349f4b249..2da8df60dca3 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) > { > @@ -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: > - free(new_partitions); > - free(str_disk_guid); > +#ifdef CONFIG_RANDOM_UUID > + if (str_disk_guid) Looks good overall, but could it be required to initialize str_disk_guid and new_partitions to 0 to make this test here work? Because set_gpt_info does not always seem to set these pointers in the error case, so they could be left uninitialized? Regards, Simon > + free(str_disk_guid); > +#endif > + if (new_partitions) > + free(new_partitions); > free(partitions_list); > return ret; > } >
On Fri, Jan 17, 2020 at 05:41:31PM +0100, Simon Goldschmidt wrote: > + Jordy, who just found a bug here... > > Am 08.11.2019 um 17:24 schrieb Tom Rini: > > 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@she-devel.com> > > Signed-off-by: Tom Rini <trini@konsulko.com> > > --- > > cmd/gpt.c | 35 ++++++++--------------------------- > > 1 file changed, 8 insertions(+), 27 deletions(-) > > > > diff --git a/cmd/gpt.c b/cmd/gpt.c > > index 0c4349f4b249..2da8df60dca3 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) > > { > > @@ -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: > > - free(new_partitions); > > - free(str_disk_guid); > > +#ifdef CONFIG_RANDOM_UUID > > + if (str_disk_guid) > > Looks good overall, but could it be required to initialize str_disk_guid and > new_partitions to 0 to make this test here work? Because set_gpt_info does > not always seem to set these pointers in the error case, so they could be > left uninitialized? OK, so, looking at everything with the patch applied again. We do initialize new_partitions to NULL, so that's set. That leaves checking str_disk_guid. We will set (to a real value or NULL) str_disk_guid in set_gpt_info(). That function will NOT do anything if str_part is NULL. In our case, if str_part would be NULL we'll have errored out of do_rename_gpt_parts() before that call, so we're safe. If str_part can't be duplicated because we ran out of memory then yes, we can't be sure str_disk_guid will have been set. I'll send v2 shortly. Thanks!
diff --git a/cmd/gpt.c b/cmd/gpt.c index 0c4349f4b249..2da8df60dca3 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) { @@ -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: - free(new_partitions); - free(str_disk_guid); +#ifdef CONFIG_RANDOM_UUID + if (str_disk_guid) + free(str_disk_guid); +#endif + if (new_partitions) + free(new_partitions); free(partitions_list); return ret; }
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@she-devel.com> Signed-off-by: Tom Rini <trini@konsulko.com> --- cmd/gpt.c | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-)