[U-Boot] [PATCH] test/overlay: Fix various malloc/free leaks

[U-Boot] [PATCH] test/overlay: Fix various malloc/free leaks

[Tom Rini]

With the overlay tests now being built in sandbox Coverity has found a
number of issues in the tests.  In short, if malloc ever failed we would
leak the previous mallocs, so we need to do the usual goto pattern to
free each in turn.  Finally, we always looked at the free()d location to
see how many tests had failed for the return code.

Reported-by: Coverity (CID: 167224, 167227, 167230, 167236)
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 test/overlay/cmd_ut_overlay.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/test/overlay/cmd_ut_overlay.c b/test/overlay/cmd_ut_overlay.c
index 24891ee82901..c730a11f5188 100644
--- a/test/overlay/cmd_ut_overlay.c
+++ b/test/overlay/cmd_ut_overlay.c
@@ -226,6 +226,7 @@ int do_ut_overlay(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	void *fdt_overlay = &__dtb_test_fdt_overlay_begin;
 	void *fdt_overlay_stacked = &__dtb_test_fdt_overlay_stacked_begin;
 	void *fdt_base_copy, *fdt_overlay_copy, *fdt_overlay_stacked_copy;
+	int ret = -ENOMEM;
 
 	uts = calloc(1, sizeof(*uts));
 	if (!uts)
@@ -236,16 +237,16 @@ int do_ut_overlay(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	fdt_base_copy = malloc(FDT_COPY_SIZE);
 	if (!fdt_base_copy)
-		return -ENOMEM;
+		goto err1;
 	uts->priv = fdt_base_copy;
 
 	fdt_overlay_copy = malloc(FDT_COPY_SIZE);
 	if (!fdt_overlay_copy)
-		return -ENOMEM;
+		goto err2;
 
 	fdt_overlay_stacked_copy = malloc(FDT_COPY_SIZE);
 	if (!fdt_overlay_stacked_copy)
-		return -ENOMEM;
+		goto err3;
 
 	/*
 	 * Resize the FDT to 4k so that we have room to operate on
@@ -293,11 +294,18 @@ int do_ut_overlay(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	printf("Failures: %d\n", uts->fail_count);
+	if (!uts->fail_count)
+		ret = 0;
+	else
+		ret = CMD_RET_FAILURE;
 
 	free(fdt_overlay_stacked_copy);
+err3:
 	free(fdt_overlay_copy);
+err2:
 	free(fdt_base_copy);
+err1:
 	free(uts);
 
-	return uts->fail_count ? CMD_RET_FAILURE : 0;
+	return ret;
 }
-- 
1.9.1

[U-Boot] [PATCH] test/overlay: Fix various malloc/free leaks

[Langer, Thomas]

Hello Tom,

I just read some days ago about the kernel Coding-Style:

<quote>
Choose label names which say what the goto does or why the goto exists.  An
example of a good name could be ``out_free_buffer:`` if the goto frees ``buffer``.
Avoid using GW-BASIC names like ``err1:`` and ``err2:``, as you would have to
renumber them if you ever add or remove exit paths, and they make correctness
difficult to verify anyway.
</quote>

Does is make sense to follow this for U-Boot also and fix the names of the labels below?

> 
>  	free(fdt_overlay_stacked_copy);
> +err3:
>  	free(fdt_overlay_copy);
> +err2:
>  	free(fdt_base_copy);
> +err1:
>  	free(uts);
> 

Best regards,
Thomas

[U-Boot] [PATCH] test/overlay: Fix various malloc/free leaks

[Tom Rini]

On Tue, Sep 26, 2017 at 06:28:40PM +0000, Langer, Thomas wrote:
> Hello Tom,
> 
> I just read some days ago about the kernel Coding-Style:
> 
> <quote>
> Choose label names which say what the goto does or why the goto exists.  An
> example of a good name could be ``out_free_buffer:`` if the goto frees ``buffer``.
> Avoid using GW-BASIC names like ``err1:`` and ``err2:``, as you would have to
> renumber them if you ever add or remove exit paths, and they make correctness
> difficult to verify anyway.
> </quote>
> 
> Does is make sense to follow this for U-Boot also and fix the names of the labels below?
> 
> > 
> >  	free(fdt_overlay_stacked_copy);
> > +err3:
> >  	free(fdt_overlay_copy);
> > +err2:
> >  	free(fdt_base_copy);
> > +err1:
> >  	free(uts);
> > 

We have in U-Boot a number of cases of both, and the majority (from a
quick read on 'git grep goto' is of the less descriptive case.  I can
certainly see how descriptive goto labels make more sense in the case of
large functions and especially complicated short-cuts.  Do you think
'malloc_base_copy_failed', 'malloc_overlay_copy_failed' and
'malloc_stacked_copy_failed' make the code more readable?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170926/552688eb/attachment.sig>

[U-Boot] test/overlay: Fix various malloc/free leaks

[Tom Rini]

On Tue, Sep 26, 2017 at 12:43:12PM -0400, Tom Rini wrote:

> With the overlay tests now being built in sandbox Coverity has found a
> number of issues in the tests.  In short, if malloc ever failed we would
> leak the previous mallocs, so we need to do the usual goto pattern to
> free each in turn.  Finally, we always looked at the free()d location to
> see how many tests had failed for the return code.
> 
> Reported-by: Coverity (CID: 167224, 167227, 167230, 167236)
> Signed-off-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171007/28169618/attachment.sig>