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

Submitted by Thomas Weber on Nov. 24, 2010, 10:15 a.m.

Details

Message ID 1290593751-540-1-git-send-email-weber@corscience.de
State Changes Requested
Headers show

Commit Message

Thomas Weber Nov. 24, 2010, 10:15 a.m.
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(-)

Comments

Wolfgang Denk Nov. 24, 2010, 11:07 a.m.
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
Sergei Shtylyov Nov. 24, 2010, 12:44 p.m.
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

Patch hide | download patch | download mbox

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;