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

> 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