From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp01.apple.com (ma1-aaemail-dr-lapp01.apple.com [17.171.2.60]) by mx.groups.io with SMTP id smtpd.web12.1884.1581724962970401422 for ; Fri, 14 Feb 2020 16:02:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=b6SACABu; spf=pass (domain: apple.com, ip: 17.171.2.60, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp01.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp01.apple.com (8.16.0.27/8.16.0.27) with SMTP id 01F02DNq064112; Fri, 14 Feb 2020 16:02:41 -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=de2upG4AinfFJGbU2wRrKXeJpwqi0PoLSHi2bHyyfCM=; b=b6SACABuZlJfmhApaf/bwugsrV1vrAKzRld1Yu68+QF+W6EyWOwmKSm9vit0/4Ri7/5E +/lyks8g40llGZXW2tSCFPm67mL8v/EAWIzoKi03yplEgb/8PpmR+jo6X5BLdE4DgQmc Y9KJKzMSpHqz7TgRcsbzBgx0FQT2LuyXsvJp0aaCmxYQOF9dinciGFagn1h/3yCQq2Dw l48B0Mvka7rGM3g7jQYdtM18h81KaO769//PtvGKuPr2nBTc+MO+p6ROR+d3cduyk46c Kyt4Su8H/HAnoWcoQrCxrJoODwRSMtICQ/11VXBfC8y9zvg/NgKaRnka55BvNIpFloOG 7g== Received: from rn-mailsvcp-mta-lapp04.rno.apple.com (rn-mailsvcp-mta-lapp04.rno.apple.com [10.225.203.152]) by ma1-aaemail-dr-lapp01.apple.com with ESMTP id 2y5bd65r3a-17 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Fri, 14 Feb 2020 16:02:41 -0800 Received: from rn-mailsvcp-mmp-lapp01.rno.apple.com (rn-mailsvcp-mmp-lapp01.rno.apple.com [17.179.253.14]) by rn-mailsvcp-mta-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) with ESMTPS id <0Q5P00KG5USBU4A0@rn-mailsvcp-mta-lapp04.rno.apple.com>; Fri, 14 Feb 2020 16:02:36 -0800 (PST) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp01.rno.apple.com by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) id <0Q5P00X00UOAEN00@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Fri, 14 Feb 2020 16:02:35 -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: 37be589f-3a1e-4891-bdf6-95fcbf34af87 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: ba471626-1198-4f0a-85ff-0cd6ce9f13b8 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572 definitions=2020-02-14_09:2020-02-14,2020-02-14 signatures=0 Received: from [17.235.48.232] by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) with ESMTPSA id <0Q5P00OMNUS9FB00@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Fri, 14 Feb 2020 16:02:34 -0800 (PST) Sender: afish@apple.com From: "Andrew Fish" Message-id: 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 16:02:32 -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> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2020-02-14_09:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_34675231-9D0C-4ABC-839F-EB518144A384" --Apple-Mail=_34675231-9D0C-4ABC-839F-EB518144A384 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 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 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 DebugAssertEnab= led () but in place of _ASSERT() use _CONSTRAINT_ASSERT #define _ASSERT(Expression) 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 backward compatible change, but I thought I'd th= row it out there.=20 There are some ancient Apple C ASSERT macros [AssertMacros.h] that also h= ave the concept of require. Require includes an exception label (goto label= ). It is like a CONSTRAINT_ASSERT() but with the label. On release builds t= he 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 yo= u could have: EFI_STATUS Status; REQUIRE_ACTION(Arg1 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMET= ER); REQUIRE_ACTION(Arg2 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMET= ER); REQUIRE_ACTION(Arg3 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMET= ER); 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 c= omplex 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 ASSER= T, CONSTRAINT, and REQUIRE. I guess you could add an ASSERT_ACTION too.=20 The AssertMacros.h versions also support _quiet (skip the print) and _stri= ng (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 too= noisy, and you can easily make noise things show up by removing the _QUIET= to debug.=20 I'd thought I throw out the other forms for folks to think about. I'm prob= ably biased as I used to looking at code and seeing things like require_act= ion_string(Arg1 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMETER, "1s= t Arg1 check"); Thanks, 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 verify version alwa= ys 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 >=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 >=20 > Best wishes, > Vitaly >=20 >=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=_34675231-9D0C-4ABC-839F-EB518144A384 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Feb 14, 2= 020, 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 recently added a new assert type called STATIC_ASSER= T() for assert conditions that can be tested at build time.=
 
A new assert type for checks that can be removed and the API still guaran= tees 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 the asserts enabled when 0x40 is clear.  We can change the n= ame of the define bit to DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED so bit 0x= 40 needs to be set in PcdDebugPropertyMask to disable these types of asserts.
 
Th= is approach does require all the = ;DebugLib implementations to be updated with the new DebugConstr= aintAssertDisabled() API.
 

Mike,
=
If you wanted to be backward compatible you could= just use DebugAssertEnabled () but in place of _ASSERT() use _CONSTRA= INT_ASSERT

#define _ASSERT(Express= ion)  DebugAssert (__FILE__, __LINE__, #Expression)

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

=
Not as elegant as the non backward compatible c= hange, but I thought I'd throw it out there. 
There are some ancient Apple C ASSERT mac= ros [AssertMacros.h]  that also have the concept of require. Require i= ncludes an exception label (goto label). It is like a CONSTRAINT_ASSER= T() but with the label. On release builds the DEBUG prints are skipped.&nbs= p;

So we could do= something like:

=   EFI_STATUS Status =3D  EFI_INVALID_PARAMETER;

  REQUIRE(= Arg1 !=3D NULL, ErrorExit);
&nbs= p; REQUIRE(Arg2 !=3D NULL, ErrorExit);
  REQUIRE(Arg3 !=3D NULL, ErrorExit);

ErrorExit:
&nbs= p; return Status;

There is another form that allows an ACTION (a statement to execute. So yo= u could have:

  EFI_STATUS Status;

=
  REQUIRE_ACTION(Arg1 !=3D NULL, ErrorExit, Stat= us =3D EFI_INVALID_PARAMETER);
  REQUIRE_ACTION(A= rg2 !=3D NULL, ErrorExit, Status =3D EFI_INVALID_PARAMETER);
  REQUIRE_ACTION(Arg3 !=3D NULL, ErrorExit, Status =3D EFI_INVA= LID_PARAMETER);

E= rrorExit:
  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);
&nb= sp;  CONSTRAINT_ASSERT (Arg2 !=3D NULL);
  &= nbsp;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. 

I guess we could argue for less typing and mo= re symmetry and say use ASSERT, CONSTRAINT, and REQUIRE. I guess you could = add an ASSERT_ACTION too. 

<= div class=3D"">The AssertMacros.h versions also support _quiet (skip the pr= int) and _string (add a string to the print) so you end up with:
REQUIRE
REQUIRE_STRING
REQUIRE_QUIET
REQUIRE_ACTION
REQUIRE_AC= TION_STRING
REQUIRE_ACTION_QUIET=

We could also en= d up with 
CONSTRAINT
CONSTRAINT_STRING
CONS= TRAINT_QUIET

I think the main idea behind _QUIET is you can sile= nce 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 t= hings like require_action_string(Arg1 !=3D NULL, ErrorExit, Status =3D EFI_= INVALID_PARAMETER, "1st Arg1 check");

<= /div>
Thanks,

Andrew Fish

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

Mike=
&= nbsp;
 
 
From: vit9696 <vit9696@protonmail.com>=  
Sent: Friday, February 14, 2020 9:38 AM
= To: Kin= ney, Michael D <michael.d.kinney@inte= l.com>
Cc: devel@edk2.groups.io<= /a>; Gao, Liming <liming.gao@intel.com&= gt;; Gao, Zhichao <zhichao.gao@intel.com>; Marvin H=C3=A4user <marvin.ha= euser@outlook.com>; Laszlo Ersek <lerse= k@redhat.com>
Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PC= D to disable safe string constraint assertions=
 <= /div>
Michael,<= /o:p>
 
Generalising the approach makes goo= d sense to me, but we need to make an obvious distinguishable difference be= tween:
- 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 unintentional behaviour when parsing untrusted data, but which d= o not break function behaviour).
=  
= As we want to use this outside of SafeString,&n= bsp; I suggest the following:<= /o:p>
- Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 bit for= PcdDebugPropertyMask instead of PcdAssertOnSafeStringConstraints= .
- Introduce DebugAssertConstraintEnabled DebugLib function to check for DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED.
- Introduce ASSERT_CONSTRAINT macro, that will asse= rt only if DebugConstraintAssertEnabled&nb= sp;returns true.
- Change Saf= eString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to ASSERT_CONSTRA= INTs.
- Use ASSERT_CONSTRAINT in other places where necessary.=


I believe t= his way lines best with EDK II design. If there are no objections, I can su= bmit 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 2= 0:00, Kinney, Michael D <michael.d.ki= nney@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):<= o:p class=3D"">
 
Vitaly,=
 
I want t= o make sure a feature PCD can be used to disable ASSERT() behavior in more = than just safe string functions in&nb= sp;BaseLib.
 
Can we consider changing the name and d= escription of PcdAssertOnSafeStringConstraints to be more generic, so if we find other lib API= s, the name will make sense?<= /span>
 
Maybe something like:=  PcdEnableLibraryAssertChecks?&nb= sp; Default is TRUE. = ; Can set to FALSE in DSC= file to disable ASSERT() checks.
 
Thanks,<= /span>
 
Mike
&nbs= p;
 
 
From: devel@edk2.groups= .io <devel@edk2= .groups.io> On Behalf Of Vitaly Cheptsov via Groups.Io
Sent:<= span class=3D"apple-converted-space"> Friday, February 14, 2020= 3:55 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Gao, Zhicha= o <zhichao.gao@intel.com>; devel@edk2.groups.io
Cc:=  Marvin H=C3=A4user &= lt;marvin.haeuser@outlook.com>; Laszlo Ersek <lersek@redhat= .com>
Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD= to disable safe string constraint assertions<= /div>
 
Replying as per L= iming's request for this to be merged into edk2-stable202002.
 <= /o:p>
<= span class=3D"">On Mon, Feb 10, 2020 at 14:12, vit9696 <vit9696@protonmail= .com> wrote:

Hello,

It has been quite som= e 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 m= erged upstream in edk2-stable202002.

Best wish= es,
Vitaly

> 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):
> 
&g= t; 
> Hi= Mike,
> 
> Any progress with this? We would really benefit from t= his landing in the next stable release.
> 
> Best,
&= gt; 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 V= italy,
>> 
>> Thanks for the additional background. I would = like
>> a couple extra day to review the PCD name and t= he 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 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.
>&= gt; 
>&g= t; Thanks,
>>&nbs= p;
>> Mike
>> 
>>> -----Origin= al Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On
>>> Behalf Of Vitaly Cheptsov via Groups.Io
&= gt;>> Sent: Monday, January 6, 2020 10:44 AM
>>&g= t; To: Kinney, Michael D <michael.d.kinney@intel.com>>>> Cc: devel@edk2.groups.io
>>> Subject: Re: [e= dk2-devel] [PATCH v3 0/1] Add PCD to
>>> disable saf= e string constraint assertions
>>> 
>>> Hi Mike,
>>> >>> Yes, the primary use case is for UEFI Applications= . We
>>> do not want to disable ASSERT=E2=80=99s com= pletely, as
>>> assertions that make sense, i.e. the= ones signalising
>>> about interface misuse, are he= lpful for debugging.
>>> 
>>> I have already explained= in the BZ that basically all
>>> safe string constr= aint assertions make no sense for
>>> handling untru= sted data. We find this use case very
>>> logical, a= s these functions behave properly with
>>> assertion= s 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
>>&= gt; 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.
>>> 
>>> A= sciiStrToGuid will ASSERT when the length of the
>>>= passed string is odd. Functions that cannot, ahem,
>>&= gt; parse, for us are pretty much useless.
>>> Ascii= StrCatS will ASSERT when the appended string does
>>>= ; not fit the buffer. For us this logic makes this
>>&g= t; function pretty much equivalent to deprecated and thus
>= ;>> unavailable AsciiStrCat, except it is also slower.
= >>> 
>>> 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 t= his patch.
>>>=  
>>> Best wishes,
>>= ;> Vitaly
>>> 
>>>> 6 =D1=8F=D0=BD=D0=B2. 2020 = =D0=B3., =D0=B2 21:28, 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,=
>>>> =
>>>> Is the use case for UEFI Application= s?
>>>>&nbs= p;
>>>> There is a different mechanism to = disable all
>>> ASSERT()
>>>&= gt; statements within a UEFI Application.
>>>> 
>>>= > If a component is consuming data from an untrusted
>&= gt;> source,
>>>> then that component is requi= red to verify the
>>> untrusted
>&g= t;>> 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() condit= ions are verified before calling.
>>>> 
>>>> If= there are some APIs that currently document their
>>&g= t; ASSERT()
>>>> behavior and we think that ASSER= T() behavior is
>>> incorrect and
>= >>> should be handled by an existing error return value,
>>> then we
>>>> should discuss e= ach of those APIs individually.
>>>> 
>>>> Mi= ke
>>>>&nbs= p;
>>>> 
>>>>> -----Original 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
&= gt;>>>> To: <= a href=3D"mailto:devel@edk2.groups.io" style=3D"color: purple; text-decorat= ion: underline;" class=3D"">devel= @edk2.groups.io
>>>>> Subject: [edk= 2-devel] [PATCH v3 0/1] Add PCD to
>>> disable
>>>>> safe string constraint assertions
>>>>> <= br class=3D"">>>>>> REF:
>>>>><= span class=3D"apple-converted-space"> htt= ps://bugzilla.tianocore.org/show_bug.cgi?id=3D2054
>>>>> >>>>> Requesting for merge in edk2-stable202002.=
>>>>>&n= bsp;
>>>>> Changes since V1:
>>>>> - Enable assertions by default to preserve the<= br class=3D"">>>> original
>>>>> beha= viour
>>>>> - Fix bugzilla reference link
>>>>> - Update documentation in BaseLib.h
>>>>> 
>>>>> Vitaly Cheptsov (1):
&= gt;>>>> MdePkg: Add PCD to disable safe string constraint
>>>>> assertions
>>>>>=  
>>&= gt;>> MdePkg/MdePkg.dec | 6 ++
>>>>> Mde= Pkg/Library/BaseLib/BaseLib.inf | 11 +--
>>>>>= MdePkg/Include/Library/BaseLib.h | 74
>>>>> += ++++++++++++-------
>>>>> MdePkg/Library/BaseL= ib/SafeString.c | 4 +-
>>>>> MdePkg/MdePkg.uni= | 6 ++
>>>>> 5 files changed, 71 insertions(+= ), 30 deletions(-)
>>>>> 
>>>>> --
>>>>> 2.21.0 (Apple Git-122.2)
>>= >>> 
>>>>> <= br class=3D"">>>>>> -=3D-=3D-=3D-=3D-=3D-=3D
&= gt;>>>> Groups.io Links: You receive all = messages sent to
>>> this
>>>= >> group.
>>>>> 
>>>>> View/Reply On= line (#52837):
>>>>> https://edk2.groups.io/g/devel/messag= e/52837
>>>>> Mute This Topic:
>>> https://groups.io/mt/69401948/1643496
>&= gt;>>> Group Owner: devel+owner@edk2.groups.io
>>>>= > Unsubscribe: h= ttps://edk2.groups.io/g/devel/unsub
>>>&g= t;> [michael.d.kinney@intel.com]
>>>= >> -=3D-=3D-=3D-=3D-=3D-=3D
>>>> 
>>> 
>>> 
>>><= span class=3D"apple-converted-space"> 
>> 
> 

 
 <= o:p class=3D"">
 

--Apple-Mail=_34675231-9D0C-4ABC-839F-EB518144A384--