From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gerlando Falauto <gerlando.falauto@keymile.com>
Date: Fri, 05 Oct 2012 12:58:11 +0200
Subject: [U-Boot] [PATCH v1] env: fix crash using default -f -a
In-Reply-To: <506EB964.4030902@denx.de>
References: <1349371843-20939-1-git-send-email-sbabic@denx.de>
	<506EB699.7080100@keymile.com> <506EB964.4030902@denx.de>
Message-ID: <506EBD43.2070602@keymile.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Stefano,

On 10/05/2012 12:41 PM, Stefano Babic wrote:
> On 05/10/2012 12:29, Gerlando Falauto wrote:
>> Hi Stefano,
>>
>
> Hi Gerlando,
>
>> 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...]
>
> Right. For PowerPC, Address 0 is a valid address in memory and the
> system does not crash.

I didn't know that, thanks.

> However, I tested on a TI's ARM and address zero
> is not valid.
>
>>
>> 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@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).
>
> I have seen, I have not fully understood. My idea is that, if someone
> want to restore the environment to the default values, no actions should
> be taken, else the "default" values cannot be really restored.

My idea here was to "apply" the changes as soon as the environment gets 
changed, like it would happen with a manual "setenv"/"env set".
So for instance, if you switch from a custom baudrate back to the 
default baudrate, you get a message prompting you to switch to the new 
baudrate (which actually *DOES* change on the console).

Whether that should also be performed as a "cleanup" action upon 
restoring defaults, well I am not sure...

>> So by adding these extra checks you're subtly canceling those efforts.
>
> That is correct, but in any case a check for a NULL pointer should be
> done before accessing to the pointer, independently what we want to do
> with it.

Absolutely. See my proposed patch.

>> So I would rather provide a default value to prevent the crashes,
>> instead of skipping settings altogether. Posting a patch in a few minutes.
>
> Ok, I will review it. In any case, we need a fix for the coming release.

Absolutely. Once again, I cannot apologize and thank you enough for 
reporting and digging into this.
I assumed providing an own patch would make more sense than asking you 
to review yours, not sure whether it is popular practice to do so.

>> Whether it's legitimate to perform these "cleanups", well that's of
>> course open for discussion.
>
> Indeed.
>
>> 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.
>
> Right. It is the same as calling the function with do_apply=0.

Right, but since the return value is also ignored, what's the point in 
calling it in the first place?

>>
>>>                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;
>>>        }
>>
>
> Best regards,
> Stefano
>

Best regards,
Gerlando