From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp03.apple.com (ma1-aaemail-dr-lapp03.apple.com [17.171.2.72]) by mx.groups.io with SMTP id smtpd.web10.5381.1581748019894565819 for ; Fri, 14 Feb 2020 22:27:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=aF8NrNDZ; spf=pass (domain: apple.com, ip: 17.171.2.72, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp03.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id 01F6QWNL052764; Fri, 14 Feb 2020 22:26:58 -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=U66a/cioaOCrZFNtz/t2g/u4D6H+O5bQVrE2Z0MqveI=; b=aF8NrNDZxUKcDzAvoWXRAwWTemmxmhiQTvrN9tiwwnBHIF5N+wbmsL2ixy1mHntGbcJ7 xHkjV+fQkzZvRhdd0HGhTq61UdBwlc6BTX+hWODPadNiiFaS9NG/1DQ5gVewKM8rHl0U yAbd/kvq9zNhp1O7eGe4ap9JnhsxWopFcm6uSHBczh7hR3OQzqbjUDz/XxrPjZtleIKP 8pizsp8g7vOwiT8dOdZXNgIJL4jAsjDhwMCIQOzqwOSm7QYQ12HO5+bS0XRmwppwTwpD HvP+gtwcvvMmZYoPB8Gu/dc9CeceWvXCVrfvB/qIOf1BLQtveE1WMwLsnNx1cyh8gBCM yg== Received: from rn-mailsvcp-mta-lapp03.rno.apple.com (rn-mailsvcp-mta-lapp03.rno.apple.com [10.225.203.151]) by ma1-aaemail-dr-lapp03.apple.com with ESMTP id 2y5bd5gd10-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Fri, 14 Feb 2020 22:26:57 -0800 Received: from rn-mailsvcp-mmp-lapp02.rno.apple.com (rn-mailsvcp-mmp-lapp02.rno.apple.com [17.179.253.15]) 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 <0Q5Q00AJLCKXJX90@rn-mailsvcp-mta-lapp03.rno.apple.com>; Fri, 14 Feb 2020 22:26:57 -0800 (PST) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp02.rno.apple.com by rn-mailsvcp-mmp-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) id <0Q5Q00700C9OYK00@rn-mailsvcp-mmp-lapp02.rno.apple.com>; Fri, 14 Feb 2020 22:26:57 -0800 (PST) X-Va-A: X-Va-T-CD: bfa6a58ae394943bdbed67caa1403570 X-Va-E-CD: ce6e59d19e41f9f978af9e8b590c623d X-Va-R-CD: 4ad19a74e8868beceae55112f7f03bd9 X-Va-CD: 0 X-Va-ID: c43c1986-1c31-45ba-8d86-a23bb15ae0c7 X-V-A: X-V-T-CD: bfa6a58ae394943bdbed67caa1403570 X-V-E-CD: ce6e59d19e41f9f978af9e8b590c623d X-V-R-CD: 4ad19a74e8868beceae55112f7f03bd9 X-V-CD: 0 X-V-ID: 31dc9013-4d3e-477a-9deb-9702ee11607f X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572 definitions=2020-02-15_01:2020-02-14,2020-02-15 signatures=0 Received: from [17.235.48.232] by rn-mailsvcp-mmp-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) with ESMTPSA id <0Q5Q001D8CKUAG50@rn-mailsvcp-mmp-lapp02.rno.apple.com>; Fri, 14 Feb 2020 22:26:56 -0800 (PST) Sender: afish@apple.com From: "Andrew Fish" Message-id: <2719704F-6DB4-444F-97FE-DBF71D39B698@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: Fri, 14 Feb 2020 22:26:54 -0800 In-reply-to: Cc: devel@edk2.groups.io, Mike Kinney , "Gao, Liming" , "Gao, Zhichao" , =?utf-8?Q?Marvin_H=C3=A4user?= , Laszlo Ersek To: vit9696 References: <20200103171242.63839-1-vit9696@protonmail.com> <492A300B-1CED-4FF0-98F8-20D1E116DCC0@protonmail.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2020-02-15_01:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_629EE81A-BBD7-446F-8C49-D4FD7CD50ADB" --Apple-Mail=_629EE81A-BBD7-446F-8C49-D4FD7CD50ADB Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Vitaly, Sorry after I sent the mail I realized it may come across as me asking yo= u to do work and that was not my intent.=20 I will point out though that a non backward compatible change to something= as fundamental as the DebugLib is a very big deal. I've got a few differen= t custom implementations that would break with this change as Mike proposed= . Given breaking every one's debug lib is such a big deal maybe it is somet= hing that we should do as a long term plan vs. some incremental fix. So my = intent was to start a conversation about what else we might want to change = if we are going to break the world. The only think worse than breaking the = world is breaking the world frequently.=20 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_CHECK a= nd making them global constructs. I think the way others have dealt with th= ings like this is to make them be DEBUG prints vs. ASSERTs. Also even somet= hing as simple as SAFE_STRING_CONSTRAINT_CHECK could be called from code th= at wants ASSERT and CONSTRAINT_ASSERT behavior. It is not clear to me that = the low level code knows the right thing to do in a global sense even if th= ere is a PCD. It almost seems like we should have wrappers for the Safe st= ring functions that implement the behavior you want as a caller. I'm not su= re about that, but it seems like it is worth talking about?=20 Thanks, Andrew Fish > On Feb 14, 2020, at 7:31 PM, vit9696 wrote: >=20 > Hi Andrew, >=20 > While your suggestions look interesting, I am afraid they are not partic= ularly what we want to cover with this discussion at the moment. >=20 > 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. >=20 > =3DTo make it clear, currently I plan to add the following interface: >=20 > #define CONSTRAINT_ASSERT(Expression) \ > do { \ > if (DebugConstraintAssertEnabled ()) { \ > if (!(Expression)) { \ > _ASSERT (Expression); \ > ANALYZER_UNREACHABLE (); \ > } \ > } \ > } while (FALSE) >=20 > with DebugConstraintAssertEnabled implemented as=20 >=20 > (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_ASSERT_= ENABLED | DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED) =3D=3D DEBUG_PROPERTY_D= EBUG_ASSERT_ENABLED) >=20 > 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. >=20 > Best wishes, > Vitaly >=20 > On Sat, Feb 15, 2020 at 03:02, Andrew Fish > wrote: >>=20 >>=20 >>=20 >>> On Feb 14, 2020, at 2:50 PM, Michael D Kinney > wrote: >>>=20 >>> Hi Vitaly, >>> >>> I agree that this proposal makes a lot of sense. We recently added a = new assert type called STATIC_ASSERT() for assert conditions that can be te= sted at build time. >>> >>> A new assert type for checks that can be removed and the API still gua= rantees 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_A= SSERT()? >>> >>> We also want the default to be enabled. The current use of bit 0x40 i= n PcdDebugPropertyMask is always clear, so we want the asserts enabled whe= n 0x40 is clear. We can change the name of the define bit to DEBUG_PROPERT= Y_CONTRAINT_ASSERT_DISABLED so bit 0x40 needs to be set in PcdDebugProperty= Mask to disable these types of asserts. >>> >>> This approach does require all the DebugLib implementations to be upda= ted with the new DebugConstraintAssertDisabled() API. >>> >>=20 >> Mike, >>=20 >> If you wanted to be backward compatible you could just use DebugAssertE= nabled () but in place of _ASSERT() use _CONSTRAINT_ASSERT >>=20 >> #define _ASSERT(Expression) DebugAssert (__FILE__, __LINE__, #Expressi= on) >>=20 >> #define _CONSTRAINT_ASSERT(Expression) DebugPrint (DEBUG_ERROR, "ASSE= RT %a(%d): %a\n",, __FILE__, __LINE__, #Expression) >>=20 >> Not as elegant as the non backward compatible change, but I thought I'd= throw it out there.=20 >>=20 >> There are some ancient Apple C ASSERT macros [AssertMacros.h] that als= o have the concept of require. Require includes an exception label (goto la= bel). It is like a CONSTRAINT_ASSERT() but with the label. On release build= s the DEBUG prints are skipped.=20 >>=20 >> So we could do something like: >>=20 >> EFI_STATUS Status =3D EFI_INVALID_PARAMETER; >>=20 >> REQUIRE(Arg1 !=3D NULL, ErrorExit); >> REQUIRE(Arg2 !=3D NULL, ErrorExit); >> REQUIRE(Arg3 !=3D NULL, ErrorExit); >>=20 >> ErrorExit: >> return Status; >>=20 >> There is another form that allows an ACTION (a statement to execute. So= you could have: >>=20 >> EFI_STATUS Status; >>=20 >> REQUIRE_ACTION(Arg1 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARA= METER); >> REQUIRE_ACTION(Arg2 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARA= METER); >> REQUIRE_ACTION(Arg3 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARA= METER); >>=20 >> ErrorExit: >> return Status; >>=20 >> If you use CONSTRAINT_ASSERT(); >>=20 >> 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; >> } >>=20 >> I'd note error processing args on entry is the simplest case. In a mor= e complex case when cleanup is required the goto label is more useful.=20 >>=20 >> I guess we could argue for less typing and more symmetry and say use AS= SERT, CONSTRAINT, and REQUIRE. I guess you could add an ASSERT_ACTION too.= =20 >>=20 >> The AssertMacros.h versions also support _quiet (skip the print) and _s= tring (add a string to the print) so you end up with: >> REQUIRE >> REQUIRE_STRING >> REQUIRE_QUIET >> REQUIRE_ACTION >> REQUIRE_ACTION_STRING >> REQUIRE_ACTION_QUIET >>=20 >> We could also end up with=20 >> CONSTRAINT >> CONSTRAINT_STRING >> CONSTRAINT_QUIET >>=20 >> I think the main idea behind _QUIET is you can silence things that are = too noisy, and you can easily make noise things show up by removing the _QU= IET to debug.=20 >>=20 >> I'd thought I throw out the other forms for folks to think about. I'm p= robably 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"); >>=20 >> Thanks, >>=20 >> Andrew Fish >>=20 >> PS The old debug macros had 2 versions of CONSTRAINT check and verify. = The check version was compiled out on a release build, the verify version a= lways does the check and just skips the DEBUG print.=20 >>=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 strin= g 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 functi= on will NOT work if they are violated) >>> - constraint asserts (i.e. assertions, which allow us to spot unintent= ional behaviour when parsing untrusted data, but which do not break functio= n behaviour). >>> >>> As we want to use this outside of SafeString, I suggest the following= : >>> - Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 bit for PcdD= ebugPropertyMask instead of PcdAssertOnSafeStringConstraints. >>> - Introduce DebugAssertConstraintEnabled DebugLib function to check fo= r DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED. >>> - Introduce ASSERT_CONSTRAINT macro, that will assert only if DebugCon= straintAssertEnabled returns true. >>> - Change SafeString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to ASSERT_= CONSTRAINTs. >>> - Use ASSERT_CONSTRAINT in other places where necessary. >>>=20 >>>=20 >>> I believe this way lines best with EDK II design. If there are no obje= ctions, I can submit the patch in the beginning of next week. >>>=20 >>>=20 >>> Best wishes, >>> Vitaly >>>=20 >>>=20 >>> 14 =D1=84=D0=B5=D0=B2=D1=80. 2020 =D0=B3., =D0=B2 20:00, Kinney, Micha= el 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() beha= vior in more than just safe string functions in BaseLib. >>> >>> Can we consider changing the name and description of PcdAssertOnSafeSt= ringConstraints to be more generic, so if we find other lib APIs, the name = will make sense? >>> >>> Maybe something like: PcdEnableLibraryAssertChecks? Default is TRUE. = Can set to FALSE in DSC file to disable ASSERT() checks. >>> >>> Thanks, >>> >>> Mike >>> >>> >>> >>> From: devel@edk2.groups.io > On Behalf Of Vitaly Cheptsov via Gr= oups.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 strin= g constraint assertions >>> >>> Replying as per Liming's request for this to be merged into edk2-stabl= e202002. >>> >>> 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 n= o negative response. As I mentioned previously, my team will strongly benef= it from its landing in EDK II mainline. Since it does not add any regressio= ns and can be viewed as a feature implementation for the rest of EDK II use= rs, 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= the next stable release. >>> >=20 >>> > Best, >>> > Vitaly >>> >=20 >>> >> 8 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 19:35, Kinney, Michael D= > =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 se= nt 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 >>=20 >=20 >=20 --Apple-Mail=_629EE81A-BBD7-446F-8C49-D4FD7CD50ADB Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Vitaly,
Sorry after I sent the mail I realized i= t may come  across as me asking you to do work and that was not my int= ent. 

I will= point out though that a non backward compatible change to something as fun= damental as the DebugLib is a very big deal. I've got a few different custo= m implementations that would break with this change as Mike proposed. Given= breaking every one's debug lib is such a big deal maybe it is something th= at we should do as a long term plan vs. some incremental fix. So my intent = was to start a conversation about what else we might want to change if we a= re going to break the world. The only think worse than breaking the world i= s breaking the world frequently. 

=
I'm also a little worried that we are taking things t= hat are today locally scoped like SAFE_STRING_CONSTRAINT_CHECK and SAF= E_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 something as simple as SAFE_STRING_CONSTRAINT_CHECK co= uld be called from code that wants ASSERT and CONSTRAINT_ASSERT behavior. I= t is not clear to me that 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 shoul= d have wrappers for the Safe string functions that implement the behavior y= ou want as a caller. I'm not sure about that, but it seems like it is worth= talking about? 

Thanks,

An= drew Fish

On Feb 14, 2020, at 7:31 PM, vit9696 <vit9696@protonmail.com> wr= ote:

Hi Andrew,

While your = suggestions look interesting, I am afraid they are not particularly&nb= sp;what we want to cover with this discussion at the moment.

Making assertions go through DEBUG printing functions/macros&= nbsp;is what we have to strongly disagree about. Assertions and debug print= s are separate things configurable by separate PCDs. We should no= t mix them. Introducing constraint assertions is a logical step forward in = understanding that different software works in different environments.
  • There are normal, or, as I call them, invariant assertions = (e.g. preconditions), 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 cam= e through 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 config= urable, because not every piece of software works in a tru= sted environment. Other than that, constraint assertions, when enabled, are= not anyhow different from normal assertions in the sense of action taken. = Assertions have configurable breakpoints and deadloops, and DEBUG prin= ts go through a different route in DebugLib that may cause entirely different eff= ects. For example, we halt execution upon printing to DEBUG_ERROR in our De= bugLib even in release builds.

=3DTo make it = clear, currently I plan to add the following interface:
=

  #define= CONSTRAINT_ASSERT(Expression) \
  &nbs= p; do { \
     &= nbsp;if (DebugConstraintAssertEnabled ()) { \
       =  if (!(Expression)) { \
  &= nbsp;       _ASSERT (Expression); \
    &nbs= p;     ANALYZER= _UNREACHABLE (); \
      &nbs= p; } \
     &nbs= p;} \
    } while (FALSE)

wi= th DebugConstraintAssertEnabled implemented as 

(BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_ASSE= RT_ENABLED | DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED) =3D=3D DEBUG_PROPERT= Y_DEBUG_ASSERT_ENABLED)
=
Your suggestion with require= macros looks interesting indeed, but I believe it is in fact parallel to t= his discussion. The change we discuss introduces a new assertion primitive = = =E2=80=94 constraint assertions, while REQUIRE macros are mostly about adv= anced 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 discus= s this later once constraint assertions are merge= d into EDK II tree.

Best wishes,
Vitaly

O= n Sat, Feb 15, 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,
&nbs= p;
I agree that th= is proposal makes a lot of sense.  We recently added a new assert type = called STATIC_ASSERT() for assert conditions that can be tested at build ti= me.
 <= /div>
A new assert type for che= cks that can be removed and the API still guarantees that it fails graceful= ly with a proper return code is a good idea.  Given we have STATIC_ASSE= RT(), how about naming the new macro CONSTRAINT_ASSERT()?
 
We also want the default to be enabled.  T= he current use of bit 0x40 in PcdDebugPropertyMask&nb= sp; is always clea= r, so we want the asserts enabled when 0x40 is clear. = ; We can change th= e name of the define bit to DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED so bit= 0x40 needs to be set in = PcdDebugPropertyMask to disable these types of asserts.
 
This approach does require all the DebugLib implementations to = be updated with the new <= span class=3D"SpellE">DebugConstraintAssertDisabled() API.
 

Mike,=

If you wanted to= be backward compatible you could just use DebugAssertEnabled () but i= n place of _ASSERT() use _CONSTRAINT_ASSERT

#define _ASSERT(Expression) &n= bsp;DebugAssert (__FILE__, __LINE__, #Expression)

#define _CONSTRAINT_A= SSERT(Expression)  DebugPrint (DEBUG_ERROR,  "ASSERT %a(%d): %a\n= ",, __FILE__, __LINE__, #Expression)

Not as elegant as the non backward compa= tible 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 CONSTR= AINT_ASSERT() but with the label. On release builds the DEBUG prints are sk= ipped. 

So w= e could do something like:

  EFI_STATUS Status =3D  EFI_INVALID_PARAMETER;

  R= EQUIRE(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, Error= Exit, Status =3D EFI_INVALID_PARAMETER);
  REQUIR= E_ACTION(Arg2 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMETER);
  REQUIRE_ACTION(Arg3 !=3D NULL, ErrorExit, Status = =3D EFI_INVALID_PARAMETER);

ErrorExit:
  return Status;

If you use CONSTRAIN= T_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);
&nb= sp;  return EFI_INVALID_PARAMETER;
 }
<= div class=3D"">
I'd note error processi= ng args on entry is the simplest case.  In a more complex case when cl= eanup is required the goto label is more useful. 

I guess we could argue for less typin= g and more symmetry and say use ASSERT, CONSTRAINT, and REQUIRE. I guess yo= u could add an ASSERT_ACTION too. 

The AssertMacros.h versions also support _quiet (ski= p the print) and _string (add a string to the print) so you end up with:
REQUIRE
REQUIRE_STRING
REQUIRE_QUIET
REQUIRE_ACTION
REQU= IRE_ACTION_STRING
REQUIRE_ACTION= _QUIET

We could a= lso end up with 
CONSTRAINT
=
CONSTRAINT_STRING
CONSTRAINT_QUIET

I think the main idea behind _QUIET is you ca= n silence things that are too noisy, and you can easily make noise things s= how up by removing the _QUIET to debug. 

I'd thought I throw out the other forms for fo= lks to think about. I'm probably biased as I used to looking at code and se= eing things like require_action_string(Arg1 !=3D NULL, ErrorExit, Status = =3D EFI_INVALID_PARAMETER, "1st Arg1 check");

Thanks,

<= /div>
Andrew Fish

=
PS The old debug macros had 2 versions of CONSTRAINT check = and verify. The check version was compiled out on a release build, the veri= fy version always does the check and just skips the DEBUG print. 

=
Mike
=
 
 
 
From: vit9696 <vit9696@protonmail.com> =
Sent: Friday, February 14, 2020 9:38 AM
To: Kinney, M= ichael D <michael.d.kinney@intel.com<= /a>>
Cc: devel@edk2.groups.io; G= ao, Liming <liming.gao@intel.com>; G= ao, Zhichao <zhichao.gao@intel.com>= ; Marvin H=C3=A4user <marvin.haeuser@= outlook.com>; Laszlo Ersek <lersek@redh= at.com>
Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to d= isable safe string constraint assertions
 
Michael,
 
Generalising the approach makes good sense to me, but we= need to make an obvious distinguishable difference between:
- precondi= tion and invariant assertions (i.e. assertions, where function will NOT wor= k if they are violated)
- constraint asserts (i.e. assertions, which al= low us to spot unintentional behaviour when parsing untrusted data, but whi= ch do not break function behaviour).
 
As we want t= o use this outside of SafeString,  I sugg= est the following:
- Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLE= D 0x40 bit for PcdDebugPropertyMask instead of PcdAssertOnSafeStr= ingConstraints.
- Introduce DebugAssertCons= traintEnabled DebugLib function to check for DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED.
- Introduce = ASSERT_CONSTRAINT macro, that will assert only if DebugConstraintAssertEnabled returns true.
=
- Change SafeString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to = ;ASSERT_CONSTRAINTs.
- Use ASSERT_CONSTRAINT in other places where necessary.


=
I believe this way lines best with EDK II design. If ther= e are no objections, I can submit the patch in the beginning of next week.<= /span>


Best wishes,
Vitaly


14 =D1=84=D0=B5=D0=B2=D1=80. 2020 =D0=B3., =D0=B2 20:00, Kinney= , Michael D <michael.d.kinney@intel.c= om> =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() behavior in more than just safe string fun= ctions in BaseLib.
 
Can we consider changing the name= and description of PcdAssertOnSafeStringConstraints to be more generic, so if we find other li= b APIs, the name will make sense?
 
Maybe something like: Pcd= EnableLibraryAssertChecks?  Default is TRUE. =  Can set to FALSE in DSC file to disable ASSERT() checks.
 =
Thanks,
 
Mike
=  
 
 
From: de= vel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, = February 14, 2020 3:55 AM
To: Kinney, Michael D <michael.d.kinn= ey@intel.com>; Gao, Liming <<= span style=3D"color: purple;" class=3D"">liming.gao@intel.com>= ;; Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
Cc: Marvi= n H=C3=A4user <marvin.haeuser@outlook.com>; Laszlo Ersek= <le= rsek@redhat.com>
Subject: Re: [edk2-devel] [PATCH v3 0= /1] Add PCD to disable safe string constraint assertions
=
&n= bsp;
Replying as per Liming's request for this to be me= rged into edk2-stable202002.
&nbs= p;
On Mon, Feb 10, 2020 at 14:12, vit9696 <vit9696@protonmail.co= m> wrote:

Hello,<= br class=3D"">
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 do= es not add any regressions and can be viewed as a feature implementation fo= r the rest of EDK II users, I request this to be merged upstream in edk2-st= able202002.

Best wishes,
Vitaly<= br class=3D"">
> 27 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0= =B2 12:47, vit9696 <vit9696@protonmail.com> =D0=BD=D0=B0=D0= = =BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):
> 
> 
> Hi Mike,
><= span class=3D"apple-converted-space"> 
> Any p= rogress with this? We would really benefit from this landing in the next st= able release.
> = ;
> Best,
> Vitaly
&= gt; 
>&g= t; 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,
>&g= t; 
>>= ; 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.
>> 
>> If we find= other APIs where ASSERT() behavior is only
>> valuable= during dev/debug to quickly identify misuse
>> with tr= usted data and the API provides predicable
>> return be= havior when ASSERT() is disabled, then I would
>> like = to have a pattern we can potentially apply to all
>> th= ese APIs across all packages.
>> 
>> Thanks,
&= gt;> 
&g= t;> Mike
>>&nb= sp;
>>> -----Original Message-----
>>> From: dev= el@edk2.groups.io <devel@edk2.groups.io> On
>>> Be= half Of Vitaly Cheptsov via Groups.Io
>>> Sent: Mond= ay, January 6, 2020 10:44 AM
>>> To: Kinney, Michael= D <michael.d.kinney@intel.com>
>>&g= t; Cc: devel@edk2.groups.io
>>> Subject: Re: [edk2-devel] [PATCH v3 0/= 1] Add PCD to
>>> disable safe string constraint ass= ertions
>>>&nb= sp;
>>> Hi Mike,
>>> 
>>>= ; 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.>>> =
>>> I have already explained in the BZ that basical= ly all
>>> safe string constraint assertions make no= sense for
>>> handling untrusted data. We find this= use case very
>>> logical, as these functions behav= e properly with
>>> assertions disabled and cover al= l these error
>>> conditions by the return statuses.= In such situation is
>>> not useful for these funct= ions to assert, as we end up
>>> inefficiently reimp= lementing the logic. I would have
>>> liked the appr= oach of discussing the interfaces
>>> individually, = but I struggle to find any that makes
>>> sense from= this point of view.
>>> 
>>> AsciiStrToGuid will ASSE= RT when the length of the
>>> passed string is odd. = Functions that cannot, ahem,
>>> parse, for us are p= retty much useless.
>>> AsciiStrCatS will ASSERT whe= n the appended string does
>>> not fit the buffer. F= or us this logic makes this
>>> function pretty much= equivalent to deprecated and thus
>>> unavailable A= sciiStrCat, except it is also slower.
>>> 
>>> My ori= ginal suggestion was to remove the assertions
>>> en= tirely, but several people here said that they use
>>&g= t; them to verify usage errors when handling trusted data.
&g= t;>> This makes good sense to me, so we suggest to support
>>> both cases by introducing 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
>>> <michael.d.kinney@int= el.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):>>>> 
>>>>&= nbsp;
>>>> Hi Vitaly,
>&g= t;>> 
>>>> Is the use case for UEFI Applications?
>= >>> 
>>>> There is a different mechanism to disable all
>>> ASSERT()
>>>> statements with= in a UEFI Application.
>>>> 
>>>> If a component= is consuming data from an untrusted
>>> source,
>>>> then that component is required to verify the>>> untrusted
>>>> data bef= ore passing it to a function that clearly
>>> docume= nts
>>>> 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 b= efore calling.
>>>> 
>>>> 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,
>>> t= hen we
>>>> should discuss each of those APIs ind= ividually.
>>>> 
>>>> Mike
>>= ;>> 
= >>>> 
>>>>> -----Original Message-----
>&g= t;>>> From: devel@ed= k2.groups.io &= lt;d= evel@edk2.groups.io> On
>>>>> Be= half Of Vitaly Cheptsov via Groups.Io
>>>>> Se= nt: Friday, January 3, 2020 9:13 AM
>>>>> To:<= span class=3D"apple-converted-space"> devel@edk2.groups.io
>>>>> Subject: [edk2-devel] [PATCH v3 0/= 1] Add PCD to
>>> disable
>>>= >> safe string constraint assertions
>>>>&g= t; 
>>= ;>>> REF:
>>>>> https://bugzilla.tianoco= re.org/show_bug.cgi?id=3D2054
>>>>>=  
>>&= gt;>> Requesting for merge in edk2-stable202002.
>&g= t;>>> 
>>>>> Changes since V1:
>>>>= ;> - Enable assertions by default to preserve the
>>= > original
>>>>> behaviour
&g= t;>>>> - Fix bugzilla reference link
>>>= >> - Update documentation in BaseLib.h
>>>>= > 
>&= gt;>>> Vitaly Cheptsov (1):
>>>>> Mde= Pkg: Add PCD to disable safe string constraint
>>>&g= t;> assertions
>>>>> 
>>>>> MdePkg/MdeP= kg.dec | 6 ++
>>>>> MdePkg/Library/BaseLib/Bas= eLib.inf | 11 +--
>>>>> MdePkg/Include/Library= /BaseLib.h | 74
>>>>> +++++++++++++-------
>>>>> MdePkg/Library/BaseLib/SafeString.c | 4 +-<= br class=3D"">>>>>> MdePkg/MdePkg.uni | 6 ++
&= gt;>>>> 5 files changed, 71 insertions(+), 30 deletions(-)
>>>>> <= /span>
>>>>> --
>>>>= > 2.21.0 (Apple Git-122.2)
>>>>> 
>>>>>= ; 
>>= >>> -=3D-=3D-=3D-=3D-=3D-=3D
>>>>> Groups.io Links: You receive all messages sent to
>>> this
>>>>> group.
>>>>> 
>>>>> View/Reply Online (#52837):
>>>>> https://edk2.groups.io/g/devel/message/52837
>>>>> Mute This Topic:
>>>=  https://groups.io/= mt/69401948/1643496
>>>>> Group Own= er: devel+owner@edk2.gr= oups.io
>>>>> Unsubscribe: https://edk2.groups.io/g/d= evel/unsub
>>>>> [michael.d.kinne= y@intel.com]
>>>>> -=3D-=3D-=3D-=3D= -=3D-=3D
>>>> 
>>> 
>>> 
>>> 
>> 
> 

 
 = ;
 



<= /blockquote>
--Apple-Mail=_629EE81A-BBD7-446F-8C49-D4FD7CD50ADB--