From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp02.apple.com (ma1-aaemail-dr-lapp02.apple.com [17.171.2.68]) by mx.groups.io with SMTP id smtpd.web12.2156.1581888341612098792 for ; Sun, 16 Feb 2020 13:25:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=WxjIIJo/; spf=pass (domain: apple.com, ip: 17.171.2.68, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp02.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp02.apple.com (8.16.0.27/8.16.0.27) with SMTP id 01GL78ed057362; Sun, 16 Feb 2020 13:25:40 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=3kIEI76fuhwsc/ElLLKT7VjSMWXi3bObZpuSk11+V44=; b=WxjIIJo/leR4zRwc2xXqU2oF1tKhlh9gEAOvlp03wWOak2ADAH08o2wmSat0F3hlf48Q +vVn6TQWbpRAXSjp+CCS9iAWOzONvc+9lEcR8PU38sudI+7V2mLhVlcTC+qDnBnX86eD 0/uqWjgOYtY3nnVlYQMYF1Oe89XwK9cBYatncgX2BlFIya826aGnFuMgUErKcVcKpp1Y 2P2hLMxM77b888MV+bIGiIPu9LTTey4w5gZzQYA0MmYvCPgajqA4YNHtOwMWh+4PSmoy ZPeeVulAinPVAkY0hOuV9PWbGQJgOkb3nPHSSul3lTG3zqLvaDpT5r9q7PuooQ4CjJ3p lA== Received: from rn-mailsvcp-mta-lapp03.rno.apple.com (rn-mailsvcp-mta-lapp03.rno.apple.com [10.225.203.151]) by ma1-aaemail-dr-lapp02.apple.com with ESMTP id 2y6e6uwysm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Sun, 16 Feb 2020 13:25:39 -0800 Received: from rn-mailsvcp-mmp-lapp03.rno.apple.com (rn-mailsvcp-mmp-lapp03.rno.apple.com [17.179.253.16]) by rn-mailsvcp-mta-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) with ESMTPS id <0Q5T00DAPCUR7D60@rn-mailsvcp-mta-lapp03.rno.apple.com>; Sun, 16 Feb 2020 13:25:39 -0800 (PST) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp03.rno.apple.com by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) id <0Q5T00L00CN2R100@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Sun, 16 Feb 2020 13:25:39 -0800 (PST) X-Va-A: X-Va-T-CD: e48e8dc3f6c377b8dc939b4126ad19f3 X-Va-E-CD: ce6e59d19e41f9f978af9e8b590c623d X-Va-R-CD: 4ad19a74e8868beceae55112f7f03bd9 X-Va-CD: 0 X-Va-ID: 115e6a32-a011-47d5-b834-5b7beed329be X-V-A: X-V-T-CD: e48e8dc3f6c377b8dc939b4126ad19f3 X-V-E-CD: ce6e59d19e41f9f978af9e8b590c623d X-V-R-CD: 4ad19a74e8868beceae55112f7f03bd9 X-V-CD: 0 X-V-ID: a0f2fe47-7a8f-4445-ba13-792f8c1577aa X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572 definitions=2020-02-16_05:2020-02-14,2020-02-16 signatures=0 Received: from [17.235.36.21] by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) with ESMTPSA id <0Q5T003KKCUOYI20@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Sun, 16 Feb 2020 13:25:38 -0800 (PST) Sender: afish@apple.com From: "Andrew Fish" Message-id: <3D7EB7E9-9B94-40AD-A0E9-BFDEB787F6DA@apple.com> MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions Date: Sun, 16 Feb 2020 13:25:36 -0800 In-reply-to: Cc: vit9696 , "Gao, Liming" , "Gao, Zhichao" , =?utf-8?Q?Marvin_H=C3=A4user?= , Laszlo Ersek To: devel@edk2.groups.io, Mike Kinney References: <20200103171242.63839-1-vit9696@protonmail.com> <492A300B-1CED-4FF0-98F8-20D1E116DCC0@protonmail.com> <2719704F-6DB4-444F-97FE-DBF71D39B698@apple.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2020-02-16_05:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_A2F7E4D2-11B4-428A-9895-A2C92B7C6C65" --Apple-Mail=_A2F7E4D2-11B4-428A-9895-A2C92B7C6C65 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Mike, Sorry I don't think I totally understood what felt wrong to me, so I did a= bad job explaining my concerns. Also I don't think I was thinking enough i= n the context of the C11 StdLib. I think my concern is still the global scope of the constraint, even if we= tune it per module. For example the DXE Core can interact with PE/COFF ima= ges and other data that could have indirectly come from the disk. So concep= tually you may want to ASSERT on some constraints and not on others. I thin= k that is why I ratholed on expanding the error handling macros as that was= more in the vein of having the caller deal with it, so that is kind of lik= e what you would do pre C11. Also the DebugLib is probably one of the MdePk= g Libs that has the most instances floating around, so I think we should ch= ange it in a non backwards way very carefully.=20 So after reading up on the C11 implementation of Constraints I think my al= ternate proposal is for us to add a ConstraintLib modeled after C11 vs. upd= ating the DebugLib. This would solve the 2 things that made me uncomfortabl= e: 1) Extending the DebugLib API; 2) Giving the caller control of the ASSER= T behavior. It would still have the down side of breaking builds as the Bas= eLib would get a new dependency, so we could talk about adding these functi= ons to the DebugLib as the cost of replicating code.=20 C11 defines constraint_handler_t and set_constraint_handler_s as a way for= the caller to configure the behavior for bounds checked functions. I think= that is the behavior I prefer. So if we are going to make a change that im= pacts DebugLib compatibility I just want to make sure we have a conversatio= n about all the options. My primary goal is we have the conversation, and i= f folks don't agree with me that is fine at least we talked about it. What I'm thinking about is as simply exposing an API to control the Constr= aint handler like C11. This could be done via an ConstrainLib or extending = the DebugLib.=20 The basic implementation of the lib would look like: typedef VOID (EFIAPI *CONSTRAINT_HANDLER) ( IN CONST CHAR8 *FileName, IN UINTN LineNumber, IN CONST CHAR8 *Description, IN EFI_STATUS Status ); // Default to AssertConstraintHandler to make it easier to implement Base = and XIP libs. // We could have a PCD that also sets the default handler in a Lib Constru= ctor. The default handler is implementation defined in C11.=20 CONSTRAINT_HANDLER gDefaultConstraintHandler =3D AssertConstraintHandler; CONSTRAINT_HANDLER gActiveConstraintHandler =3D gDefaultConstraintHandler; BOOLEAN EFIAPI ConstraintAssertEnabled ( VOID ) { return (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_= CONSTRAINT_ENABLED) !=3D 0);=20 } EFI_STATUS EFIAPI SetConstraintHandler ( IN CONSTRAINT_HANDLER Handler ) {=20 if (Handler =3D=3D NULL) { gActiveConstraintHandler =3D gDefaultConstraintHandler; } else { gActiveConstraintHandler =3D Handler; } } VOID AssertConstraintHandler ( IN CONST CHAR8 *FileName, IN UINTN LineNumber, IN CONST CHAR8 *Description, IN EFI_STATUS Status ) { if (ConstraintAssertEnabled ()) { DEBUG ((EFI_D_ERROR, "\Constraint ASSERT (Status =3D %r): ", Status))= ; DebugAssert (FileName, LineNumber, Description) } return; } VOID IgnoreConstraintHandler ( IN CONST CHAR8 *FileName, IN UINTN LineNumber, IN CONST CHAR8 *Description, IN EFI_STATUS Status ) { return; } We could add macros for the code in the lib to call: #define CONSTRAINT_CHECK(Expression, Status) \ do { \ if (!(Expression)) { \ gActiveConstraintHandler (__FILE__, __LINE__, Expression, Status); \ return Status; \ } \ } while (FALSE) #define CONSTRAINT_REQUIRE(Expression, Status, Label) \ do { \ if (!(Expression)) { \ gActiveConstraintHandler (__FILE__, __LINE__, Expression, Status); \ goto Label; \ } \ } while (FALSE) As a caller we have now have control: EFI_STATUS Status; CHAR16 Dst[2]; SetConstraintHandler (IgnoreConstraintHandler); Status =3D StrCpyS (Dst, sizeof (Dst), L"Too Long"); Print (L"Dst =3D%s (%r)\n", Dst, Status);=20 SetConstraintHandler (AssertConstraintHandler); StrCpyS (Dst, sizeof (Dst), L"Too Long"); Thanks, Andrew Fish PS Since I'm on a crazy idea roll another idea would be to add a MdePkgCom= patability.dsc.inc file that could be used to future proof adding dependent= libs to existing MdePkg libs. So a platform could include this .DSC and th= at would give them the default library mapping to keep code compiling. It w= ill only work after other platforms start including it, but after that it w= ould give default mappings for dependent libs.=20 In our above example we could have added this and then existing builds tha= t included MdePkgCompatability.dsc.inc would keep compiling.=20 [LibraryClasses] DebugConstraintLib|MdePkg/Library/DebugConstraintLib/DebugConstraintLib.= inf > On Feb 15, 2020, at 11:38 AM, Michael D Kinney wrote: >=20 > Andrew, > > I do not think of this as a globally scoped PCD. It can be set in globa= l scope in DSC. But is can also be set to values based on module type or f= or specific modules. In the case of the safe string functions, I think the= re is a desire to disable the constraint asserts when building a UEFI App o= r UEFI Driver and implement those modules to handle the error return values= . Enabling the constraint asserts for PEI Core, DXE Core, SMM/MM Core, PEI= M, DXE, SMM/MM modules makes sense to find incorrect input to these functio= ns from modules that can guarantee the inputs would never return an error a= nd catch these as part of dev/debug/validation builds. > > I would not expect disabling on a module by module basis to be common. > > I think the rule for API implementations is to only use CONSTRAINT_ASSER= T() for conditions that are also checked and return an error or fail with p= redicable behavior that allows the system to continue to function. ASSERT(= ) is for conditions that the system can notcontinue. > > Best regards, > > Mike > > From: afish@apple.com >=20 > Sent: Friday, February 14, 2020 10:27 PM > To: vit9696 > > Cc: devel@edk2.groups.io ; Kinney, Michael = D >; Gao, Li= ming >; Gao, Zhichao >; Marvin H=C3=A4user >; Laszlo Ersek= > > Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string = constraint assertions > > Vitaly, > > Sorry after I sent the mail I realized it may come across as me asking = you to do work and that was not my intent.=20 > > I will point out though that a non backward compatible change to somethi= ng as fundamental as the DebugLib is a very big deal. I've got a few differ= ent custom implementations that would break with this change as Mike propos= ed. Given breaking every one's debug lib is such a big deal maybe it is som= ething that we should do as a long term plan vs. some incremental fix. So m= y intent was to start a conversation about what else we might want to chang= e if we are going to break the world. The only think worse than breaking th= e world is breaking the world frequently.=20 > > I'm also a little worried that we are taking things that are today local= ly scoped like SAFE_STRING_CONSTRAINT_CHECK and SAFE_PRINT_CONSTRAINT_CHECK= and making them global constructs. I think the way others have dealt with = things like this is to make them be DEBUG prints vs. ASSERTs. Also even som= ething as simple as SAFE_STRING_CONSTRAINT_CHECK could be called from code = that wants ASSERT and CONSTRAINT_ASSERT behavior. It is not clear to me tha= t the low level code knows the right thing to do in a global sense even if = there is a PCD. It almost seems like we should have wrappers for the Safe = string functions that implement the behavior you want as a caller. I'm not = sure about that, but it seems like it is worth talking about?=20 > > Thanks, > > Andrew Fish >=20 >=20 > On Feb 14, 2020, at 7:31 PM, vit9696 > wrote: > > Hi Andrew, > > While your suggestions look interesting, I am afraid they are not partic= ularly what we want to cover with this discussion at the moment. > > Making assertions go through DEBUG printing functions/macros is what we = have to strongly disagree about. Assertions and debug prints are separate t= hings configurable by separate PCDs. We should not mix them. Introducing co= nstraint assertions is a logical step forward in understanding that differe= nt software works in different environments. > There are normal, or, as I call them, invariant assertions (e.g. precond= itions), for places where the function cannot work unless the assertion is = satisfied. This is where we ASSERT. > There are constraint assertions, which signalise that bad data came thro= ugh the function, even though the function was called from a trusted source= . This is where we call CONSTRAINT_ASSERT. > The thing we need is to have the latter separable and configurable, beca= use not every piece of software works in a trusted environment. Other than = that, constraint assertions, when enabled, are not anyhow different from no= rmal assertions in the sense of action taken. Assertions have configurable = breakpoints and deadloops, and DEBUG prints go through a different route in= DebugLib that may cause entirely different effects. For example, we halt e= xecution upon printing to DEBUG_ERROR in our DebugLib even in release build= s. > > =3DTo make it clear, currently I plan to add the following interface: > > #define CONSTRAINT_ASSERT(Expression) \ > do { \ > if (DebugConstraintAssertEnabled ()) { \ > if (!(Expression)) { \ > _ASSERT (Expression); \ > ANALYZER_UNREACHABLE (); \ > } \ > } \ > } while (FALSE) > > with DebugConstraintAssertEnabled implemented as=20 > > (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_ASSERT_= ENABLED | DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED) =3D=3D DEBUG_PROPERTY_D= EBUG_ASSERT_ENABLED) > > Your suggestion with require macros looks interesting indeed, but I beli= eve it is in fact parallel to this discussion. The change we discuss introd= uces a new assertion primitive =E2=80=94 constraint assertions, while REQUI= RE macros are mostly about advanced syntax sugar and higher level assertion= primitives on top of existing ones. Perhaps we can have this and make a go= od use of it, especially given that it brought some practical benefit in Ap= ple, but I would rather discuss this later once constraint assertions are m= erged into EDK II tree. > > Best wishes, > Vitaly > > On Sat, Feb 15, 2020 at 03:02, Andrew Fish > wrote: > >=20 >=20 > On Feb 14, 2020, at 2:50 PM, Michael D Kinney > wrote: > > Hi Vitaly, > > I agree that this proposal makes a lot of sense. We recently added a ne= w assert type called STATIC_ASSERT() for assert conditions that can be test= ed at build time. > > A new assert type for checks that can be removed and the API still guara= ntees that it fails gracefully with a proper return code is a good idea. G= iven we have STATIC_ASSERT(), how about naming the new macro CONSTRAINT_ASS= ERT()? > > We also want the default to be enabled. The current use of bit 0x40 in = PcdDebugPropertyMask is always clear, so we want the asserts enabled when = 0x40 is clear. We can change the name of the define bit to DEBUG_PROPERTY_= CONTRAINT_ASSERT_DISABLED so bit 0x40 needs to be set in PcdDebugPropertyMa= sk to disable these types of asserts. > > This approach does require all the DebugLib implementations to be update= d with the new DebugConstraintAssertDisabled() API. > > > Mike, > > If you wanted to be backward compatible you could just use DebugAssertEn= abled () but in place of _ASSERT() use _CONSTRAINT_ASSERT > > #define _ASSERT(Expression) DebugAssert (__FILE__, __LINE__, #Expressio= n) > > #define _CONSTRAINT_ASSERT(Expression) DebugPrint (DEBUG_ERROR, "ASSER= T %a(%d): %a\n",, __FILE__, __LINE__, #Expression) > > Not as elegant as the non backward compatible change, but I thought I'd = throw it out there.=20 > > There are some ancient Apple C ASSERT macros [AssertMacros.h] that also= have the concept of require. Require includes an exception label (goto lab= el). It is like a CONSTRAINT_ASSERT() but with the label. On release builds= the DEBUG prints are skipped.=20 > > So we could do something like: > > EFI_STATUS Status =3D EFI_INVALID_PARAMETER; > > REQUIRE(Arg1 !=3D NULL, ErrorExit); > REQUIRE(Arg2 !=3D NULL, ErrorExit); > REQUIRE(Arg3 !=3D NULL, ErrorExit); > > ErrorExit: > return Status; > > There is another form that allows an ACTION (a statement to execute. So = you could have: > > EFI_STATUS Status; > > REQUIRE_ACTION(Arg1 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAM= ETER); > REQUIRE_ACTION(Arg2 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAM= ETER); > REQUIRE_ACTION(Arg3 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAM= ETER); > > ErrorExit: > return Status; > > If you use CONSTRAINT_ASSERT(); > > if (Arg1 =3D=3D NULL || Arg2 =3D=3D NULL || Arg3 =3D=3D NULL) { > CONSTRAINT_ASSERT (Arg1 !=3D NULL); > CONSTRAINT_ASSERT (Arg2 !=3D NULL); > CONSTRAINT_ASSERT (Arg3 !=3D NULL); > return EFI_INVALID_PARAMETER; > } > > I'd note error processing args on entry is the simplest case. In a more= complex case when cleanup is required the goto label is more useful.=20 > > I guess we could argue for less typing and more symmetry and say use ASS= ERT, CONSTRAINT, and REQUIRE. I guess you could add an ASSERT_ACTION too.= =20 > > The AssertMacros.h versions also support _quiet (skip the print) and _st= ring (add a string to the print) so you end up with: > REQUIRE > REQUIRE_STRING > REQUIRE_QUIET > REQUIRE_ACTION > REQUIRE_ACTION_STRING > REQUIRE_ACTION_QUIET > > We could also end up with=20 > CONSTRAINT > CONSTRAINT_STRING > CONSTRAINT_QUIET > > I think the main idea behind _QUIET is you can silence things that are t= oo noisy, and you can easily make noise things show up by removing the _QUI= ET to debug.=20 > > I'd thought I throw out the other forms for folks to think about. I'm pr= obably biased as I used to looking at code and seeing things like require_a= ction_string(Arg1 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMETER, "= 1st Arg1 check"); > > Thanks, > > Andrew Fish > > PS The old debug macros had 2 versions of CONSTRAINT check and verify. T= he check version was compiled out on a release build, the verify version al= ways does the check and just skips the DEBUG print.=20 > > Mike > > > > From: vit9696 >= =20 > Sent: Friday, February 14, 2020 9:38 AM > To: Kinney, Michael D > > Cc: devel@edk2.groups.io ; Gao, Liming >; Gao, Zhichao >; Marvin H=C3=A4user >; Laszlo Ersek > > Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string = constraint assertions > > Michael, > > Generalising the approach makes good sense to me, but we need to make an= obvious distinguishable difference between: > - precondition and invariant assertions (i.e. assertions, where function= will NOT work if they are violated) > - constraint asserts (i.e. assertions, which allow us to spot unintentio= nal behaviour when parsing untrusted data, but which do not break function = behaviour). > > As we want to use this outside of SafeString, I suggest the following: > - Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 bit for PcdDeb= ugPropertyMask instead of PcdAssertOnSafeStringConstraints. > - Introduce DebugAssertConstraintEnabled DebugLib function to check for = DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED. > - Introduce ASSERT_CONSTRAINT macro, that will assert only if DebugConst= raintAssertEnabled returns true. > - Change SafeString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to ASSERT_CO= NSTRAINTs. > - Use ASSERT_CONSTRAINT in other places where necessary. > >=20 > I believe this way lines best with EDK II design. If there are no object= ions, I can submit the patch in the beginning of next week. > >=20 > Best wishes, > Vitaly > >=20 > 14 =D1=84=D0=B5=D0=B2=D1=80. 2020 =D0=B3., =D0=B2 20:00, Kinney, Michael= D > =D0=BD= =D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): > > Vitaly, > > I want to make sure a feature PCD can be used to disable ASSERT() behavi= or in more than just safe string functions in BaseLib. > > Can we consider changing the name and description of PcdAssertOnSafeStri= ngConstraints to be more generic, so if we find other lib APIs, the name wi= ll make sense? > > Maybe something like: PcdEnableLibraryAssertChecks? Default is TRUE. C= an set to FALSE in DSC file to disable ASSERT() checks. > > Thanks, > > Mike > > > > From: devel@edk2.groups.io > On Behalf Of Vitaly Cheptsov via Grou= ps.Io > Sent: Friday, February 14, 2020 3:55 AM > To: Kinney, Michael D >; Gao, Liming >; Gao, Zhichao >;= devel@edk2.groups.io > Cc: Marvin H=C3=A4user >; Laszlo Ersek = > > Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string = constraint assertions > > Replying as per Liming's request for this to be merged into edk2-stable2= 02002. > > On Mon, Feb 10, 2020 at 14:12, vit9696 > wrote: > Hello, >=20 > It has been quite some time since we submitted the patch with so far no = negative response. As I mentioned previously, my team will strongly benefit= from its landing in EDK II mainline. Since it does not add any regressions= and can be viewed as a feature implementation for the rest of EDK II users= , I request this to be merged upstream in edk2-stable202002. >=20 > Best wishes, > Vitaly >=20 > > 27 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 12:47, vit9696 > =D0=BD=D0=B0=D0=BF=D0=B8=D1= =81=D0=B0=D0=BB(=D0=B0): > >=20 > >=20 > > Hi Mike, > >=20 > > Any progress with this? We would really benefit from this landing in t= he next stable release. > >=20 > > Best, > > Vitaly > >=20 > >> 8 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 19:35, Kinney, Michael D <= michael.d.kinney@intel.com > =D0=BD=D0= =B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): > >>=20 > >>=20 > >> Hi Vitaly, > >>=20 > >> Thanks for the additional background. I would like > >> a couple extra day to review the PCD name and the places > >> the PCD might potentially be used. > >>=20 > >> If we find other APIs where ASSERT() behavior is only > >> valuable during dev/debug to quickly identify misuse > >> with trusted data and the API provides predicable > >> return behavior when ASSERT() is disabled, then I would > >> like to have a pattern we can potentially apply to all > >> these APIs across all packages. > >>=20 > >> Thanks, > >>=20 > >> Mike > >>=20 > >>> -----Original Message----- > >>> From: devel@edk2.groups.io > On > >>> Behalf Of Vitaly Cheptsov via Groups.Io > >>> Sent: Monday, January 6, 2020 10:44 AM > >>> To: Kinney, Michael D > > >>> Cc: devel@edk2.groups.io > >>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to > >>> disable safe string constraint assertions > >>>=20 > >>> Hi Mike, > >>>=20 > >>> Yes, the primary use case is for UEFI Applications. We > >>> do not want to disable ASSERT=E2=80=99s completely, as > >>> assertions that make sense, i.e. the ones signalising > >>> about interface misuse, are helpful for debugging. > >>>=20 > >>> I have already explained in the BZ that basically all > >>> safe string constraint assertions make no sense for > >>> handling untrusted data. We find this use case very > >>> logical, as these functions behave properly with > >>> assertions disabled and cover all these error > >>> conditions by the return statuses. In such situation is > >>> not useful for these functions to assert, as we end up > >>> inefficiently reimplementing the logic. I would have > >>> liked the approach of discussing the interfaces > >>> individually, but I struggle to find any that makes > >>> sense from this point of view. > >>>=20 > >>> AsciiStrToGuid will ASSERT when the length of the > >>> passed string is odd. Functions that cannot, ahem, > >>> parse, for us are pretty much useless. > >>> AsciiStrCatS will ASSERT when the appended string does > >>> not fit the buffer. For us this logic makes this > >>> function pretty much equivalent to deprecated and thus > >>> unavailable AsciiStrCat, except it is also slower. > >>>=20 > >>> My original suggestion was to remove the assertions > >>> entirely, but several people here said that they use > >>> them to verify usage errors when handling trusted data. > >>> This makes good sense to me, so we suggest to support > >>> both cases by introducing a PCD in this patch. > >>>=20 > >>> Best wishes, > >>> Vitaly > >>>=20 > >>>> 6 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 21:28, Kinney, Michael D > >>> > =D0= = =BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): > >>>>=20 > >>>>=20 > >>>> Hi Vitaly, > >>>>=20 > >>>> Is the use case for UEFI Applications? > >>>>=20 > >>>> There is a different mechanism to disable all > >>> ASSERT() > >>>> statements within a UEFI Application. > >>>>=20 > >>>> If a component is consuming data from an untrusted > >>> source, > >>>> then that component is required to verify the > >>> untrusted > >>>> data before passing it to a function that clearly > >>> documents > >>>> is input requirements. If this approach is followed, > >>> then > >>>> the BaseLib functions can be used "as is" as long as > >>> the > >>>> ASSERT() conditions are verified before calling. > >>>>=20 > >>>> If there are some APIs that currently document their > >>> ASSERT() > >>>> behavior and we think that ASSERT() behavior is > >>> incorrect and > >>>> should be handled by an existing error return value, > >>> then we > >>>> should discuss each of those APIs individually. > >>>>=20 > >>>> Mike > >>>>=20 > >>>>=20 > >>>>> -----Original Message----- > >>>>> From: devel@edk2.groups.io > On > >>>>> Behalf Of Vitaly Cheptsov via Groups.Io > >>>>> Sent: Friday, January 3, 2020 9:13 AM > >>>>> To: devel@edk2.groups.io > >>>>> Subject: [edk2-devel] [PATCH v3 0/1] Add PCD to > >>> disable > >>>>> safe string constraint assertions > >>>>>=20 > >>>>> REF: > >>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054 > >>>>>=20 > >>>>> Requesting for merge in edk2-stable202002. > >>>>>=20 > >>>>> Changes since V1: > >>>>> - Enable assertions by default to preserve the > >>> original > >>>>> behaviour > >>>>> - Fix bugzilla reference link > >>>>> - Update documentation in BaseLib.h > >>>>>=20 > >>>>> Vitaly Cheptsov (1): > >>>>> MdePkg: Add PCD to disable safe string constraint > >>>>> assertions > >>>>>=20 > >>>>> MdePkg/MdePkg.dec | 6 ++ > >>>>> MdePkg/Library/BaseLib/BaseLib.inf | 11 +-- > >>>>> MdePkg/Include/Library/BaseLib.h | 74 > >>>>> +++++++++++++------- > >>>>> MdePkg/Library/BaseLib/SafeString.c | 4 +- > >>>>> MdePkg/MdePkg.uni | 6 ++ > >>>>> 5 files changed, 71 insertions(+), 30 deletions(-) > >>>>>=20 > >>>>> -- > >>>>> 2.21.0 (Apple Git-122.2) > >>>>>=20 > >>>>>=20 > >>>>> -=3D-=3D-=3D-=3D-=3D-=3D > >>>>> Groups.io Links: You receive all messages sent= to > >>> this > >>>>> group. > >>>>>=20 > >>>>> View/Reply Online (#52837): > >>>>> https://edk2.groups.io/g/devel/message/52837 > >>>>> Mute This Topic: > >>> https://groups.io/mt/69401948/1643496 > >>>>> Group Owner: devel+owner@edk2.groups.io > >>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub > >>>>> [michael.d.kinney@intel.com ] > >>>>> -=3D-=3D-=3D-=3D-=3D-=3D > >>>>=20 > >>>=20 > >>>=20 > >>>=20 > >>=20 > >=20 >=20 > > > > > > > >=20 --Apple-Mail=_A2F7E4D2-11B4-428A-9895-A2C92B7C6C65 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Mike,

Sorry I don't think I totally understood w= hat felt wrong to me, so I did a bad job explaining my concerns. Also I don= 't think I was thinking enough in the context of the C11 StdLib.

I think my concern is still= the global scope of the constraint, even if we tune it per module. For exa= mple the DXE Core can interact with PE/COFF images and other data that coul= d have indirectly come from the disk. So conceptually you may want to ASSER= T on some constraints and not on others. I think that is why I ratholed on = expanding the error handling macros as that was more in the vein of having = the caller deal with it, so that is kind of like what you would do pre C11.= Also the DebugLib is probably one of the MdePkg Libs that has the most ins= tances floating around, so I think we should change it in a non backwards w= ay very carefully. 

So after reading up on the C11 implementation of Constraints I thin= k my alternate proposal is for us to add a ConstraintLib modeled after C11 = vs. updating the DebugLib. This would solve the 2 things that made me uncom= fortable: 1) Extending the DebugLib API; 2) Giving the caller control of th= e ASSERT behavior. It would still have the down side of breaking builds as = the BaseLib would get a new dependency, so we could talk about adding these= functions to the DebugLib as the cost of replicating code. 

C11 defines constrain= t_handler_t and set_constraint_handler_s as a way for the caller to co= nfigure the behavior for bounds checked functions. I think that is the beha= vior I prefer. So if we are going to make a change that impacts DebugLib co= mpatibility I just want to make sure we have a conversation about all the o= ptions. My primary goal is we have the conversation, and if folks don't agr= ee with me that is fine at least we talked about it.
<= br class=3D"">
What I'm thinking about is as simply ex= posing an API to control the Constraint handler like C11. This could be don= e via an ConstrainLib or extending the DebugLib. 

The basic implementation of the lib w= ould look like:

t= ypedef
VOID
(EFIAPI *CONSTRAINT_H= ANDLER) (
  IN CONST CHAR8  = *FileName,
  IN UINTN        =         LineNumber,
  IN CON= ST CHAR8  *Description,
  IN EFI_STATUS &nbs= p;    Status
  );


// Default to AssertConstraintHandler to make it easier to implement= Base and XIP libs.
// We could have a PCD that also s= ets the default handler in a Lib Constructor. The default handler is implem= entation defined in C11. 
CONSTRA= INT_HANDLER gDefaultConstraintHandler =3D AssertConstraintHandler;
CONSTRAINT_HANDLER gActiveConstrain= tHandler =3D gDefaultConstraintHandler;
BOOLEAN
EFIAPI
=
ConstraintAssertEnabled (
  VOID<= /div>
  )
{
&= nbsp; return (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PRO= PERTY_DEBUG_CONSTRAINT_ENABLED) !=3D 0); 
}
=

EFI_STATUS
EFIAPI
SetConstraintHandler (
  IN CONSTRAINT_HANDLER Handler
  )
{ 
  if (Handler =3D=3D = NULL) {
    gActiveConstraintHandler =3D&nbs= p;gDefaultConstraintHandler;
  } else {
    gActiveConstraintHandler =3D Handler;
  }
}

VOID
AssertCon= straintHandler (
  IN CONST CHAR8=  *FileName,
  IN UINTN      =           LineNumber,
 = IN CONST CHAR8  *Description,
  IN EFI_STAT= US      Status
  )
{
  if (ConstraintAssertEn= abled ()) {
     DEBUG ((EFI_D_ERROR, "= \Constraint ASSERT (Status =3D %r): ", Status));
 = ;    DebugAssert (FileName, LineNumber, Description)
  }

 return;
}

VOID
Ig= noreConstraintHandler (
  IN CONS= T CHAR8  *FileName,
  IN UINTN    =             LineNumber,
  IN CONST CHAR8  *Description,
  IN E= FI_STATUS      Status
  )
{
  return;
}

We cou= ld add macros for the code in the lib to call:

#define CONSTRAINT_CHECK(Expr= ession, Status)  \
  do { \
    if (!(Expression)) { \
  &nb= sp;   gActiveConstraintHandler (__FILE__, __LINE__, Expression, S= tatus); \
      return Status; \
<= div class=3D"">    } \
  } while (FALSE= )

#define CONSTRAINT_REQUIRE(Expression, Status, Label)  \
<= div class=3D"">  do { \
    if (!(Expre= ssion)) { \
      gActiveConstraintHand= ler (__FILE__, __LINE__, Expression, Status); \
&= nbsp;     goto Label; \
    }= \
  } while (FALSE)
<= br class=3D"">

As= a caller we have now have control:
  EFI_STATUS = Status;
  CHAR16        Dst[2= ];

  SetCons= traintHandler (IgnoreConstraintHandler);
  Status= =3D StrCpyS (Dst, sizeof (Dst), L"Too Long");
  = Print (L"Dst =3D%s (%r)\n",  Dst, Status); 
=
  SetConstraintHandler (AssertCon= straintHandler);
  StrCpyS (Dst, sizeof (Dst), L"= Too Long");
  

Thanks,

Andrew Fish

<= div class=3D"">PS Since I'm on a crazy idea roll another idea would be to a= dd a MdePkgCompatability.dsc.inc file that could be used to future proof ad= ding dependent libs to existing MdePkg libs. So a platform could include th= is .DSC and that would give them the default library mapping to keep code c= ompiling. It will only work after other platforms start including it, but a= fter that it would give default mappings for dependent libs. 

In our above example we c= ould have added this and then existing builds that included MdePkgCompatabi= lity.dsc.inc would keep compiling. 

 [LibraryClasses]
  = DebugConstraintLib|MdePkg/Library/DebugConstraintLib/DebugConstraintLib.inf=


<= /div>

On Feb 15, 2020, at 11:38 AM, Michael D Kinney <<= a href=3D"mailto:michael.d.kinney@intel.com" class=3D"">michael.d.kinney@in= tel.com> wrote:

Andrew,<= /o:p>
 
I do not think of this as a globally scoped PCD.  It can be set in gl= obal scope in DSC.  But is can also be set to values based on module ty= pe or for specific modules.  In the case of the safe string functions, = I think there is a desire to disable the constraint asserts when building a= UEFI App or UEFI Driver and implement those modules to handle the error re= turn values. &n= bsp;Enabling the constraint asserts for PEI Core, DXE Core, S= MM/MM Core, PEIM, DXE, SMM/MM modules makes sense to find incorrect input t= o these functions from modules that can guarantee the inputs would never re= turn an error and catch these as part of dev/debug/validation builds.
<= o:p class=3D""> 
I would not expect disabling on a module by module basis to be co= mmon.
 
I think the rule for API implementations is to only= use CONSTRAINT_ASSERT() for conditions that are also checked and return an= error or fail with predicable behavior that allows the system to continue = to function. &n= bsp;ASSERT() is for conditions that the system can notco= ntinue.
 
Best regards,
 
Mike
 
From: afish@apple.com <afish@apple.= com> 
Sent: Friday, February 14, 2020 10:27 PM
To: vit9696 <vit9696@protonmail.com>
C= c: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Marvin H=C3=A4use= r <marvin.haeuser@outlook.com>= ; Laszlo Ersek <lersek@redhat.com>
Subject:&= nbsp;Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string = constraint assertions
 
Vitaly,
 =
Sorry after I sent the mail I realized it may come  across = as me asking you to do work and that was not my intent. 
 
I will point out though that = a non backward compatible change to something as fundamental as the DebugLi= b is a very big deal. I've got a few different custom implementations that = would break with this change as Mike proposed. Given breaking every one's d= ebug lib is such a big deal maybe it is something that we should do as a lo= ng term plan vs. some incremental fix. So my intent was to start a conversa= tion about what else we might want to change if we are going to break the w= orld. The only think worse than breaking the world is breaking the world fr= equently. 
 
= I'm also a little worried that we are taking things that are today locally = scoped like SAFE_STRING_CONSTRAINT_CHECK and SAFE_PRINT_CONSTRAINT_CHE= CK and making them global constructs. I think the way others have dealt wit= h things like this is to make them be DEBUG prints vs. ASSERTs. Also even s= omething as simple as SAFE_STRING_CONSTRAINT_CHECK could be called from cod= e that wants ASSERT and CONSTRAINT_ASSERT behavior. It is not clear to me t= hat the low level code knows the right thing to do in a global sense even i= f there is a PCD.  It almost seems like we should have wrappers for th= e Safe string functions that implement the behavior you want as a caller. I= 'm not sure about that, but it seems like it is worth talking about? <= o:p class=3D"">
 
Thanks,
 
Andrew Fish


On Feb 14, 2020, = at 7:31 PM, vit9696 <vit9696@protonmail.c= om> wrote:
 
Hi Andrew,=
 
While your suggestions look interesting,&= nbsp;I am afraid they are not particularly what we want to cover = with this discussion at the moment.
=
 
Making assertions go through DEBUG printing functions/macros is what = we have to strongly disagree about. Assertions and debug prints are separat= e things configurable by separate PCDs. We should not mix them. I= ntroducing constraint assertions is a logical step forward in understanding= that different software works in different environments.
=
  • T= here are normal, or, as I call them, invariant assertions (e.g. precon= ditions), for places where the function cannot work unless the assertion is= satisfied. This is where we ASSERT.
  • There are constraint assertions, which signalise that bad= data came through the function, even though the function was called from a= trusted source. This is where we call CONSTRAINT_ASSERT.
    • <= /ul>
    The thing we need is to have the latter= separable and configurable, because not every piece of software works in a t= rusted environment. Other than that, constraint assertions, when enabled, a= re not anyhow different from normal assertions in the sense of action taken= . Assertions have configurable breakpoints and deadloops, and DEBUG pr= ints go through a different route in DebugLib that may cause enti= rely different effects. For example, we halt execution upon printing t= o DEBUG_ERROR in our DebugLib even in release builds.=
     
    =
<= div style=3D"margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibr= i, sans-serif;" class=3D"">=3DTo make it clear, currently I plan to add the following in= terface:
<= /div>
 
  #define CONSTRAIN= T_ASSERT(Expression) \
    do { \
      if (DebugConstraintAssertEnabled ()) { \
=
       =  if (!(Expression)) { \
         &n= bsp;_ASSERT (Expression); \
          A= NALYZER_UNREACHABLE (); \
        = } \
     <= /span> } \
    } while (FALSE)
 
with DebugConstraintAssertEnabled implemented= as 
 <= /span>
(BOOLEAN) ((PcdGet8(Pc= dDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED | DEBUG_PROPE= RTY_CONTRAINT_ASSERT_DISABLED) =3D=3D DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED)<= /span>
 
Your suggestion with require macros= looks interesting indeed, but I believe it is in fact parallel to this dis= cussion. The change we discuss introduces a new assertion primitive =E2=80= =94 constraint assertions, while REQUIRE macros are mostly about advanced = syntax sugar and higher level assertion primitives on top of existing ones.= Perhaps we can have this and make a good use of it, especially given that = it brought some practical benefit in Apple, but I would rather discuss this= later once constraint assertions are merged into EDK II tree.<= /div>
 
Best wishes,
Vitaly
=
 
On Sat, Feb 1= 5, 2020 at 03:02, Andrew Fish <afish@apple.com= > wrote:
 


On Feb 14, 2020, at 2:50 PM, Michael D Kinney <michael.d.kinney@intel.com> wrote:
 
=
Hi Vitaly,
 
I agree that this proposal makes a = lot of sense.  We re= cently added a new assert type called STATIC_ASSERT() for assert conditions= that can be tested at build time.
=
 
A new assert type for checks that can be removed and the= API still guarantees that it fails gracefully with a proper return code is= a good idea.  Given= we have STATIC_ASSERT(), how about naming the new macro CONSTRAINT_ASSERT(= )?
 
We also want = the default to be enabled.  = ;The current use of bit 0x40 in PcdDebugPropertyMask  is always clear, so we want t= he asserts enabled when 0x40 is clear.  We can change the name of the define bit to DEBUG_PROPE= RTY_CONTRAINT_ASSERT_DISABLED so bit 0x40 needs to be set in PcdDebugProperty= Mask to disable th= ese types of asserts.
 
This approach does require all the DebugLib implementations to be updated with the new Deb= ugConstraintAssertDisabled() API.
=
 
<= div style=3D"margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibr= i, sans-serif;" class=3D""> 
Mike,
 
If you= wanted to be backward compatible you could just use DebugAssertEnable= d () but in place of _ASSERT() use _CONSTRAINT_ASSERT=
 
#define _ASSERT(Exp= ression)  DebugAssert (__FILE__, __LINE__, #Expression)
 
=
#define _= CONSTRAINT_ASSERT(Expression)  DebugPrint (DEBUG_ERROR,  "ASSERT = %a(%d): %a\n",, __FILE__, __LINE__, #Expression)
 
Not as elegant as the non bac= kward compatible change, but I thought I'd throw it out there. 
 
There are some ancient= Apple C ASSERT macros [AssertMacros.h]  that also have the concept of= require. Require includes an exception label (goto label). It is like= a CONSTRAINT_ASSERT() but with the label. On release builds the DEBUG prin= ts are skipped. 
 
So we could do something like:
 
  EFI_STATUS Status =3D  EFI_INVALID_PARA= METER;
 
  REQUIRE(Arg1 !=3D NULL, ErrorExit);<= /span>
  REQUIRE(Arg2 !=3D NULL, ErrorExit);
  REQUIRE(Arg3 !=3D NULL, Er= rorExit);
 
= ErrorExit:
  return Status;<= /o:p>
 
There is another form that allow= s an ACTION (a statement to execute. So you could have:
 
  EFI_STATUS S= tatus;
 
 = REQUIRE_ACTION(Arg1 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMETER= );
  REQUIRE_ACTION(Arg2 !=3D NULL, ErrorExi= t, Status =3D EFI_INVALID_PARAMETER);
  REQU= IRE_ACTION(Arg3 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMETER);
 
ErrorExit:
  return Status;
=
 
If you use CONSTRAINT_ASSERT();
 
  if (Arg1 =3D=3D NULL = || Arg2 =3D=3D NULL || Arg3 =3D=3D NULL) {
 =  CONSTRAINT_ASSERT (Arg1 !=3D NULL);
 =  CONSTRAINT_ASSERT (Arg2 !=3D NULL);
 =  CONSTRAINT_ASSERT (Arg3 !=3D NULL);
 =  return EFI_INVALID_PARAMETER;
 }
 
I'd note error pro= cessing args on entry is the simplest case.  In a more complex case wh= en cleanup is required the goto label is more useful. =
 
<= div style=3D"margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibr= i, sans-serif;" class=3D"">I guess we could argue for less= typing and more symmetry and say use ASSERT, CONSTRAINT, and REQUIRE. I gu= ess you could add an ASSERT_ACTION too. <= /div>
 
The AssertMacros.h versions also support _qui= et (skip the print) and _string (add a string to the print) so you end up w= ith:
REQUIRE
REQUIRE_ST= RING
REQUIRE_QUIET
REQU= IRE_ACTION
REQUIRE_ACTION_STRING<= /o:p>
REQUIRE_ACTION_QUIET
&n= bsp;
We could also end up with =
CONSTRAINT
CONSTRAINT_STRING
CONSTRAINT_QUIET
=
 
I think the main idea behind _QUIET is you c= an silence things that are too noisy, and you can easily make noise things = show up by removing the _QUIET to debug. =
 
I'd thought I throw out the other forms for = folks to think about. I'm probably biased as I used to looking at code and = seeing things like require_action_string(Arg1 !=3D NULL, ErrorExit, Status = = =3D EFI_INVALID_PARAMETER, "1st Arg1 check");=
 
Thanks,
 
Andrew Fish
 
PS The old debug macros had 2 versions of CONSTRAINT ch= eck and verify. The check version was compiled out on a release build, the = verify version always does the check and just skips the DEBUG print. <= o:p class=3D"">
 
Mike<= o:p class=3D"">
 
 
 
From: vit9696 <vit9696@protonmail.com> 
S= ent: Friday, February= 14, 2020 9:38 AM
To: Kinney, Michael D <michael.d.kinney@intel= .com>
Cc: devel@edk2.groups.io; Gao, Liming <= ;lim= ing.gao@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Marvin H=C3=A4user <marvin.haeuser@outlook.com&= gt;; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-d= evel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions
 
Micha= el,
 <= /span>
Generalising the approach makes good sense to me,= but we need to make an obvious distinguishable difference between:
- precondition and invariant asser= tions (i.e. assertions, where function will NOT work if they are violated)<= o:p class=3D"">
- constraint asserts (i.e. = assertions, which allow us to spot unintentional behaviour when parsing unt= rusted data, but which do not break function behaviour).
 
As we want to use this outside of SafeString,  I sugges= t the following:
- Intro= duce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 bit for PcdDebugProperty= Mask instead of PcdAssertOnSafeStringConstraints.=
- Introduce DebugAssertConstraintEnable= d DebugLib function to check for DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENAB= LED.
- Introduce ASSERT_CON= STRAINT macro, that will assert only if DebugConstraintAssertEnabled&n= bsp;returns true.
- Chang= e SafeString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to ASSERT_CO= NSTRAINTs.
- Use ASSER= T_CONSTRAINT in other places where necessary.

 

I believe this way lines best with EDK II design. If there are no ob= jections, I can submit the patch in the beginning of next week.

 

Best wishes,
Vitaly

 

14 =D1=84=D0=B5=D0= =B2=D1=80. 2020 =D0=B3., =D0=B2 20:00, Kinney, Michael D <michael.d.k= inney@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(= =D0=B0):
 
Vitaly,
 
I want to make sure a feature PCD can be used to disable ASSE= RT() behavior in more than just safe string functions in BaseLib.
 
Can we consider changing the name and descriptio= n of PcdAssertOnSafeStringConstraints to be more generic, so if we find other lib APIs, the nam= e will make sense?
 
Maybe something like: PcdE= nableLibraryAssertChecks?  Default is TRUE. &= nbsp;Can set to FALSE in DSC file to disable ASSERT() checks.=
 
Thanks,<= /o:p>
 
Mike
 =
 <= /o:p>
 
<= div class=3D"">
From: devel@edk2.groups.io <devel@edk2.grou= ps.io> On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, February 14, 2020 3:55= AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhich= ao.gao@intel.com>; = ;devel@edk2.groups.io
Cc:<= span class=3D"apple-converted-space"> Marvin H=C3=A4user <= marvin.haeuser@outlook.com>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disab= le safe string constraint assertions
 
Replying as per Liming's request for this to be merged = into edk= 2-stable202002.
<= /div>
 
On Mon, Feb 10, 2020 at 14:12, vit9696 <vit9696@= protonmail.com> wrote:

Hello,

It ha= s been quite some time since we submitted the patch with so far no negative= response. As I mentioned previously, my team will strongly benefit from it= s landing in EDK II mainline. Since it does not add any regressions and can= be viewed as a feature implementation for the rest of EDK II users, I requ= est this to be merged upstream in edk2-stable202002.

Best wishes,
Vitaly

> = 27 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 12:47, vit9696 <vit9696@proton= mail.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):=
> 
> 
> Hi Mike,
> 
> Any progress with this? We would really = benefit from this landing in the next stable release.
> 
> Best,> Vitaly
> 
>> 8 =D1=8F=D0=BD=D0=B2. 2020 =D0= =B3., =D0=B2 19:35, Kinney, Michael D <michael.d.kinney@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):
>> 
>> 
>> Hi Vitaly,
>> 
>> Thanks for the additional backgr= ound. I would like
>> a couple extra day to review the = PCD name and the places
>> the PCD might potentially be= used.
>> 
>> If we find other APIs where ASSERT() behavior = is only
>> valuable during dev/debug to quickly identif= y misuse
>> with trusted data and the API provides pred= icable
>> return behavior when ASSERT() is disabled, th= en I would
>> like to have a pattern we can potentially= apply to all
>> these APIs across all packages.
>> 
>> Thanks,
>> 
>> Mike
>> 
>>>= ; -----Original Message-----
>>> From: devel@edk2.groups.io <<= span style=3D"color: purple;" class=3D"">devel@edk2.groups.io>= ; On
>>> Behalf Of Vitaly Cheptsov via Groups.Io
>>> Sent: Monday, January 6, 2020 10:44 AM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: devel@edk2.groups.io
>>> Subj= ect: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to
>>> = disable safe string constraint assertions
>>> 
>>> Hi = Mike,
>>> = ;
>>> Yes, the primary use case is for UEFI A= pplications. We
>>> do not want to disable ASSERT=E2= = =80=99s completely, as
>>> assertions that make sen= se, i.e. the ones signalising
>>> about interface mi= suse, are helpful for debugging.
>>> 
>>> I have alrea= dy explained in the BZ that basically all
>>> safe s= tring constraint assertions make no sense for
>>> ha= ndling untrusted data. We find this use case very
>>>= ; logical, as these functions behave properly with
>>&g= t; assertions disabled and cover all these error
>>>= conditions by the return statuses. In such situation is
>= >> not useful for these functions to assert, as we end up
>>> inefficiently reimplementing the logic. I would have
>>> liked the approach of discussing the interfaces
>>> individually, but I struggle to find any that makes>>> sense from this point of view.
>&= gt;> 
&g= t;>> AsciiStrToGuid will ASSERT when the length of the
= >>> passed string is odd. Functions that cannot, ahem,
>>> parse, for us are pretty much useless.
>&g= t;> AsciiStrCatS will ASSERT when the appended string does
>>> not fit the buffer. For us this logic makes this
>>> function pretty much equivalent to deprecated and thus
>>> unavailable AsciiStrCat, except it is also slower.>>> =
>>> My original suggestion was to remove the assert= ions
>>> entirely, but several people here said that= they use
>>> them to verify usage errors when handl= ing trusted data.
>>> This makes good sense to me, s= o we suggest to support
>>> both cases by introducin= g a PCD in this patch.
>>> 
>>> Best wishes,
>>> Vitaly
>>> 
>>>> 6 =D1=8F=D0=BD= = =D0=B2. 2020 =D0=B3., =D0=B2 21:28, Kinney, Michael D
>&g= t;> <michael.d.kinney@intel.com> =D0=BD=D0=B0=D0=BF= =D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):
>>>> 
>>>> 
>>>= > Hi Vitaly,
>>>> 
>>>> Is the use case for U= EFI Applications?
>>>> 
>>>> There is a differen= t mechanism to disable all
>>> ASSERT()
>>>> statements within a UEFI Application.
>= >>> 
>>>> If a component is consuming data from an untrusted
>>> source,
>>>> then that com= ponent is required to verify the
>>> untrusted
>>>> data before passing it to a function that clearly=
>>> documents
>>>> is inp= ut requirements. If this approach is followed,
>>> t= hen
>>>> the BaseLib functions can be used "as is= " as long as
>>> the
>>>> = ASSERT() conditions are verified before calling.
>>>= > 
>&= gt;>> If there are some APIs that currently document their
>>> ASSERT()
>>>> behavior and we= think that ASSERT() behavior is
>>> incorrect and>>>> should be handled by an existing error return= value,
>>> then we
>>>> s= hould discuss each of those APIs individually.
>>>&g= t; 
>>= ;>> Mike
>>>> 
>>>> 
>>>>> -----Origin= al Message-----
>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On>>>>> Behalf Of Vitaly Cheptsov via Groups.Io>>>>> Sent: Friday, January 3, 2020 9:13 AM
>>>>> To:&n= bsp;devel@edk2.groups.io
>>>>> S= ubject: [edk2-devel] [PATCH v3 0/1] Add PCD to
>>> d= isable
>>>>> safe string constraint assertions=
>>>>>&n= bsp;
>>>>> REF:
>>&= gt;>> https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054>>>>>&nbs= p;
>>>>> Requesting for merge in edk2-s= table202002.
>>>>> 
>>>>> Changes since V1= :
>>>>> - Enable assertions by default to pres= erve the
>>> original
>>>>= > behaviour
>>>>> - Fix bugzilla reference = link
>>>>> - Update documentation in BaseLib.h=
>>>>>&n= bsp;
>>>>> Vitaly Cheptsov (1):
>>>>> MdePkg: Add PCD to disable safe string constrai= nt
>>>>> assertions
>>>= >> 
&= gt;>>>> MdePkg/MdePkg.dec | 6 ++
>>>>= > MdePkg/Library/BaseLib/BaseLib.inf | 11 +--
>>>= >> MdePkg/Include/Library/BaseLib.h | 74
>>>&g= t;> +++++++++++++-------
>>>>> MdePkg/Libra= ry/BaseLib/SafeString.c | 4 +-
>>>>> MdePkg/Md= ePkg.uni | 6 ++
>>>>> 5 files changed, 71 inse= rtions(+), 30 deletions(-)
>>>>> 
>>>>> --=
>>>>> 2.21.0 (Apple Git-122.2)
= >>>>> 
>>>>> =
>>>>> -=3D-=3D-=3D-=3D-=3D-=3D
>>>>> Groups.io Links: You rece= ive all messages sent to
>>> this
>= >>>> group.
>>>>> 
>>>>> View/= Reply Online (#52837):
>>>>> https://edk2.groups.io/g/d= evel/message/52837
>>>>> Mute This = Topic:
>>>&nbs= p;https://groups.io/mt/69401948/1643496
>>>>> Group Owner:&= nbsp;devel+owner@edk2.groups.io
>>&= gt;>> Unsubscribe: = https://edk2.groups.io/g/devel/unsub
>&g= t;>>> [michael.d.kinney@intel.com]
>= >>>> -=3D-=3D-=3D-=3D-=3D-=3D
>>>> 
>>>=  
>>&= gt; 
>&g= t;> 
>= ;> 
>=  

 
 
 <= /div>

<= /p>

 
 =
 
 

--Apple-Mail=_A2F7E4D2-11B4-428A-9895-A2C92B7C6C65--