All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak
@ 2019-05-20 16:05 Thierry Reding
  2019-05-20 16:05 ` [U-Boot] [PATCH 2/2] fdtdec: Remove fdt_{addr,size}_unpack() Thierry Reding
  2019-05-20 20:51 ` [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak Simon Glass
  0 siblings, 2 replies; 7+ messages in thread
From: Thierry Reding @ 2019-05-20 16:05 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] fdtdec: Remove fdt_{addr,size}_unpack()
  2019-05-20 16:05 [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak Thierry Reding
@ 2019-05-20 16:05 ` Thierry Reding
  2019-05-22 13:21   ` Simon Glass
  2019-05-20 20:51 ` [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak Simon Glass
  1 sibling, 1 reply; 7+ messages in thread
From: Thierry Reding @ 2019-05-20 16:05 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak
  2019-05-20 16:05 [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak Thierry Reding
  2019-05-20 16:05 ` [U-Boot] [PATCH 2/2] fdtdec: Remove fdt_{addr,size}_unpack() Thierry Reding
@ 2019-05-20 20:51 ` Simon Glass
  2019-05-21 10:21   ` Thierry Reding
  1 sibling, 1 reply; 7+ messages in thread
From: Simon Glass @ 2019-05-20 20:51 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak
  2019-05-20 20:51 ` [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak Simon Glass
@ 2019-05-21 10:21   ` Thierry Reding
  2019-05-21 16:43     ` Simon Glass
  0 siblings, 1 reply; 7+ messages in thread
From: Thierry Reding @ 2019-05-21 10:21 UTC (permalink / raw)
  To: u-boot

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak
  2019-05-21 10:21   ` Thierry Reding
@ 2019-05-21 16:43     ` Simon Glass
  2019-06-28 13:54       ` Simon Glass
  0 siblings, 1 reply; 7+ messages in thread
From: Simon Glass @ 2019-05-21 16:43 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] fdtdec: Remove fdt_{addr,size}_unpack()
  2019-05-20 16:05 ` [U-Boot] [PATCH 2/2] fdtdec: Remove fdt_{addr,size}_unpack() Thierry Reding
@ 2019-05-22 13:21   ` Simon Glass
  0 siblings, 0 replies; 7+ messages in thread
From: Simon Glass @ 2019-05-22 13:21 UTC (permalink / raw)
  To: u-boot

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!

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak
  2019-05-21 16:43     ` Simon Glass
@ 2019-06-28 13:54       ` Simon Glass
  0 siblings, 0 replies; 7+ messages in thread
From: Simon Glass @ 2019-06-28 13:54 UTC (permalink / raw)
  To: u-boot

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!

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-28 13:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-20 16:05 [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak Thierry Reding
2019-05-20 16:05 ` [U-Boot] [PATCH 2/2] fdtdec: Remove fdt_{addr,size}_unpack() Thierry Reding
2019-05-22 13:21   ` Simon Glass
2019-05-20 20:51 ` [U-Boot] [PATCH 1/2] fdtdec: test: Fix memory leak Simon Glass
2019-05-21 10:21   ` Thierry Reding
2019-05-21 16:43     ` Simon Glass
2019-06-28 13:54       ` Simon Glass

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.