Skip to contentSkip to conversation

common: cli_hush: avoid memory leak

PATCH New Peng Fan 4 participants 7 messages Nov 25, 2015 – Dec 6, 2015

lore
Share & tools
lore

Peng Fan

author
#0 lore raw

PATCH 1/1 common: cli_hush: avoid memory leak

Need to free memory avoid memory leak, when error.

Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Tom Rini <[email protected]>
---
 common/cli_hush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
common/cli_hush.c +3 -1
index f075459..ab85225 100644
@@ -2474,8 +2474,10 @@ static int done_word(o_string *dest, struct p_context *ctx)
 		if (child->argv == NULL) return 1;
 		child->argv_nonnull = realloc(child->argv_nonnull,
 					(argc+1)*sizeof(*child->argv_nonnull));
-		if (child->argv_nonnull == NULL)
+		if (child->argv_nonnull == NULL) {
+			free(str);
 			return 1;
+		}
 		child->argv[argc-1]=str;
 		child->argv_nonnull[argc-1] = dest->nonnull;
 		child->argv[argc]=NULL;
Signature · 1 line
2.6.2
author
#1 lore raw

PATCH 1/1 common: mmc: unsigned char compared against 0

"enable" is unsigned char type and its value will not be negative, so discard "enable < 0".

Signed-off-by Peng Fan <[email protected]>
Cc Diego Santa Cruz <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Andrew Gabbasov <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Tom Rini <[email protected]>
---
 common/cmd_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/cmd_mmc.c +1 -1
index dfc1ec8..a6b7313 100644
@@ -747,7 +747,7 @@ static int do_mmc_rst_func(cmd_tbl_t *cmdtp, int flag,
 	dev = simple_strtoul(argv[1], NULL, 10);
 	enable = simple_strtoul(argv[2], NULL, 10);
 
-	if (enable > 2 || enable < 0) {
+	if (enable > 2) {
 		puts("Invalid RST_n_ENABLE value\n");
 		return CMD_RET_USAGE;
 	}
Signature · 1 line
2.6.2
#2 lore raw

PATCH 1/1 common: mmc: unsigned char compared against 0

Hi Peng,

Quoted 10 lines · Peng Fan

"enable" is unsigned char type and its value will not be negative, so discard "enable < 0".

Signed-off-by Peng Fan <[email protected]>
Cc Diego Santa Cruz <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Andrew Gabbasov <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Tom Rini <[email protected]>
Quoted 3 lines
---
 common/cmd_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by Simon Glass <[email protected]>

Even better if this variable changed to uint, instead of u8.

Regards,
Simon

#3 lore raw

PATCH 1/1 common: mmc: unsigned char compared against 0

Hi Simon,

Quoted 17 lines · Simon Glass

Hi Peng,

Quoted 10 lines · Peng Fan

"enable" is unsigned char type and its value will not be negative, so discard "enable < 0".

Signed-off-by Peng Fan <[email protected]>
Cc Diego Santa Cruz <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Andrew Gabbasov <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Tom Rini <[email protected]>
Quoted 3 lines
---
 common/cmd_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 3 lines
Reviewed-by Simon Glass <[email protected]>

Even better if this variable changed to uint, instead of u8.

Thanks for reviewing, But I prefer to let it be, since mmc_set_rst_n_function takes u8 type for input parameter.

Thanks,
Peng.

Quoted 3 lines

Regards,
Simon

#4 lore raw

[U-Boot] common: mmc: unsigned char compared against 0

In reply to Peng Fan
Quoted 11 lines · Peng Fan

"enable" is unsigned char type and its value will not be negative, so discard "enable < 0".

Signed-off-by Peng Fan <[email protected]>
Cc Diego Santa Cruz <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Andrew Gabbasov <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Tom Rini <[email protected]>
Reviewed-by 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/20151206/c0c0ab22/attachment.sig>
#5 lore raw

PATCH 1/1 common: cli_hush: avoid memory leak

In reply to Peng Fan

Hi Peng,

Quoted 26 lines · Peng Fan

Need to free memory avoid memory leak, when error.

Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Tom Rini <[email protected]>
---
 common/cli_hush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
common/cli_hush.c +3 -2
index f075459..ab85225 100644
@@ -2474,8 +2474,10 @@ static int done_word(o_string *dest, struct p_context *ctx)
                if (child->argv == NULL) return 1;
                child->argv_nonnull = realloc(child->argv_nonnull,
                                        (argc+1)*sizeof(*child->argv_nonnull));
-               if (child->argv_nonnull == NULL)
+               if (child->argv_nonnull == NULL) {
+                       free(str);
                        return 1;
+               }
                child->argv[argc-1]=str;
                child->argv_nonnull[argc-1] = dest->nonnull;
                child->argv[argc]=NULL;
--
Quoted 3 lines

2.6.2

Reviewed-by Simon Glass <[email protected]>

It looks like there is another memory leak at the 'return 1' immediately above.

Regards,
Simon

#6 lore raw

PATCH 1/1 common: cli_hush: avoid memory leak

Hi Simon,

Quoted 35 lines · Simon Glass

Hi Peng,

Quoted 26 lines · Peng Fan

Need to free memory avoid memory leak, when error.

Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Tom Rini <[email protected]>
---
 common/cli_hush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
common/cli_hush.c +3 -2
index f075459..ab85225 100644
@@ -2474,8 +2474,10 @@ static int done_word(o_string *dest, struct p_context *ctx)
                if (child->argv == NULL) return 1;
                child->argv_nonnull = realloc(child->argv_nonnull,
                                        (argc+1)*sizeof(*child->argv_nonnull));
-               if (child->argv_nonnull == NULL)
+               if (child->argv_nonnull == NULL) {
+                       free(str);
                        return 1;
+               }
                child->argv[argc-1]=str;
                child->argv_nonnull[argc-1] = dest->nonnull;
                child->argv[argc]=NULL;
--
Quoted 3 lines

2.6.2

Reviewed-by Simon Glass <[email protected]>
Quoted 1 line

It looks like there is another memory leak at the 'return 1' immediately above.

My coverity check tool does not report this (:-, but seems this is true memory leak if child->argv is NULL. Thanks for pointing this out. I'll fix this in V2.

Thanks,
Peng.

Quoted 3 lines

Regards,
Simon