* [PATCHv2] cmd/gpt: Address error cases during gpt rename more correctly
@ 2020-01-20 22:53 Tom Rini
2020-01-21 7:33 ` Simon Goldschmidt
0 siblings, 1 reply; 3+ messages in thread
From: Tom Rini @ 2020-01-20 22:53 UTC (permalink / raw)
To: u-boot
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>
Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Cc: Jordy <jordy@simplyhacker.com>
Signed-off-by: Tom Rini <trini@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:
- 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCHv2] cmd/gpt: Address error cases during gpt rename more correctly
2020-01-20 22:53 [PATCHv2] cmd/gpt: Address error cases during gpt rename more correctly Tom Rini
@ 2020-01-21 7:33 ` Simon Goldschmidt
2020-01-21 16:41 ` Tom Rini
0 siblings, 1 reply; 3+ messages in thread
From: Simon Goldschmidt @ 2020-01-21 7:33 UTC (permalink / raw)
To: u-boot
On Mon, Jan 20, 2020 at 11:53 PM Tom Rini <trini@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@she-devel.com>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Cc: Jordy <jordy@simplyhacker.com>
> Signed-off-by: Tom Rini <trini@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCHv2] cmd/gpt: Address error cases during gpt rename more correctly
2020-01-21 7:33 ` Simon Goldschmidt
@ 2020-01-21 16:41 ` Tom Rini
0 siblings, 0 replies; 3+ messages in thread
From: Tom Rini @ 2020-01-21 16:41 UTC (permalink / raw)
To: u-boot
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@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@she-devel.com>
> > Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > Cc: Jordy <jordy@simplyhacker.com>
> > Signed-off-by: Tom Rini <trini@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>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-21 16:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-20 22:53 [PATCHv2] cmd/gpt: Address error cases during gpt rename more correctly Tom Rini
2020-01-21 7:33 ` Simon Goldschmidt
2020-01-21 16:41 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.