[U-Boot] [PATCH] cmd/gpt: Address error cases during gpt rename more correctly

[Tom Rini]

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 at she-devel.com>
> > Signed-off-by: Tom Rini <trini at 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!

-- 
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/20200120/a7599901/attachment.sig>