[U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak

[U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Free the memory allocated to store the test FDT upon test completion to
avoid leaking the memory. We don't bother cleaning up on test failure
since the code is broken in that case and should be fixed, in which case
the leak would also go away.

Reported-by: Tom Rini <tom.rini@gmail.com>
Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 lib/fdtdec_test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/fdtdec_test.c b/lib/fdtdec_test.c
index f6defe16c5a6..54efcc3d46ac 100644
--- a/lib/fdtdec_test.c
+++ b/lib/fdtdec_test.c
@@ -138,6 +138,7 @@ static int run_test(const char *aliases, const char *nodes, const char *expect)
 	}
 
 	printf("pass\n");
+	free(blob);
 	return 0;
 }
 
@@ -288,6 +289,7 @@ static int check_carveout(void)
 	CHECKVAL(make_fdt_carveout(fdt, FDT_SIZE, 2, 2), 0);
 	CHECKOK(check_fdt_carveout(fdt, 2, 2));
 
+	free(fdt);
 	return 0;
 }
 
-- 
2.21.0

[U-Boot] [PATCH 2/2] fdtdec: Remove fdt_{addr,size}_unpack()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

U-Boot already defines the {upper,lower}_32_bits() macros that have the
same purpose. Use the existing macros instead of defining new APIs.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/fdtdec.h  | 24 ------------------------
 lib/fdtdec.c      |  8 ++++++--
 lib/fdtdec_test.c |  8 ++++++--
 3 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 110aa6ab6dea..fa8e34f6f960 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -24,30 +24,6 @@
 typedef phys_addr_t fdt_addr_t;
 typedef phys_size_t fdt_size_t;
 
-static inline fdt32_t fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper)
-{
-	if (upper)
-#ifdef CONFIG_PHYS_64BIT
-		*upper = addr >> 32;
-#else
-		*upper = 0;
-#endif
-
-	return addr;
-}
-
-static inline fdt32_t fdt_size_unpack(fdt_size_t size, fdt32_t *upper)
-{
-	if (upper)
-#ifdef CONFIG_PHYS_64BIT
-		*upper = size >> 32;
-#else
-		*upper = 0;
-#endif
-
-	return size;
-}
-
 #ifdef CONFIG_PHYS_64BIT
 #define FDT_ADDR_T_NONE (-1U)
 #define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index fea44a9a8c65..d0ba88897335 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1300,6 +1300,7 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 	fdt32_t cells[4] = {}, *ptr = cells;
 	uint32_t upper, lower, phandle;
 	int parent, node, na, ns, err;
+	fdt_size_t size;
 	char name[64];
 
 	/* create an empty /reserved-memory node if one doesn't exist */
@@ -1340,7 +1341,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 	 * Unpack the start address and generate the name of the new node
 	 * base on the basename and the unit-address.
 	 */
-	lower = fdt_addr_unpack(carveout->start, &upper);
+	upper = upper_32_bits(carveout->start);
+	lower = lower_32_bits(carveout->start);
 
 	if (na > 1 && upper > 0)
 		snprintf(name, sizeof(name), "%s@%x,%x", basename, upper,
@@ -1374,7 +1376,9 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 	*ptr++ = cpu_to_fdt32(lower);
 
 	/* store one or two size cells */
-	lower = fdt_size_unpack(carveout->end - carveout->start + 1, &upper);
+	size = carveout->end - carveout->start + 1;
+	upper = upper_32_bits(size);
+	lower = lower_32_bits(size);
 
 	if (ns > 1)
 		*ptr++ = cpu_to_fdt32(upper);
diff --git a/lib/fdtdec_test.c b/lib/fdtdec_test.c
index 54efcc3d46ac..e8bfd1fb1ec3 100644
--- a/lib/fdtdec_test.c
+++ b/lib/fdtdec_test.c
@@ -156,11 +156,13 @@ static int make_fdt_carveout_device(void *fdt, uint32_t na, uint32_t ns)
 	};
 	fdt32_t cells[4], *ptr = cells;
 	uint32_t upper, lower;
+	fdt_size_t size;
 	char name[32];
 	int offset;
 
 	/* store one or two address cells */
-	lower = fdt_addr_unpack(carveout.start, &upper);
+	upper = upper_32_bits(carveout.start);
+	lower = lower_32_bits(carveout.start);
 
 	if (na > 1 && upper > 0)
 		snprintf(name, sizeof(name), "%s@%x,%x", basename, upper,
@@ -174,7 +176,9 @@ static int make_fdt_carveout_device(void *fdt, uint32_t na, uint32_t ns)
 	*ptr++ = cpu_to_fdt32(lower);
 
 	/* store one or two size cells */
-	lower = fdt_size_unpack(carveout.end - carveout.start + 1, &upper);
+	size = carveout.end - carveout.start + 1;
+	upper = upper_32_bits(size);
+	lower = lower_32_bits(size);
 
 	if (ns > 1)
 		*ptr++ = cpu_to_fdt32(upper);
-- 
2.21.0

[U-Boot] [PATCH 2/2] fdtdec: Remove fdt_{addr,size}_unpack()

[Simon Glass]

On Mon, 20 May 2019 at 10:05, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> U-Boot already defines the {upper,lower}_32_bits() macros that have the
> same purpose. Use the existing macros instead of defining new APIs.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  include/fdtdec.h  | 24 ------------------------
>  lib/fdtdec.c      |  8 ++++++--
>  lib/fdtdec_test.c |  8 ++++++--
>  3 files changed, 12 insertions(+), 28 deletions(-)

Applied to u-boot-dm, thanks!

[U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak

[Simon Glass]

Hi Thierry,

On Mon, 20 May 2019 at 10:05, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> Free the memory allocated to store the test FDT upon test completion to
> avoid leaking the memory. We don't bother cleaning up on test failure
> since the code is broken in that case and should be fixed, in which case
> the leak would also go away.
>
> Reported-by: Tom Rini <tom.rini@gmail.com>
> Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  lib/fdtdec_test.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/fdtdec_test.c b/lib/fdtdec_test.c
> index f6defe16c5a6..54efcc3d46ac 100644
> --- a/lib/fdtdec_test.c
> +++ b/lib/fdtdec_test.c
> @@ -138,6 +138,7 @@ static int run_test(const char *aliases, const char *nodes, const char *expect)
>         }
>
>         printf("pass\n");
> +       free(blob);

Strictly speaking, CHECKVAL() can cause a function return in the case
of an error.

So a better solution might be to put the code after the malloc() into
a separate function.


>         return 0;
>  }
>
> @@ -288,6 +289,7 @@ static int check_carveout(void)
>         CHECKVAL(make_fdt_carveout(fdt, FDT_SIZE, 2, 2), 0);
>         CHECKOK(check_fdt_carveout(fdt, 2, 2));
>
> +       free(fdt);
>         return 0;
>  }
>
> --
> 2.21.0
>

Regards,
Simon

[U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak

[Thierry Reding]

On Mon, May 20, 2019 at 02:51:08PM -0600, Simon Glass wrote:
> Hi Thierry,
> 
> On Mon, 20 May 2019 at 10:05, Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Free the memory allocated to store the test FDT upon test completion to
> > avoid leaking the memory. We don't bother cleaning up on test failure
> > since the code is broken in that case and should be fixed, in which case
> > the leak would also go away.
> >
> > Reported-by: Tom Rini <tom.rini@gmail.com>
> > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  lib/fdtdec_test.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/fdtdec_test.c b/lib/fdtdec_test.c
> > index f6defe16c5a6..54efcc3d46ac 100644
> > --- a/lib/fdtdec_test.c
> > +++ b/lib/fdtdec_test.c
> > @@ -138,6 +138,7 @@ static int run_test(const char *aliases, const char *nodes, const char *expect)
> >         }
> >
> >         printf("pass\n");
> > +       free(blob);
> 
> Strictly speaking, CHECKVAL() can cause a function return in the case
> of an error.
> 
> So a better solution might be to put the code after the malloc() into
> a separate function.

When Heinrich suggested this fix he brought up the same issue, but
concluded, and I agree with him, that it wasn't worth addressing the
CHECKVAL case because if CHECKVAL failed, our code was buggy and would
need fixing, at which point the leak would go away along with the bug.

Do you feel strongly about reworking this so it doesn't leak in the
error case either?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190521/5fa2473b/attachment.sig>

[U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak

[Simon Glass]

Hi Thierry,

On Tue, 21 May 2019 at 04:21, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, May 20, 2019 at 02:51:08PM -0600, Simon Glass wrote:
> > Hi Thierry,
> >
> > On Mon, 20 May 2019 at 10:05, Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Free the memory allocated to store the test FDT upon test completion to
> > > avoid leaking the memory. We don't bother cleaning up on test failure
> > > since the code is broken in that case and should be fixed, in which case
> > > the leak would also go away.
> > >
> > > Reported-by: Tom Rini <tom.rini@gmail.com>
> > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  lib/fdtdec_test.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/lib/fdtdec_test.c b/lib/fdtdec_test.c
> > > index f6defe16c5a6..54efcc3d46ac 100644
> > > --- a/lib/fdtdec_test.c
> > > +++ b/lib/fdtdec_test.c
> > > @@ -138,6 +138,7 @@ static int run_test(const char *aliases, const char *nodes, const char *expect)
> > >         }
> > >
> > >         printf("pass\n");
> > > +       free(blob);
> >
> > Strictly speaking, CHECKVAL() can cause a function return in the case
> > of an error.
> >
> > So a better solution might be to put the code after the malloc() into
> > a separate function.
>
> When Heinrich suggested this fix he brought up the same issue, but
> concluded, and I agree with him, that it wasn't worth addressing the
> CHECKVAL case because if CHECKVAL failed, our code was buggy and would
> need fixing, at which point the leak would go away along with the bug.
>
> Do you feel strongly about reworking this so it doesn't leak in the
> error case either?

No. Oddly enough I haven't seen a Coverity report on this.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

[U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak

[Simon Glass]

On Tue, 21 May 2019 at 10:43, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Thierry,
>
> On Tue, 21 May 2019 at 04:21, Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Mon, May 20, 2019 at 02:51:08PM -0600, Simon Glass wrote:
> > > Hi Thierry,
> > >
> > > On Mon, 20 May 2019 at 10:05, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Free the memory allocated to store the test FDT upon test completion to
> > > > avoid leaking the memory. We don't bother cleaning up on test failure
> > > > since the code is broken in that case and should be fixed, in which case
> > > > the leak would also go away.
> > > >
> > > > Reported-by: Tom Rini <tom.rini@gmail.com>
> > > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > >  lib/fdtdec_test.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/lib/fdtdec_test.c b/lib/fdtdec_test.c
> > > > index f6defe16c5a6..54efcc3d46ac 100644
> > > > --- a/lib/fdtdec_test.c
> > > > +++ b/lib/fdtdec_test.c
> > > > @@ -138,6 +138,7 @@ static int run_test(const char *aliases, const char *nodes, const char *expect)
> > > >         }
> > > >
> > > >         printf("pass\n");
> > > > +       free(blob);
> > >
> > > Strictly speaking, CHECKVAL() can cause a function return in the case
> > > of an error.
> > >
> > > So a better solution might be to put the code after the malloc() into
> > > a separate function.
> >
> > When Heinrich suggested this fix he brought up the same issue, but
> > concluded, and I agree with him, that it wasn't worth addressing the
> > CHECKVAL case because if CHECKVAL failed, our code was buggy and would
> > need fixing, at which point the leak would go away along with the bug.
> >
> > Do you feel strongly about reworking this so it doesn't leak in the
> > error case either?
>
> No. Oddly enough I haven't seen a Coverity report on this.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm/next, thanks!