From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0F62D21F3C18B for ; Tue, 10 Oct 2017 02:09:06 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP; 10 Oct 2017 02:12:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,504,1500966000"; d="scan'208";a="908460368" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by FMSMGA003.fm.intel.com with ESMTP; 10 Oct 2017 02:12:32 -0700 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 10 Oct 2017 02:12:31 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 10 Oct 2017 02:12:30 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.152]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Tue, 10 Oct 2017 17:12:29 +0800 From: "Ni, Ruiyu" To: "Yao, Jiewen" , Laszlo Ersek CC: "Zeng, Star" , Ard Biesheuvel , "Dong, Eric" , "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" Thread-Topic: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname Thread-Index: AQHTPKM4IQtsISoF1kelo6reT32Ix6LSSAiAgAAFEACAAOKYgIAAAX8AgAAEMwCAAA/lgIABEy8AgAAH9gCAABMhAIAIYtjg Date: Tue, 10 Oct 2017 09:12:28 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BA8D171@SHSMSX104.ccr.corp.intel.com> References: <20171003171727.5641-1-ard.biesheuvel@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103B97E276@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A9CE19A@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B97E44C@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B97E486@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B97E6F4@shsmsx102.ccr.corp.intel.com>, <74e4651f-f245-51fa-ee49-4f547a9a929d@redhat.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Oct 2017 09:09:07 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable 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 > Cc: Zeng, Star ; Ard Biesheuvel > ; Ni, Ruiyu ; Dong, Eric > ; edk2-devel@lists.01.org; leif.lindholm@linaro.org > Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT > on 'BootNext' varname >=20 > Thank you star. >=20 > I ack this update. >=20 > I recall we did a review for bds on variable usage and assert usage and f= ixed such > issue. If this is a regression, we probable need review again. >=20 > thank you! > Yao, Jiewen >=20 >=20 > > =1B$B:_=1B(B 2017=1B$BG/=1B(B10=1B$B7n=1B(B5=1B$BF|!$2<8a=1B(B3:59=1B$B= !$=1B(BLaszlo Ersek =1B$B > > >> 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 shou= ld 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 =3D0000. > >> 3. Reboot and then ASSERT. > >> ASSERT!: [BdsDxe] > >> i:\git\edk2git\edk2\MdeModulePkg\Library\UefiBootManagerLib\BmMisc.c > >> (423): ((BOOLEAN)(0=3D=3D1)) > >> > >> 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 ; Ard Biesheuvel > >> > >> Cc: Ni, Ruiyu ; Dong, Eric ; > >> edk2-devel@lists.01.org; leif.lindholm@linaro.org; Yao, Jiewen > >> > >> 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 R= uiyu > 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 r= eturn > FALSE. > >> > >> Perhaps it's relevant: the function was made public in commit 3dc5c1ae= 5c757. > >> > >> Thanks > >> Laszlo > >> > >>> -----Original Message----- > >>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >>> Sent: Wednesday, October 4, 2017 9:54 PM > >>> To: Zeng, Star > >>> Cc: Yao, Jiewen ; Ni, Ruiyu > >>> ; edk2-devel@lists.01.org; Dong, Eric > >>> ; 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 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 =1B$B!H=1B(BTable 13. Global Var= iables=1B$B!I=1B(B > >>>> if the VendorGuid is gEfiGlobalVariableGuid. > >>>> > >>>> > >>>> > >>>> I would suspect there is a bug at other place if the code ends up > >>>> calling this function(EfiBootManagerIsValidLoadOptionVariableName) o= n > L"BootNext". > >>>> > >>> > >>> That still does not mean you should ASSERT() here. The state of the v= ariable > store !=3D the internals of the code, and so it should be considered exte= rnal 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 gu= ess > others may run into it as well. > >>> > >>>> I may want to wait for Ruiyu=1B$B!G=1B(Bs 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 > >>> > >> > >