[U-Boot] [PATCH 1/1] common: cli: avoid memory leak
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; }
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? */
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
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@denx.de Beware of the Turing Tar-pit in which everything is possible but nothing of interest is easy.
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';
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.
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.
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
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
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
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
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.
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!
participants (6)
-
Fabio Estevam -
Joe Hershberger -
Peng Fan -
Simon Glass -
Tom Rini -
Wolfgang Denk