[U-Boot] [PATCH v1] env: fix crash using default -f -a

[Gerlando Falauto]

Hi Stefano,

thanks for reporting and providing a fix for this.
I'm very sorry for introducing this problem and for the late response.

Please see my comments below.

[As a side node, I couldn't really reproduce the issue neither on 
PowerPC nor on ARM (though simple_strtoul should legitimately crash) as 
apparently accessing NULL locations doesn't really bother our 
architectures...]

On 10/04/2012 07:30 PM, Stefano Babic wrote:
> newval pointer is not checked before its usage,
> inside env_check_apply().
>
> Signed-off-by: Stefano Babic<sbabic at denx.de>
> CC: gerlando.falauto at keymile.com
> ---
>   common/cmd_nvedit.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
> index 3474bc6..8144c85 100644
> --- a/common/cmd_nvedit.c
> +++ b/common/cmd_nvedit.c
> @@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char *oldval,
>   		/*
>   		 * Switch to new baudrate if new baudrate is supported
>   		 */
> -		if (strcmp(name, "baudrate") == 0) {
> +		if ((strcmp(name, "baudrate") == 0)&&  (newval != NULL)) {

You're right in that the problem here is due to the function explicitly 
being called with a NULL value.
My idea was to have "env default" perform some sort of cleanup before 
assigning the actual values from the default environment (if any).
So by adding these extra checks you're subtly canceling those efforts.
So I would rather provide a default value to prevent the crashes, 
instead of skipping settings altogether. Posting a patch in a few minutes.

Whether it's legitimate to perform these "cleanups", well that's of 
course open for discussion.
But I believe it doesn't make much sense to call a function and pass an 
argument which is sort of guaranteed to have no effect whatsoever.

>   			int baudrate = simple_strtoul(newval, NULL, 10);
>   			int i;
>   			for (i = 0; i<  N_BAUDRATES; ++i) {
> @@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char *oldval,
>   	 * Some variables should be updated when the corresponding
>   	 * entry in the environment is changed
>   	 */
> -	if (strcmp(name, "loadaddr") == 0) {
> +	if ((strcmp(name, "loadaddr") == 0)&&  (newval != NULL)) {
>   		load_addr = simple_strtoul(newval, NULL, 16);
>   		return 0;
>   	}
>   #if defined(CONFIG_CMD_NET)
> -	else if (strcmp(name, "bootfile") == 0) {
> +	else if ((strcmp(name, "bootfile") == 0)&&  (newval != NULL)) {
>   		copy_filename(BootFile, newval, sizeof(BootFile));
>   		return 0;
>   	}

Thank you and sorry again,
Gerlando