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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thank you star.

I ack this update.

I recall we did a review for bds on variable usage and assert usage and fixed such issue. If this is a regression, we probable need review again. 

thank you!
Yao, Jiewen


> 在 2017年10月5日，下午3:59，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
>>> 
>> 
>