[U-Boot] [PATCH 1/1] common: cli: avoid memory leak

[U-Boot] [PATCH 1/1] common: cli: avoid memory leak

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

Whether CONFIG_SYS_HUSH_PARSER is defined or not, should always
check to free 'buff' to avoid memory leak.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Simon Glass <sjg@chromium.org>
---
 common/cli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/cli.c b/common/cli.c
index fbcd339..119d282 100644
--- a/common/cli.c
+++ b/common/cli.c
@@ -103,9 +103,9 @@ int run_command_list(const char *cmd, int len, int flag)
 	 * is pretty rare.
 	 */
 	rcode = cli_simple_run_command_list(buff, flag);
+#endif
 	if (need_buff)
 		free(buff);
-#endif
 
 	return rcode;
 }
-- 
2.6.2

[U-Boot] [PATCH 1/1] common: env: initialize scalar variable

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

Before calling hsearch_r, initialize callback entry to NULL.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
---
 common/env_callback.c | 1 +
 common/env_flags.c    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/common/env_callback.c b/common/env_callback.c
index f4d3dbd..1957cc1 100644
--- a/common/env_callback.c
+++ b/common/env_callback.c
@@ -97,6 +97,7 @@ static int set_callback(const char *name, const char *value, void *priv)
 
 	e.key	= name;
 	e.data	= NULL;
+	e.callback = NULL;
 	hsearch_r(e, FIND, &ep, &env_htab, 0);
 
 	/* does the env variable actually exist? */
diff --git a/common/env_flags.c b/common/env_flags.c
index e682d85..7719355 100644
--- a/common/env_flags.c
+++ b/common/env_flags.c
@@ -455,6 +455,7 @@ static int set_flags(const char *name, const char *value, void *priv)
 
 	e.key	= name;
 	e.data	= NULL;
+	e.callback = NULL;
 	hsearch_r(e, FIND, &ep, &env_htab, 0);
 
 	/* does the env variable actually exist? */
-- 
2.6.2

[U-Boot] [PATCH 1/1] common: env: initialize scalar variable

[Wolfgang Denk]

Dear Peng Fan,

In message <1450775655-2979-2-git-send-email-van.freenix@gmail.com> you wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Before calling hsearch_r, initialize callback entry to NULL.

Which exact problem are you fixing here?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Beware of the Turing Tar-pit in  which  everything  is  possible  but
nothing of interest is easy.

[U-Boot] [PATCH 1/1] common: env: initialize scalar variable

[Peng Fan]

Hi Wolfgang
On Tue, Dec 22, 2015 at 10:29:51AM +0100, Wolfgang Denk wrote:
>Dear Peng Fan,
>
>In message <1450775655-2979-2-git-send-email-van.freenix@gmail.com> you wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
>> Before calling hsearch_r, initialize callback entry to NULL.
>
>Which exact problem are you fixing here?

This was reported by Coverity.
"
Uninitialized scalar variable (UNINIT)
uninit_use_in_call: Using uninitialized value e. Field e.callback is uninitialized when calling hsearch_r.
"
I'll add more commit log in V2.

Regards,
Peng.
>
>Best regards,
>
>Wolfgang Denk
>
>-- 
>DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
>Beware of the Turing Tar-pit in  which  everything  is  possible  but
>nothing of interest is easy.

[U-Boot] [PATCH 1/1] common: nvedit: use snprintf instead of sprintf

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

Use snprintf to replace sprintf.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Joe Hershberger <joe.hershberger@ni.com>
---
 common/cmd_nvedit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 2f9cdd0..5ae9d9d 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -595,7 +595,7 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc,
 	/* Set read buffer to initial value or empty sting */
 	init_val = getenv(argv[1]);
 	if (init_val)
-		sprintf(buffer, "%s", init_val);
+		snprintf(buffer, CONFIG_SYS_CBSIZE, "%s", init_val);
 	else
 		buffer[0] = '\0';
 
-- 
2.6.2

[U-Boot] [PATCH 1/1] common: nvedit: use snprintf instead of sprintf

[Fabio Estevam]

Hi Peng,

On Tue, Dec 22, 2015 at 7:14 AM, Peng Fan <van.freenix@gmail.com> wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Use snprintf to replace sprintf.

You need to improve your commit log by saying why you are doing this change.

[U-Boot] [PATCH 1/1] common: nvedit: use snprintf instead of sprintf

[Tom Rini]

On Tue, Dec 22, 2015 at 07:53:12AM -0200, Fabio Estevam wrote:
> Hi Peng,
> 
> On Tue, Dec 22, 2015 at 7:14 AM, Peng Fan <van.freenix@gmail.com> wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Use snprintf to replace sprintf.
> 
> You need to improve your commit log by saying why you are doing this change.


Yes, please do so.  And if you're using Coverity internally you can
still do a Reported-by: Coverity.

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

[U-Boot] [PATCH 1/1] common: nvedit: use snprintf instead of sprintf

[Peng Fan]

Hi All,
On Tue, Dec 22, 2015 at 02:40:58PM -0500, Tom Rini wrote:
>On Tue, Dec 22, 2015 at 07:53:12AM -0200, Fabio Estevam wrote:
>> Hi Peng,
>> 
>> On Tue, Dec 22, 2015 at 7:14 AM, Peng Fan <van.freenix@gmail.com> wrote:
>> > From: Peng Fan <peng.fan@nxp.com>
>> >
>> > Use snprintf to replace sprintf.
>> 
>> You need to improve your commit log by saying why you are doing this change.

will add more commit log.
>
>
>Yes, please do so.  And if you're using Coverity internally you can
>still do a Reported-by: Coverity.
will add this.

Thanks,
Peng.


>
>-- 
>Tom

[U-Boot] [PATCH 1/1] common: nvedit: use snprintf instead of sprintf

[Joe Hershberger]

On Tue, Dec 22, 2015 at 3:14 AM, Peng Fan <van.freenix@gmail.com> wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Use snprintf to replace sprintf.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Joe Hershberger <joe.hershberger@ni.com>

Seems safer.
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 1/1] common: cli: avoid memory leak

[Simon Glass]

Hi Peng,

On 22 December 2015 at 02:14, Peng Fan <van.freenix@gmail.com> wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Whether CONFIG_SYS_HUSH_PARSER is defined or not, should always
> check to free 'buff' to avoid memory leak.

Are you sure? I believe need_buff is only true if the simple parser is
being used.

>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  common/cli.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/cli.c b/common/cli.c
> index fbcd339..119d282 100644
> --- a/common/cli.c
> +++ b/common/cli.c
> @@ -103,9 +103,9 @@ int run_command_list(const char *cmd, int len, int flag)
>          * is pretty rare.
>          */
>         rcode = cli_simple_run_command_list(buff, flag);
> +#endif
>         if (need_buff)
>                 free(buff);
> -#endif
>
>         return rcode;
>  }
> --
> 2.6.2
>

Regards,
Simon

[U-Boot] [PATCH 1/1] common: cli: avoid memory leak

[Peng Fan]

Hi Simon,
On Sun, Dec 27, 2015 at 09:22:01PM -0700, Simon Glass wrote:
>Hi Peng,
>
>On 22 December 2015 at 02:14, Peng Fan <van.freenix@gmail.com> wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> Whether CONFIG_SYS_HUSH_PARSER is defined or not, should always
>> check to free 'buff' to avoid memory leak.
>
>Are you sure? I believe need_buff is only true if the simple parser is
>being used.

If CONFIG_SYS_HUSH_PARSER is defined and len is not -1, need_buff is 1, then
will malloc buffer and assign return value to buff.
But we only free buff, when CONFIG_SYS_HUSH_PARSER is not defined and need_buff is 1.
So I think this may leaks memory.

I am not familar with HUSH PARSER internal. If it can handle the upper case that
I described, then no need this patch.

Regards,
Peng.

>
>>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>  common/cli.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/cli.c b/common/cli.c
>> index fbcd339..119d282 100644
>> --- a/common/cli.c
>> +++ b/common/cli.c
>> @@ -103,9 +103,9 @@ int run_command_list(const char *cmd, int len, int flag)
>>          * is pretty rare.
>>          */
>>         rcode = cli_simple_run_command_list(buff, flag);
>> +#endif
>>         if (need_buff)
>>                 free(buff);
>> -#endif
>>
>>         return rcode;
>>  }
>> --
>> 2.6.2
>>
>
>Regards,
>Simon

[U-Boot] [PATCH 1/1] common: cli: avoid memory leak

[Tom Rini]

On Mon, Dec 28, 2015 at 01:12:21PM +0800, Peng Fan wrote:
> Hi Simon,
> On Sun, Dec 27, 2015 at 09:22:01PM -0700, Simon Glass wrote:
> >Hi Peng,
> >
> >On 22 December 2015 at 02:14, Peng Fan <van.freenix@gmail.com> wrote:
> >> From: Peng Fan <peng.fan@nxp.com>
> >>
> >> Whether CONFIG_SYS_HUSH_PARSER is defined or not, should always
> >> check to free 'buff' to avoid memory leak.
> >
> >Are you sure? I believe need_buff is only true if the simple parser is
> >being used.
> 
> If CONFIG_SYS_HUSH_PARSER is defined and len is not -1, need_buff is 1, then
> will malloc buffer and assign return value to buff.
> But we only free buff, when CONFIG_SYS_HUSH_PARSER is not defined and need_buff is 1.
> So I think this may leaks memory.

Yes, this may happen.  The code itself however should be rearranged (not
in this patch) to be clear on what we're doing here and if len will
really should ever be -1 (or always be -1?) or not.

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

[U-Boot] [U-Boot,1/1] common: cli: avoid memory leak

[Tom Rini]

On Tue, Dec 22, 2015 at 05:14:13PM +0800, Peng Fan wrote:

> From: Peng Fan <peng.fan@nxp.com>
> 
> Whether CONFIG_SYS_HUSH_PARSER is defined or not, should always
> check to free 'buff' to avoid memory leak.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

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