On Mon, Jan 20, 2020 at 11: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>
>
> ---
> 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?
Regards,
Simon
> - 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;
> }
> --
> 2.17.1
