Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Laszlo Ersek <lersek@redhat.com>]

On 10/05/17 09:31, Zeng, Star wrote:
> I got your point.
> From literal meaning of the API, I agree the ASSERT should be removed.
> If the input parameter is assumed to be valid always, the API could be not called at all.
> If the input parameter is not assumed to be valid always, the API should not assert.
> 
> I just tried an experiment and can easily reproduce the assert.
> 1. Boot NT32 to shell.
> 2. Create L"BootNext" variable with shell command: setvar BootNext -NV -RT -BS =0000.
> 3. Reboot and then ASSERT.
> ASSERT!: [BdsDxe] i:\git\edk2git\edk2\MdeModulePkg\Library\UefiBootManagerLib\BmMisc.c (423): ((BOOLEAN)(0==1))
> 
> The calling stack is:
> BdsEntry(BdsEntry.c L844) ->
>   EfiBootManagerGetLoadOptions(BdsLoadOption.c L1092) ->
>     BmCollectLoadOptions() with L"BootNext" from the loop in BmForEachVariable() ->
>       EfiBootManagerIsValidLoadOptionVariableName() ->
>         BmCharToUint() ->
>           ASSERT(FALSE)
> 
> The assert seems new caused by 0e6584e38650cef9a6b4579553679c0f12d897bc as L"BootNext" was deleted before calling EfiBootManagerGetLoadOptions() when no this commit.

Ah, good point!

OK, so let's wait until Ray acks the removal of the assert.

Thanks!
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Wednesday, October 4, 2017 11:06 PM
> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; leif.lindholm@linaro.org; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname
> 
> Star,
> 
> On 10/04/17 16:09, Zeng, Star wrote:
>> Thanks for confirming the urgency.
>>
>> I have no strong motivation to keep/remove the ASSERT, I would like Ruiyu to argue and make the decision.
>> I mainly want the issue (the code ends up calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext") could be root caused.
> 
> it might be interesting to find out about the exact call stack. However, I'd like to point out that the exact purpose of the
> EfiBootManagerIsValidLoadOptionVariableName() function is to check
> *whether* the variable name is a valid boot option name or not. If not
> -- for whatever reason -- then it shouldn't ASSERT(); it should just return FALSE.
> 
> Perhaps it's relevant: the function was made public in commit 3dc5c1ae5c757.
> 
> Thanks
> Laszlo
> 
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Wednesday, October 4, 2017 9:54 PM
>> To: Zeng, Star <star.zeng@intel.com>
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu 
>> <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric 
>> <eric.dong@intel.com>; leif.lindholm@linaro.org
>> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't 
>> ASSERT on 'BootNext' varname
>>
>> On 4 October 2017 at 14:49, Zeng, Star <star.zeng@intel.com> wrote:
>>> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it 
>>> will be rejected by 
>>> MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that will check the VariableName against UEFI spec “Table 13. Global Variables”
>>> if the VendorGuid is gEfiGlobalVariableGuid.
>>>
>>>
>>>
>>> I would suspect there is a bug at other place if the code ends up 
>>> calling this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext".
>>>
>>
>> That still does not mean you should ASSERT() here. The state of the variable store != the internals of the code, and so it should be considered external input to some extent. ASSERTs are meant to catch programming errors, not errors in the varstore image.
>>
>>
>>>
>>> Ard,
>>>
>>> Is the fix urgent or not for you?
>>>
>>
>> Not really. But fwupdate is shipping as part of many distros, so I guess others may run into it as well.
>>
>>> I may want to wait for Ruiyu’s back to take some look at the detail of it.
>>>
>>
>> That is fine.
>>
>>> At the same time, you may help check the code flow in some detail if 
>>> you have free time, I think that will be helpful. J
>>>
>>
>> OK.
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>