[U-Boot] [RFC/PATCH] common/command.c: Guard strchr/strlen from NULL pointer

[U-Boot] [RFC/PATCH] common/command.c: Guard strchr/strlen from NULL pointer

[Thomas Weber]

Guard strchr/strlen from being called with NULL pointer. 
This line is crashing on OMAP3/Devkit8000 when command "env" is called without subcommand.

Toolchain is Codesourcery 2010q1.

The cmd is NULL in this case because the calling function "do_env" decremented the argc 
without checking if there are still arguments available.

caller:
static int do_env (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
...
        /* drop initial "env" arg */
        argc--;
        argv++;

        cp = find_cmd_tbl(argv[0], cmd_env_sub, ARRAY_SIZE(cmd_env_sub));


Signed-off-by: Thomas Weber <weber@corscience.de>
---
 common/command.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/command.c b/common/command.c
index 0020eac..03a713a 100644
--- a/common/command.c
+++ b/common/command.c
@@ -105,14 +105,15 @@ cmd_tbl_t *find_cmd_tbl (const char *cmd, cmd_tbl_t *table, int table_len)
 	cmd_tbl_t *cmdtp;
 	cmd_tbl_t *cmdtp_temp = table;	/*Init value */
 	const char *p;
-	int len;
+	int len = 0;
 	int n_found = 0;
 
 	/*
 	 * Some commands allow length modifiers (like "cp.b");
 	 * compare command name only until first dot.
 	 */
-	len = ((p = strchr(cmd, '.')) == NULL) ? strlen (cmd) : (p - cmd);
+	if (cmd != NULL)
+		len = ((p = strchr(cmd, '.')) == NULL) ? strlen (cmd) : (p - cmd);
 
 	for (cmdtp = table;
 	     cmdtp != table + table_len;
-- 
1.7.3.2

[U-Boot] [RFC/PATCH] common/command.c: Guard strchr/strlen from NULL pointer

[Wolfgang Denk]

Dear Thomas Weber,

In message <1290593751-540-1-git-send-email-weber@corscience.de> you wrote:
> Guard strchr/strlen from being called with NULL pointer. 
> This line is crashing on OMAP3/Devkit8000 when command "env" is called without subcommand.
> 
> Toolchain is Codesourcery 2010q1.
> 
> The cmd is NULL in this case because the calling function "do_env" decremented the argc 
> without checking if there are still arguments available.

One could argue if "env" should be fixed, then.

>  	cmd_tbl_t *cmdtp;
>  	cmd_tbl_t *cmdtp_temp = table;	/*Init value */
>  	const char *p;
> -	int len;
> +	int len = 0;
>  	int n_found = 0;
>  
>  	/*
>  	 * Some commands allow length modifiers (like "cp.b");
>  	 * compare command name only until first dot.
>  	 */
> -	len = ((p = strchr(cmd, '.')) == NULL) ? strlen (cmd) : (p - cmd);
> +	if (cmd != NULL)
> +		len = ((p = strchr(cmd, '.')) == NULL) ? strlen (cmd) : (p - cmd);

This is a pretty logish way for a simple thing. It's recommended
practice to use a minimal return path for error handling.

Like that:

@@ -108,6 +108,9 @@ cmd_tbl_t *find_cmd_tbl (const char *cmd, cmd_tbl_t *table, int table_len)
 	int len;
 	int n_found = 0;
 
+	if (!cmd)
+		return NULL;
+
 	/*
 	 * Some commands allow length modifiers (like "cp.b");
 	 * compare command name only until first dot.

Does this work for you as well?  If yes, then please resubmit like
this.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
God grant me the senility to accept the things I cannot  change,  The
frustration  to  try to change things I cannot affect, and the wisdom
to tell the difference.

[U-Boot] [RFC/PATCHv2 1/2] Common/command: Guard strchr/strlen from NULL pointer

[Thomas Weber]

Guard strchr/strlen from being called with NULL pointer.
This line is crashing when command "env" is called without subcommand.

The cmd is NULL in this case because the calling function "do_env"
decremented the argc without checking if there are still arguments available.

Signed-off-by: Thomas Weber <weber@corscience.de>
---
Changes for v2: 
	- Use shorter way to leave function in error case.

 common/command.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/common/command.c b/common/command.c
index 0020eac..0b1a3fb 100644
--- a/common/command.c
+++ b/common/command.c
@@ -108,6 +108,8 @@ cmd_tbl_t *find_cmd_tbl (const char *cmd, cmd_tbl_t *table, int table_len)
 	int len;
 	int n_found = 0;
 
+	if (!cmd)
+		return NULL;
 	/*
 	 * Some commands allow length modifiers (like "cp.b");
 	 * compare command name only until first dot.
-- 
1.7.3.2

[U-Boot] [RFC/PATCHv2 1/2] Common/command: Guard strchr/strlen from NULL pointer

[Wolfgang Denk]

Dear Thomas Weber,

In message <1290600472-23147-1-git-send-email-weber@corscience.de> you wrote:
> Guard strchr/strlen from being called with NULL pointer.
> This line is crashing when command "env" is called without subcommand.
> 
> The cmd is NULL in this case because the calling function "do_env"
> decremented the argc without checking if there are still arguments available.
> 
> Signed-off-by: Thomas Weber <weber@corscience.de>
> ---
> Changes for v2: 
> 	- Use shorter way to leave function in error case.
> 
>  common/command.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
Every little picofarad has a nanohenry all its own.      - Don Vonada

[U-Boot] [RFC/PATCHv2 2/2] Common/cmd_nvedit: Check for env subcommand

[Thomas Weber]

The env command needs one subcommand. If this is not available
print the usage help.

Signed-off-by: Thomas Weber <weber@corscience.de>
---
 common/cmd_nvedit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 3fd8abc..52c5e7c 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -848,6 +848,9 @@ static int do_env (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	cmd_tbl_t *cp;
 
+	if (argc < 2)
+		return cmd_usage(cmdtp);
+
 	/* drop initial "env" arg */
 	argc--;
 	argv++;
-- 
1.7.3.2

[U-Boot] [RFC/PATCHv2 2/2] Common/cmd_nvedit: Check for env subcommand

[Wolfgang Denk]

Dear Thomas Weber,

In message <1290600472-23147-2-git-send-email-weber@corscience.de> you wrote:
> The env command needs one subcommand. If this is not available
> print the usage help.
> 
> Signed-off-by: Thomas Weber <weber@corscience.de>
> ---
>  common/cmd_nvedit.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
Each kiss is as the first.
	-- Miramanee, Kirk's wife, "The Paradise Syndrome",
	   stardate 4842.6

[U-Boot] [RFC/PATCH] common/command.c: Guard strchr/strlen from NULL pointer

[Sergei Shtylyov]

Hello.

On 24-11-2010 13:15, Thomas Weber wrote:

> Guard strchr/strlen from being called with NULL pointer.
> This line is crashing on OMAP3/Devkit8000 when command "env" is called without subcommand.

> Toolchain is Codesourcery 2010q1.

> The cmd is NULL in this case because the calling function "do_env" decremented the argc
> without checking if there are still arguments available.

> caller:
> static int do_env (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> ...
>          /* drop initial "env" arg */
>          argc--;
>          argv++;
>
>          cp = find_cmd_tbl(argv[0], cmd_env_sub, ARRAY_SIZE(cmd_env_sub));


> Signed-off-by: Thomas Weber<weber@corscience.de>
> ---
>   common/command.c |    5 +++--
>   1 files changed, 3 insertions(+), 2 deletions(-)

> diff --git a/common/command.c b/common/command.c
> index 0020eac..03a713a 100644
> --- a/common/command.c
> +++ b/common/command.c
> @@ -105,14 +105,15 @@ cmd_tbl_t *find_cmd_tbl (const char *cmd, cmd_tbl_t *table, int table_len)
>   	cmd_tbl_t *cmdtp;
>   	cmd_tbl_t *cmdtp_temp = table;	/*Init value */
>   	const char *p;
> -	int len;
> +	int len = 0;
>   	int n_found = 0;
>
>   	/*
>   	 * Some commands allow length modifiers (like "cp.b");
>   	 * compare command name only until first dot.
>   	 */
> -	len = ((p = strchr(cmd, '.')) == NULL) ? strlen (cmd) : (p - cmd);
> +	if (cmd != NULL)
> +		len = ((p = strchr(cmd, '.')) == NULL) ? strlen (cmd) : (p - cmd);

    checkpatch.pl would complain about the space between 'strlen' and (, so 
seems a high time to fix this. Besides, it's not consistent with strchr() 
invocation where there's no space...

WBR, Sergei