[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 at 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