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 <ler...@redhat.com> > Cc: Zeng, Star <star.z...@intel.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; Ni, Ruiyu <ruiyu...@intel.com>; Dong, Eric > <eric.d...@intel.com>; edk2-devel@lists.01.org; leif.lindh...@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 <ler...@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:ler...@redhat.com] > >> Sent: Wednesday, October 4, 2017 11:06 PM > >> To: Zeng, Star <star.z...@intel.com>; Ard Biesheuvel > >> <ard.biesheu...@linaro.org> > >> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Dong, Eric <eric.d...@intel.com>; > >> edk2-devel@lists.01.org; leif.lindh...@linaro.org; Yao, Jiewen > >> <jiewen....@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.biesheu...@linaro.org] > >>> Sent: Wednesday, October 4, 2017 9:54 PM > >>> To: Zeng, Star <star.z...@intel.com> > >>> Cc: Yao, Jiewen <jiewen....@intel.com>; Ni, Ruiyu > >>> <ruiyu...@intel.com>; edk2-devel@lists.01.org; Dong, Eric > >>> <eric.d...@intel.com>; leif.lindh...@linaro.org > >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't > >>> ASSERT on 'BootNext' varname > >>> > >>>> On 4 October 2017 at 14:49, Zeng, Star <star.z...@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 > >>> > >> > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
