[PATCH 1/1] mvebu: fix end-of-array check

[PATCH 1/1] mvebu: fix end-of-array check

[Derek LaHousse]

Properly seek the end of default_environment variables.

The current algorithm overwrites from the second variable.  This
replacement finds the end of the array of strings.

Stomped variables include "board", "soc", "loadaddr".  These can be
seen on a "env default -a" after patch, but they are not seen with a
version before the patch.

Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
---
 board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/board/Marvell/mvebu_armada-37xx/board.c
b/board/Marvell/mvebu_armada-37xx/board.c
index c6ecc323bb..ac29ac5b95 100644
--- a/board/Marvell/mvebu_armada-37xx/board.c
+++ b/board/Marvell/mvebu_armada-37xx/board.c
@@ -100,8 +100,11 @@ int board_late_init(void)
 		return 0;
 
 	/* Find free buffer in default_environment[] for new variables
*/
-	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
-	ptr += 2;
+	if (*ptr != '\0') { // Defending against empty default env
+		while ((i = strlen(ptr)) != 0) {
+			ptr += i + 1;
+		}
+	}
 
 	/*
 	 * Ensure that 'env default -a' does not erase permanent MAC
addresses
-- 
2.30.2

Re: [EXT] [PATCH 1/1] mvebu: fix end-of-array check

[Kostya Porotchkin]

________________________________
From: Derek LaHousse <derek@seaofdirac.org>
Sent: Wednesday, November 30, 2022 20:33
To: u-boot@lists.denx.de <u-boot@lists.denx.de>
Cc: sr@denx.de <sr@denx.de>; Kostya Porotchkin <kostap@marvell.com>; pali@kernel.org <pali@kernel.org>
Subject: [EXT] [PATCH 1/1] mvebu: fix end-of-array check

External Email

----------------------------------------------------------------------
Properly seek the end of default_environment variables.

The current algorithm overwrites from the second variable.  This
replacement finds the end of the array of strings.

Stomped variables include "board", "soc", "loadaddr".  These can be
seen on a "env default -a" after patch, but they are not seen with a
version before the patch.

Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
---
 board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/board/Marvell/mvebu_armada-37xx/board.c
b/board/Marvell/mvebu_armada-37xx/board.c
index c6ecc323bb..ac29ac5b95 100644
--- a/board/Marvell/mvebu_armada-37xx/board.c
+++ b/board/Marvell/mvebu_armada-37xx/board.c
@@ -100,8 +100,11 @@ int board_late_init(void)
                 return 0;

         /* Find free buffer in default_environment[] for new variables
*/
-       while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
-       ptr += 2;
+       if (*ptr != '\0') { // Defending against empty default env
+               while ((i = strlen(ptr)) != 0) {
+                       ptr += i + 1;
+               }
+       }

         /*
          * Ensure that 'env default -a' does not erase permanent MAC
addresses

Acked-by: Konstantin Porotchkin <kostap@marvell.com>

--
2.30.2

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Pali Rohár]

Hello!

I would suggest to change description of the patch from

  "mvebu: fix end-of-array check"

to something like:

  "arm: mvebu: Espressobin: fix end-of-array check in env"

as it affects only Espressobin boards (not other mvebu).

Stefan, please look below as this issue/fix is important.

On Wednesday 30 November 2022 13:33:40 Derek LaHousse wrote:
> Properly seek the end of default_environment variables.
> 
> The current algorithm overwrites from the second variable.  This
> replacement finds the end of the array of strings.
> 
> Stomped variables include "board", "soc", "loadaddr".  These can be
> seen on a "env default -a" after patch, but they are not seen with a
> version before the patch.

This is a real issue which I introduced in the past. So it some fix for
this issue should be pulled into the v2023.01 release.

> Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
> ---
>  board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/board/Marvell/mvebu_armada-37xx/board.c
> b/board/Marvell/mvebu_armada-37xx/board.c
> index c6ecc323bb..ac29ac5b95 100644
> --- a/board/Marvell/mvebu_armada-37xx/board.c
> +++ b/board/Marvell/mvebu_armada-37xx/board.c
> @@ -100,8 +100,11 @@ int board_late_init(void)
>  		return 0;
>  
>  	/* Find free buffer in default_environment[] for new variables
> */
> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
> -	ptr += 2;
> +	if (*ptr != '\0') { // Defending against empty default env
> +		while ((i = strlen(ptr)) != 0) {
> +			ptr += i + 1;
> +		}
> +	}

If I'm looking at the outer-if condition correctly then it is not
needed. strlen("") returns zero and so inner-while loop stops
immediately.

My proposed fix for this issue was just changing correct while loop
check to ensure that ptr is set after the _last_ variable.

-	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
-	ptr += 2;
+	while (*ptr != '\0' || *(ptr+1) != '\0') ptr++;
+	ptr++;

Both changes should be equivalent, but I'm not sure which one is more
readable. The original issue was introduced really by non-readable
code...

Stefan, do you have a preference which one fix is better / more
readable?

It would be really a good idea if you try to check if any of those
proposed fixes/patches are now really correct.

>  
>  	/*
>  	 * Ensure that 'env default -a' does not erase permanent MAC
> addresses
> -- 
> 2.30.2
> 
> 
> 

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Stefan Roese]

Hi!

On 12/4/22 11:39, Pali Rohár wrote:
> Hello!
> 
> I would suggest to change description of the patch from
> 
>    "mvebu: fix end-of-array check"
> 
> to something like:
> 
>    "arm: mvebu: Espressobin: fix end-of-array check in env"
> 
> as it affects only Espressobin boards (not other mvebu).

Yes, please update the commit subject here.

> Stefan, please look below as this issue/fix is important.

Yes.

> On Wednesday 30 November 2022 13:33:40 Derek LaHousse wrote:
>> Properly seek the end of default_environment variables.
>>
>> The current algorithm overwrites from the second variable.  This
>> replacement finds the end of the array of strings.
>>
>> Stomped variables include "board", "soc", "loadaddr".  These can be
>> seen on a "env default -a" after patch, but they are not seen with a
>> version before the patch.
> 
> This is a real issue which I introduced in the past. So it some fix for
> this issue should be pulled into the v2023.01 release.

Understood.

>> Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
>> ---
>>   board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/board/Marvell/mvebu_armada-37xx/board.c
>> b/board/Marvell/mvebu_armada-37xx/board.c
>> index c6ecc323bb..ac29ac5b95 100644
>> --- a/board/Marvell/mvebu_armada-37xx/board.c
>> +++ b/board/Marvell/mvebu_armada-37xx/board.c
>> @@ -100,8 +100,11 @@ int board_late_init(void)
>>   		return 0;
>>   
>>   	/* Find free buffer in default_environment[] for new variables
>> */
>> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
>> -	ptr += 2;
>> +	if (*ptr != '\0') { // Defending against empty default env
>> +		while ((i = strlen(ptr)) != 0) {
>> +			ptr += i + 1;
>> +		}
>> +	}
> 
> If I'm looking at the outer-if condition correctly then it is not
> needed. strlen("") returns zero and so inner-while loop stops
> immediately.
> 
> My proposed fix for this issue was just changing correct while loop
> check to ensure that ptr is set after the _last_ variable.
> 
> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
> -	ptr += 2;
> +	while (*ptr != '\0' || *(ptr+1) != '\0') ptr++;
> +	ptr++;
> 
> Both changes should be equivalent, but I'm not sure which one is more
> readable. The original issue was introduced really by non-readable
> code...
> 
> Stefan, do you have a preference which one fix is better / more
> readable?

I would prefer to get Pali's corrected version included. Could you
please prepare a v2 patch with this update and also with the added
or changed patch subject.

> It would be really a good idea if you try to check if any of those
> proposed fixes/patches are now really correct.

Yes, please do.

Thanks,
Stefan

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Pali Rohár]

On Monday 05 December 2022 12:42:44 Stefan Roese wrote:
> Hi!
> 
> On 12/4/22 11:39, Pali Rohár wrote:
> > Hello!
> > 
> > I would suggest to change description of the patch from
> > 
> >    "mvebu: fix end-of-array check"
> > 
> > to something like:
> > 
> >    "arm: mvebu: Espressobin: fix end-of-array check in env"
> > 
> > as it affects only Espressobin boards (not other mvebu).
> 
> Yes, please update the commit subject here.
> 
> > Stefan, please look below as this issue/fix is important.
> 
> Yes.
> 
> > On Wednesday 30 November 2022 13:33:40 Derek LaHousse wrote:
> > > Properly seek the end of default_environment variables.
> > > 
> > > The current algorithm overwrites from the second variable.  This
> > > replacement finds the end of the array of strings.
> > > 
> > > Stomped variables include "board", "soc", "loadaddr".  These can be
> > > seen on a "env default -a" after patch, but they are not seen with a
> > > version before the patch.
> > 
> > This is a real issue which I introduced in the past. So it some fix for
> > this issue should be pulled into the v2023.01 release.
> 
> Understood.
> 
> > > Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
> > > ---
> > >   board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/board/Marvell/mvebu_armada-37xx/board.c
> > > b/board/Marvell/mvebu_armada-37xx/board.c
> > > index c6ecc323bb..ac29ac5b95 100644
> > > --- a/board/Marvell/mvebu_armada-37xx/board.c
> > > +++ b/board/Marvell/mvebu_armada-37xx/board.c
> > > @@ -100,8 +100,11 @@ int board_late_init(void)
> > >   		return 0;
> > >   	/* Find free buffer in default_environment[] for new variables
> > > */
> > > -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
> > > -	ptr += 2;
> > > +	if (*ptr != '\0') { // Defending against empty default env
> > > +		while ((i = strlen(ptr)) != 0) {
> > > +			ptr += i + 1;
> > > +		}
> > > +	}
> > 
> > If I'm looking at the outer-if condition correctly then it is not
> > needed. strlen("") returns zero and so inner-while loop stops
> > immediately.
> > 
> > My proposed fix for this issue was just changing correct while loop
> > check to ensure that ptr is set after the _last_ variable.
> > 
> > -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
> > -	ptr += 2;
> > +	while (*ptr != '\0' || *(ptr+1) != '\0') ptr++;
> > +	ptr++;
> > 
> > Both changes should be equivalent, but I'm not sure which one is more
> > readable. The original issue was introduced really by non-readable
> > code...
> > 
> > Stefan, do you have a preference which one fix is better / more
> > readable?
> 
> I would prefer to get Pali's corrected version included. Could you
> please prepare a v2 patch with this update and also with the added
> or changed patch subject.

Originally this issue was reported half year ago on the armbian forum:
https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=138136

U-Boot "changed" variable name scriptaddr to criptaddr (without leading
s) and broke booting.

Details are later in comments:
https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154062
https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154235

If you prefer my variant, I can prepare a v2 patch.

Please let me know.

> > It would be really a good idea if you try to check if any of those
> > proposed fixes/patches are now really correct.
> 
> Yes, please do.
> 
> Thanks,
> Stefan

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Stefan Roese]

Hi Pali,

On 12/5/22 19:18, Pali Rohár wrote:
> On Monday 05 December 2022 12:42:44 Stefan Roese wrote:
>> Hi!
>>
>> On 12/4/22 11:39, Pali Rohár wrote:
>>> Hello!
>>>
>>> I would suggest to change description of the patch from
>>>
>>>     "mvebu: fix end-of-array check"
>>>
>>> to something like:
>>>
>>>     "arm: mvebu: Espressobin: fix end-of-array check in env"
>>>
>>> as it affects only Espressobin boards (not other mvebu).
>>
>> Yes, please update the commit subject here.
>>
>>> Stefan, please look below as this issue/fix is important.
>>
>> Yes.
>>
>>> On Wednesday 30 November 2022 13:33:40 Derek LaHousse wrote:
>>>> Properly seek the end of default_environment variables.
>>>>
>>>> The current algorithm overwrites from the second variable.  This
>>>> replacement finds the end of the array of strings.
>>>>
>>>> Stomped variables include "board", "soc", "loadaddr".  These can be
>>>> seen on a "env default -a" after patch, but they are not seen with a
>>>> version before the patch.
>>>
>>> This is a real issue which I introduced in the past. So it some fix for
>>> this issue should be pulled into the v2023.01 release.
>>
>> Understood.
>>
>>>> Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
>>>> ---
>>>>    board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/board/Marvell/mvebu_armada-37xx/board.c
>>>> b/board/Marvell/mvebu_armada-37xx/board.c
>>>> index c6ecc323bb..ac29ac5b95 100644
>>>> --- a/board/Marvell/mvebu_armada-37xx/board.c
>>>> +++ b/board/Marvell/mvebu_armada-37xx/board.c
>>>> @@ -100,8 +100,11 @@ int board_late_init(void)
>>>>    		return 0;
>>>>    	/* Find free buffer in default_environment[] for new variables
>>>> */
>>>> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
>>>> -	ptr += 2;
>>>> +	if (*ptr != '\0') { // Defending against empty default env
>>>> +		while ((i = strlen(ptr)) != 0) {
>>>> +			ptr += i + 1;
>>>> +		}
>>>> +	}
>>>
>>> If I'm looking at the outer-if condition correctly then it is not
>>> needed. strlen("") returns zero and so inner-while loop stops
>>> immediately.
>>>
>>> My proposed fix for this issue was just changing correct while loop
>>> check to ensure that ptr is set after the _last_ variable.
>>>
>>> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
>>> -	ptr += 2;
>>> +	while (*ptr != '\0' || *(ptr+1) != '\0') ptr++;
>>> +	ptr++;
>>>
>>> Both changes should be equivalent, but I'm not sure which one is more
>>> readable. The original issue was introduced really by non-readable
>>> code...
>>>
>>> Stefan, do you have a preference which one fix is better / more
>>> readable?
>>
>> I would prefer to get Pali's corrected version included. Could you
>> please prepare a v2 patch with this update and also with the added
>> or changed patch subject.
> 
> Originally this issue was reported half year ago on the armbian forum:
> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=138136
> 
> U-Boot "changed" variable name scriptaddr to criptaddr (without leading
> s) and broke booting.
> 
> Details are later in comments:
> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154062
> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154235
> 
> If you prefer my variant, I can prepare a v2 patch.

Yes, please do. That's easiest for me. I'll push this quickly then.

Thanks,
Stefan

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Pali Rohár]

On Tuesday 06 December 2022 07:10:15 Stefan Roese wrote:
> Hi Pali,
> 
> On 12/5/22 19:18, Pali Rohár wrote:
> > On Monday 05 December 2022 12:42:44 Stefan Roese wrote:
> > > Hi!
> > > 
> > > On 12/4/22 11:39, Pali Rohár wrote:
> > > > Hello!
> > > > 
> > > > I would suggest to change description of the patch from
> > > > 
> > > >     "mvebu: fix end-of-array check"
> > > > 
> > > > to something like:
> > > > 
> > > >     "arm: mvebu: Espressobin: fix end-of-array check in env"
> > > > 
> > > > as it affects only Espressobin boards (not other mvebu).
> > > 
> > > Yes, please update the commit subject here.
> > > 
> > > > Stefan, please look below as this issue/fix is important.
> > > 
> > > Yes.
> > > 
> > > > On Wednesday 30 November 2022 13:33:40 Derek LaHousse wrote:
> > > > > Properly seek the end of default_environment variables.
> > > > > 
> > > > > The current algorithm overwrites from the second variable.  This
> > > > > replacement finds the end of the array of strings.
> > > > > 
> > > > > Stomped variables include "board", "soc", "loadaddr".  These can be
> > > > > seen on a "env default -a" after patch, but they are not seen with a
> > > > > version before the patch.
> > > > 
> > > > This is a real issue which I introduced in the past. So it some fix for
> > > > this issue should be pulled into the v2023.01 release.
> > > 
> > > Understood.
> > > 
> > > > > Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
> > > > > ---
> > > > >    board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
> > > > >    1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/board/Marvell/mvebu_armada-37xx/board.c
> > > > > b/board/Marvell/mvebu_armada-37xx/board.c
> > > > > index c6ecc323bb..ac29ac5b95 100644
> > > > > --- a/board/Marvell/mvebu_armada-37xx/board.c
> > > > > +++ b/board/Marvell/mvebu_armada-37xx/board.c
> > > > > @@ -100,8 +100,11 @@ int board_late_init(void)
> > > > >    		return 0;
> > > > >    	/* Find free buffer in default_environment[] for new variables
> > > > > */
> > > > > -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
> > > > > -	ptr += 2;
> > > > > +	if (*ptr != '\0') { // Defending against empty default env
> > > > > +		while ((i = strlen(ptr)) != 0) {
> > > > > +			ptr += i + 1;
> > > > > +		}
> > > > > +	}
> > > > 
> > > > If I'm looking at the outer-if condition correctly then it is not
> > > > needed. strlen("") returns zero and so inner-while loop stops
> > > > immediately.
> > > > 
> > > > My proposed fix for this issue was just changing correct while loop
> > > > check to ensure that ptr is set after the _last_ variable.
> > > > 
> > > > -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
> > > > -	ptr += 2;
> > > > +	while (*ptr != '\0' || *(ptr+1) != '\0') ptr++;
> > > > +	ptr++;
> > > > 
> > > > Both changes should be equivalent, but I'm not sure which one is more
> > > > readable. The original issue was introduced really by non-readable
> > > > code...
> > > > 
> > > > Stefan, do you have a preference which one fix is better / more
> > > > readable?
> > > 
> > > I would prefer to get Pali's corrected version included. Could you
> > > please prepare a v2 patch with this update and also with the added
> > > or changed patch subject.
> > 
> > Originally this issue was reported half year ago on the armbian forum:
> > https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=138136
> > 
> > U-Boot "changed" variable name scriptaddr to criptaddr (without leading
> > s) and broke booting.
> > 
> > Details are later in comments:
> > https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154062
> > https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154235
> > 
> > If you prefer my variant, I can prepare a v2 patch.
> 
> Yes, please do. That's easiest for me. I'll push this quickly then.
> 
> Thanks,
> Stefan

Hello Stefan! During discussion on the forum, Derek pointed that my
"fix" does not work for another edge case when *ptr is '\0' before
entering into while-loop. That is a valid point and code needs to be
adjusted.

With Derek we do not have an agreement how to write this peace of the
code in readable way, which should move ptr pointer to the end of env
list - find two nul bytes which indicates end of the env list and then
set ptr to the second nul byte, so at this position can be put another
new variable. Plus handle special case nul byte is at the beginning.

My idea was to use single while loop to find this location to have code
minimal in its size. Derek thinks that code is better readable if
iteration is done via strlen() calls in while-loop, like it is in this
version.

Stefan, do you have an idea how to write this code in a way that is fast
and also readable and maintainable?

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Stefan Roese]

Hi Pali,
Hi Derek,

On 12/6/22 20:56, Pali Rohár wrote:
> On Tuesday 06 December 2022 07:10:15 Stefan Roese wrote:
>> Hi Pali,
>>
>> On 12/5/22 19:18, Pali Rohár wrote:
>>> On Monday 05 December 2022 12:42:44 Stefan Roese wrote:
>>>> Hi!
>>>>
>>>> On 12/4/22 11:39, Pali Rohár wrote:
>>>>> Hello!
>>>>>
>>>>> I would suggest to change description of the patch from
>>>>>
>>>>>      "mvebu: fix end-of-array check"
>>>>>
>>>>> to something like:
>>>>>
>>>>>      "arm: mvebu: Espressobin: fix end-of-array check in env"
>>>>>
>>>>> as it affects only Espressobin boards (not other mvebu).
>>>>
>>>> Yes, please update the commit subject here.
>>>>
>>>>> Stefan, please look below as this issue/fix is important.
>>>>
>>>> Yes.
>>>>
>>>>> On Wednesday 30 November 2022 13:33:40 Derek LaHousse wrote:
>>>>>> Properly seek the end of default_environment variables.
>>>>>>
>>>>>> The current algorithm overwrites from the second variable.  This
>>>>>> replacement finds the end of the array of strings.
>>>>>>
>>>>>> Stomped variables include "board", "soc", "loadaddr".  These can be
>>>>>> seen on a "env default -a" after patch, but they are not seen with a
>>>>>> version before the patch.
>>>>>
>>>>> This is a real issue which I introduced in the past. So it some fix for
>>>>> this issue should be pulled into the v2023.01 release.
>>>>
>>>> Understood.
>>>>
>>>>>> Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
>>>>>> ---
>>>>>>     board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
>>>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/board/Marvell/mvebu_armada-37xx/board.c
>>>>>> b/board/Marvell/mvebu_armada-37xx/board.c
>>>>>> index c6ecc323bb..ac29ac5b95 100644
>>>>>> --- a/board/Marvell/mvebu_armada-37xx/board.c
>>>>>> +++ b/board/Marvell/mvebu_armada-37xx/board.c
>>>>>> @@ -100,8 +100,11 @@ int board_late_init(void)
>>>>>>     		return 0;
>>>>>>     	/* Find free buffer in default_environment[] for new variables
>>>>>> */
>>>>>> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
>>>>>> -	ptr += 2;
>>>>>> +	if (*ptr != '\0') { // Defending against empty default env
>>>>>> +		while ((i = strlen(ptr)) != 0) {
>>>>>> +			ptr += i + 1;
>>>>>> +		}
>>>>>> +	}
>>>>>
>>>>> If I'm looking at the outer-if condition correctly then it is not
>>>>> needed. strlen("") returns zero and so inner-while loop stops
>>>>> immediately.
>>>>>
>>>>> My proposed fix for this issue was just changing correct while loop
>>>>> check to ensure that ptr is set after the _last_ variable.
>>>>>
>>>>> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
>>>>> -	ptr += 2;
>>>>> +	while (*ptr != '\0' || *(ptr+1) != '\0') ptr++;
>>>>> +	ptr++;
>>>>>
>>>>> Both changes should be equivalent, but I'm not sure which one is more
>>>>> readable. The original issue was introduced really by non-readable
>>>>> code...
>>>>>
>>>>> Stefan, do you have a preference which one fix is better / more
>>>>> readable?
>>>>
>>>> I would prefer to get Pali's corrected version included. Could you
>>>> please prepare a v2 patch with this update and also with the added
>>>> or changed patch subject.
>>>
>>> Originally this issue was reported half year ago on the armbian forum:
>>> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=138136
>>>
>>> U-Boot "changed" variable name scriptaddr to criptaddr (without leading
>>> s) and broke booting.
>>>
>>> Details are later in comments:
>>> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154062
>>> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154235
>>>
>>> If you prefer my variant, I can prepare a v2 patch.
>>
>> Yes, please do. That's easiest for me. I'll push this quickly then.
>>
>> Thanks,
>> Stefan
> 
> Hello Stefan! During discussion on the forum, Derek pointed that my
> "fix" does not work for another edge case when *ptr is '\0' before
> entering into while-loop. That is a valid point and code needs to be
> adjusted.
> 
> With Derek we do not have an agreement how to write this peace of the
> code in readable way, which should move ptr pointer to the end of env
> list - find two nul bytes which indicates end of the env list and then
> set ptr to the second nul byte, so at this position can be put another
> new variable. Plus handle special case nul byte is at the beginning.
> 
> My idea was to use single while loop to find this location to have code
> minimal in its size. Derek thinks that code is better readable if
> iteration is done via strlen() calls in while-loop, like it is in this
> version.
> 
> Stefan, do you have an idea how to write this code in a way that is fast
> and also readable and maintainable?

Sorry, I don't have an "idea" quickly that might be "better" than your
suggested version(s) without diving into this deeper. You are both
experienced developers I assume. I have no real problem using Derek's
version if this solves the problem and is a bit slower. The code
patch we are talking about is not accessed frequently I assume.

Thanks,
Stefan

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Simon Glass]

On Thu, 1 Dec 2022 at 07:55, Derek LaHousse <derek@seaofdirac.org> wrote:
>
> Properly seek the end of default_environment variables.
>
> The current algorithm overwrites from the second variable.  This
> replacement finds the end of the array of strings.
>
> Stomped variables include "board", "soc", "loadaddr".  These can be
> seen on a "env default -a" after patch, but they are not seen with a
> version before the patch.
>
> Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
> ---
>  board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

It seems odd to update the default environment, but if we want to do
this, it should really be done in a function in env/ along with some
tests in test/env

Regards,
Simon

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Pali Rohár]

On Monday 05 December 2022 10:16:51 Simon Glass wrote:
> On Thu, 1 Dec 2022 at 07:55, Derek LaHousse <derek@seaofdirac.org> wrote:
> >
> > Properly seek the end of default_environment variables.
> >
> > The current algorithm overwrites from the second variable.  This
> > replacement finds the end of the array of strings.
> >
> > Stomped variables include "board", "soc", "loadaddr".  These can be
> > seen on a "env default -a" after patch, but they are not seen with a
> > version before the patch.
> >
> > Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
> > ---
> >  board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> It seems odd to update the default environment, but if we want to do
> this, it should really be done in a function in env/ along with some
> tests in test/env
> 
> Regards,
> Simon

Well, this is just a temporary solution until Marek's env patch series is merged:
https://lore.kernel.org/u-boot/20211103232332.2737-1-kabel@kernel.org/

But I agree that tests would prevent these bugs.

Re: [PATCH 1/1] mvebu: fix end-of-array check

[Simon Glass]

Hi Pali,

On Mon, 5 Dec 2022 at 10:51, Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 05 December 2022 10:16:51 Simon Glass wrote:
> > On Thu, 1 Dec 2022 at 07:55, Derek LaHousse <derek@seaofdirac.org> wrote:
> > >
> > > Properly seek the end of default_environment variables.
> > >
> > > The current algorithm overwrites from the second variable.  This
> > > replacement finds the end of the array of strings.
> > >
> > > Stomped variables include "board", "soc", "loadaddr".  These can be
> > > seen on a "env default -a" after patch, but they are not seen with a
> > > version before the patch.
> > >
> > > Signed-off-by: Derek LaHousse <derek@seaofdirac.org>
> > > ---
> > >  board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > It seems odd to update the default environment, but if we want to do
> > this, it should really be done in a function in env/ along with some
> > tests in test/env
> >
> > Regards,
> > Simon
>
> Well, this is just a temporary solution until Marek's env patch series is merged:
> https://lore.kernel.org/u-boot/20211103232332.2737-1-kabel@kernel.org/
>
> But I agree that tests would prevent these bugs.

OK I see.

Regards,
Simon