Skip to contentSkip to conversation

common: cli: avoid memory leak

PATCH New Peng Fan 6 participants 13 messages Dec 22, 2015 – Jan 4, 2016

lore
Share & tools
lore

Peng Fan

author
#0 lore raw

PATCH 1/1 common: cli: avoid memory leak

From: Peng Fan <[email protected]>

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

Signed-off-by Peng Fan <[email protected]>
Cc Tom Rini <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/cli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/cli.c +1 -1
index fbcd339..119d282 100644
@@ -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;
 }
Signature · 1 line
2.6.2
author
#1 lore raw

PATCH 1/1 common: env: initialize scalar variable

From: Peng Fan <[email protected]>

Before calling hsearch_r, initialize callback entry to NULL.

Signed-off-by Peng Fan <[email protected]>
Cc Tom Rini <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/env_callback.c | 1 +
 common/env_flags.c    | 1 +
 2 files changed, 2 insertions(+)
common/env_callback.c +1 -0
index f4d3dbd..1957cc1 100644
@@ -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? */
common/env_flags.c +1 -0
index e682d85..7719355 100644
@@ -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? */
Signature · 1 line
2.6.2
#2 lore raw

PATCH 1/1 common: env: initialize scalar variable

Dear Peng Fan,

Quoted 3 lines · In message <[email protected]> you wrote:

From: Peng Fan <[email protected]>

Before calling hsearch_r, initialize callback entry to NULL.

Which exact problem are you fixing here?

Best regards,

Wolfgang Denk

Signature · 5 lines
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.
author
#3 lore raw

PATCH 1/1 common: env: initialize scalar variable

Hi Wolfgang

Quoted 8 lines · Wolfgang Denk

Dear Peng Fan,

Quoted 3 lines · In message <[email protected]> you wrote:

From: Peng Fan <[email protected]>

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.

Quoted 11 lines

Best regards,

Wolfgang Denk

Signature · 5 lines
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.
author
#4 lore raw

PATCH 1/1 common: nvedit: use snprintf instead of sprintf

In reply to Peng Fan

From: Peng Fan <[email protected]>

Use snprintf to replace sprintf.

Signed-off-by Peng Fan <[email protected]>
Cc Tom Rini <[email protected]>
Cc Simon Glass <[email protected]>
Cc Joe Hershberger <[email protected]>
---
 common/cmd_nvedit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/cmd_nvedit.c +1 -1
index 2f9cdd0..5ae9d9d 100644
@@ -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';
Signature · 1 line
2.6.2
#5 lore raw

PATCH 1/1 common: nvedit: use snprintf instead of sprintf

Hi Peng,

Quoted 3 lines · Peng Fan

From: Peng Fan <[email protected]>

Use snprintf to replace sprintf.

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

#6 lore raw

PATCH 1/1 common: nvedit: use snprintf instead of sprintf

Quoted 8 lines · Fabio Estevam

Hi Peng,

Quoted 3 lines · Peng Fan

From: Peng Fan <[email protected]>

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.

Signature · 8 lines
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>
author
#7 lore raw

PATCH 1/1 common: nvedit: use snprintf instead of sprintf

Hi All,

Quoted 9 lines · Tom Rini
Quoted 8 lines · Fabio Estevam

Hi Peng,

Quoted 3 lines · Peng Fan

From: Peng Fan <[email protected]>

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.

Quoted 4 lines

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

will add this.

Thanks,
Peng.

Quoted 3 lines
Signature · 2 lines
-- 
Tom
#9 lore raw

PATCH 1/1 common: cli: avoid memory leak

In reply to Peng Fan

Hi Peng,

Quoted 4 lines · Peng Fan

From: Peng Fan <[email protected]>

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.

Quoted 25 lines
Signed-off-by Peng Fan <[email protected]>
Cc Tom Rini <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/cli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/cli.c +1 -2
index fbcd339..119d282 100644
@@ -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;
 }
--
Quoted 2 lines

2.6.2

Regards,
Simon

author
#10 lore raw

PATCH 1/1 common: cli: avoid memory leak

Hi Simon,

Quoted 10 lines · Simon Glass

Hi Peng,

Quoted 4 lines · Peng Fan

From: Peng Fan <[email protected]>

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.

Quoted 31 lines
Quoted 26 lines
Signed-off-by Peng Fan <[email protected]>
Cc Tom Rini <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/cli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/cli.c +1 -2
index fbcd339..119d282 100644
@@ -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;
 }
--
Quoted 2 lines

2.6.2

Regards,
Simon

#11 lore raw

PATCH 1/1 common: cli: avoid memory leak

Quoted 10 lines · Peng Fan

Hi Simon,

Quoted 8 lines · Simon Glass

Hi Peng,

Quoted 4 lines · Peng Fan

From: Peng Fan <[email protected]>

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

Quoted 7 lines
Quoted 2 lines

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.

Signature · 8 lines
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>
#12 lore raw

PATCH 1/1 common: cli: avoid memory leak

In reply to Peng Fan
Quoted 9 lines · Peng Fan

From: Peng Fan <[email protected]>

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

Signed-off-by Peng Fan <[email protected]>
Cc Tom Rini <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Simon Glass <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
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>