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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

All,
I just sent out two patches. One to remove the assertion, the other to fix a bug in EfiBootManagerIsValidLoadOptionVariableName.

Thanks/Ray

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, October 5, 2017 5:08 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric
> <eric.dong@intel.com>; edk2-devel@lists.01.org; leif.lindholm@linaro.org
> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT
> on 'BootNext' varname
> 
> 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
> >>>
> >>
> >