From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-40131.protonmail.ch (mail-40131.protonmail.ch [185.70.40.131]) by mx.groups.io with SMTP id smtpd.web11.7261.1581768150670151009 for ; Sat, 15 Feb 2020 04:02:31 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@protonmail.com header.s=default header.b=iKSumQns; spf=pass (domain: protonmail.com, ip: 185.70.40.131, mailfrom: vit9696@protonmail.com) Date: Sat, 15 Feb 2020 12:02:23 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=default; t=1581768148; bh=zY1ZhS+STZV2EgMHV5u5Y39dIUZIUHXgxF/hCTQSk0A=; h=Date:To:From:Cc:Reply-To:Subject:In-Reply-To:References: Feedback-ID:From; b=iKSumQnsIvK8gwC010u+SHKqxSR0ONCR4RHf/j0frPPQkLG/0hA/ibrfNbrZvcJEu ef7Kvh0WuM7Ynb9xof9FJySNoTOgt/EcdyoQU+S3GdPyG3D4d93dsvB1lelxvvdoHs Gty3Qb1ozVLKLmr+yTVXTOYTiArIB8BoP6fCdQ6g= To: Andrew Fish From: "Vitaly Cheptsov" Cc: devel@edk2.groups.io, Mike Kinney , "Gao, Liming" , "Gao, Zhichao" , =?UTF-8?Q?Marvin_H=C3=A4user?= , Laszlo Ersek Reply-To: vit9696 Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions Message-ID: In-Reply-To: <9ED72404-C505-4589-BA13-91C7437C36A0@protonmail.com> References: <20200103171242.63839-1-vit9696@protonmail.com> <2719704F-6DB4-444F-97FE-DBF71D39B698@apple.com> <9ED72404-C505-4589-BA13-91C7437C36A0@protonmail.com> Feedback-ID: p9QuX-L1wMgUm6nrSvNrf8juLupNs0VSnzXGVXuYDxlEahFdWtaedWDMB9zpwGDklGt7kzs1-RBc0cqz327Gcg==:Ext:ProtonMail MIME-Version: 1.0 X-Spam-Status: No, score=-0.7 required=7.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,FREEMAIL_REPLYTO_END_DIGIT,HTML_MESSAGE shortcircuit=no autolearn=disabled version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on mail.protonmail.ch X-Groupsio-MsgNum: 54499 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha256; boundary="---------------------81644aced9a039435484881dad91fe38"; charset=UTF-8 -----------------------81644aced9a039435484881dad91fe38 Cc: devel@edk2.groups.io, Mike Kinney , "Gao, Liming" , "Gao, Zhichao" , =?utf-8?Q?Marvin_H=C3=A4user?= , Laszlo Ersek Content-Type: multipart/alternative; boundary="Apple-Mail=_2E89BF18-3237-4631-A601-F7469673CF5F" Date: Sat, 15 Feb 2020 15:01:18 +0300 From: vit9696 In-Reply-To: <9ED72404-C505-4589-BA13-91C7437C36A0@protonmail.com> Message-Id: Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\)) References: <20200103171242.63839-1-vit9696@protonmail.com> <2719704F-6DB4-444F-97FE-DBF71D39B698@apple.com> <9MoxdJ8nml1WYF1JViU2YrwRwoNEP8Bu7Ax0wps7KatP9SiQyoly1LXHuozlAZdVMTsXi1-rcHMh1E5UZN8VyQ==@protonmail.conversationid> <9ED72404-C505-4589-BA13-91C7437C36A0@protonmail.com> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions To: Andrew Fish X-Mailer: Apple Mail (2.3608.60.0.2.5) --Apple-Mail=_2E89BF18-3237-4631-A601-F7469673CF5F Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 It seems that edk2.groups.io has hit the limits fo= r message size due to quotation levels (?), so I send another copy of my pr= evious message to have it visible from the web-interface. Andrew, It is ok, as we are all here for mutual benefit, no worries. But you are ri= ght that we indeed want a solution for BZ 2054[1] to land as early as possi= ble. It is great that after more than half a year :) we have some productiv= e discussion, so I am all open for it. * The compliant about a breaking change in DebugLib is honestly fair, and I= wonder whether we could avoid it. The reason it potentially breaks is the = introduction of DebugConstraintAssertEnabled function that will need to lan= d in every DebugLib implementation. I think we can look differently at this= problem. Currently functions like DebugAssertEnabled, DebugPrintEnabled, D= ebugClearMemoryEnabled, and so on are copy-pasted from one DebugLib to anot= her introducing almost 100 lines of exactly the same code every DebugLib im= plementation should write. It can be argued that one can implement these fu= nctions differently, but neither the description in DebugLib.h permits this= , nor I have seen any implementation doing it so far. I believe we should c= heck Pcd values directly in DebugLib.h header, which has a lot of pros incl= uding the backwards compatibility resolution: =E2=80=94 reduction of code copy-pasting. =E2=80=94 making compilers without LTO produce smaller binaries. =E2=80=94 making third-party DebugLib implementations more flexible and eas= ier to maintain. =E2=80=94 resolving the problem of third-party DebugLib implementations not= working with CONSTRAINT_ASSERT. To make it less intrusive we can provide an iterative approach: 1. Introduce Pcd checking macros like DEBUG_PRINT_ENABLED, DEBUG_ASSERT_ENA= BLED: #define DEBUG_PRINT_ENABLED() (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & D= EBUG_PROPERTY_DEBUG_PRINT_ENABLED) !=3D 0) 2. Switch DEBUG, ASSERT, and other related macros to use these new macros b= y replacing calls like DebugPrintEnabled() with macros like DEBUG_PRINT_ENA= BLED(). 3. Introduce DEBUG_CONSTRAINT_ASSERT_ENABLED and CONSTRAINT_ASSERT as discu= ssed previously except DebugConstraintAssertEnabled is now a macro. 4. Remove DebugPrintEnabled and other functions from EDK II DebugLib implem= entations and header. Step 4 can be done through deprecation phase. However, I think these functi= ons are not used anywhere but inside DebugLib implementations anyway, and t= he way they are structured should let still legacy third-party DebugLib imp= lementations still having these functions to compile fine even without the = immediate refactoring. * The point of AssertMacros and changing the world is valid, but to be fran= k with you I have an opinion that we should not incorporate this experience= in EDK II. I do not know if these macros are used in Mac EFI code, as my o= nly experience with them was during XNU kernelspace programming, but I do n= ot have a memory of them bringing enough benefit. Basically, macro construc= tions that cannot be easily expressed with normal C functions, especially o= nes changing control flow with something like a goto, are unintuitive and o= vercomplicated. They make the code harder to read, especially to those not = familiar with the rest of the codebase, for little to no reason. While ther= e certainly may be some positive sides in these macros, like improved struc= tural separation by introducing different categories of checks, I am afraid= we do not need them as is. Even if we do implement something close in the = future, I believe they should rather exist in a separate library and be an = opt-in for newly contributed code. * The point of caller/callee control on the constraint violation is slightl= y tricky. We have already spent some time trying to nail it down, and I bel= ieve there is a good level of logic that can be split in two parts: 1. Assertion type choose. Which to write ASSERT or CONSTRAINT_ASSERT? You can rely on a simple thought experiment here. Does failing this asserti= on make the function work in the conditions it was not meant to be? I.e. wi= ll the function break or crash or will some other spec/doc-compliant implem= entations of this potentially do. When the answer is affirmative, it is an = ASSERT, otherwise it is a CONSTRAINT_ASSERT. Examples in the BZ mentioning = SafeString give a good grasp of the use cases. 2. Assertion handling choice. When do we want ASSERT or CONSTRAINT_ASSERT t= o be enabled? This is more likely what your question was about. First is that CONSTRAINT_= ASSERTs only make sense to enable in DEBUG builds where ASSERTs are enabled= . So the question comes to whether DEBUG builds should behave the same way = as RELEASE builds functionality-wise or not. Let=E2=80=99s take an another = simple thought experiment: should we halt in a build when external untruste= d data theoretically exists and is not valid? If the answer is yes, then CO= NSTRAINT_ASSERT should be enabled in a DEBUG build, otherwise no. Basically= you enable CONSTRAINT_ASSERTs when you e.g. debug your code trying to find= a problem (let=E2=80=99s say a typo) and disable CONSTRAINT_ASSERTs when y= ou give a DEBUG build to the end user for a more or less long-term usage. Sorry for the long read, but I hope I answered all the questions in conside= ration. Best wishes, Vitaly --Apple-Mail=_2E89BF18-3237-4631-A601-F7469673CF5F Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
It seems t= hat edk2.groups.io ha= s hit the limits for message size due to quotation levels (?), so I send an= other copy of my previous message to have it visible from the web-interface= .

Andrew,
It is ok, as we are all here for mutual benefit, no worries= . But you are right that we indeed want a solution for BZ 2054[1] to land a= s early as possible. It is great that after more than half a year :) we hav= e some productive discussion, so I am all open for it.

<= /div>
* The compliant ab= out a breaking change in DebugLib is honestly fair, and I wonder whether we= could avoid it. The reason it potentially breaks is the introduction of De= bugConstraintAssertEnabled function that will need to land in every DebugLi= b implementation. I think we can look differently at this problem. Currentl= y functions like DebugAssertEnabled, DebugPrintEnabled, Debu= gClearMemoryEnabled, and so on are copy-pasted from one DebugLib to another= introducing almost 100 lines of exactly the same code every DebugLib imple= mentation should write. It can be argued that one can implement these funct= ions differently, but neither the description in DebugLib.h permits this, n= or I have seen any implementation doing it so far. I believe we should chec= k Pcd values directly in DebugLib.h header, which has a lot of pros includi= ng the backwards compatibility resolution:

=E2=80= =94 reduction of code copy-pasting.
=E2=80=94 making compilers without = LTO produce smaller binaries.
=E2=80=94 making third-party DebugLib imp= lementations more flexible and easier to maintain.
=E2=80=94 resolving = the problem of third-party DebugLib implementations not working with CONSTR= AINT_ASSERT.

To make it less intrusive we can provi= de an iterative approach:
1. Introduce Pcd checking macros like DEBUG_P= RINT_ENABLED, DEBUG_ASSERT_ENABLED:
#define DEBUG_PRINT_ENABLED() = (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_PRINT_= ENABLED) !=3D 0)
2. Switch DEBUG, ASSERT, and other related macros to u= se these new macros by replacing calls like DebugPrintEnabled() with m= acros like DEBUG_PRINT_ENABLED().
3. Introduce DEBUG_CONSTRAINT_ASSERT_= ENABLED and CONSTRAINT_ASSERT as discussed previously except DebugConstrain= tAssertEnabled is now a macro.
4. Remove <= /span>DebugPrintEnabled and other functi= ons from EDK II DebugLib implementations and header.

Step 4 can be done through deprecation phase. However, I think= these functions are not used anywhere but inside DebugLib implementations = anyway, and the way they are structured should let still legacy third-party= DebugLib implementations still having these functions to compile fine even= without the immediate refactoring.

* The point of AssertMacros an= d changing the world is valid, but to be frank with you I have an opinion t= hat we should not incorporate this experience in EDK II. I do not know if t= hese macros are used in Mac EFI code, as my only experience with them was d= uring XNU kernelspace programming, but I do not have a memory of them bring= ing enough benefit. Basically, macro constructions that cannot be easily ex= pressed with normal C functions, especially ones changing control flow with= something like a goto, are unintuitive and overcomplicated. They make the = code harder to read, especially to those not familiar with the rest of the = codebase, for little to no reason. While there certainly may be some positi= ve sides in these macros, like improved structural separation by introducin= g different categories of checks, I am afraid we do not need them as is. Ev= en if we do implement something close in the future, I believe they should = rather exist in a separate library and be an opt-in for newly contributed c= ode.

* The point of caller/callee control on the co= nstraint violation is slightly tricky. We have already spent some time tryi= ng to nail it down, and I believe there is a good level of logic that can b= e split in two parts:

1. Assertion type choose. Whi= ch to write ASSERT or CONSTRAINT_ASSERT?

You can re= ly on a simple thought experiment here. Does failing this assertion make th= e function work in the conditions it was not meant to be? I.e. will the function break or crash or will some other s= pec/doc-compliant implementations of this potentially do. When the answer i= s affirmative, it is an ASSERT, otherwise it is a CONSTRAINT_ASSERT. Exampl= es in the BZ mentioning SafeString give a good grasp of the use cases.
=
2. Assertion handling choice. When do we want ASSERT or= CONSTRAINT_ASSERT to be enabled?

This is more like= ly what your question was about. First is that CONSTRAINT_ASSERTs only make= sense to enable in DEBUG builds where ASSERTs are enabled. So the question= comes to whether DEBUG builds should behave the same way as RELEASE builds= functionality-wise or not. Let=E2=80=99s take an another&= nbsp;simple thought experiment: should we halt in a build when external unt= rusted data theoretically exists and is not valid? If the answer is yes, th= en CONSTRAINT_ASSERT should be enabled in a DEBUG build, otherwise no. = ;Basically you enable CONSTRAINT_ASSERTs when you e.g. debug your code trying t= o find a problem (let=E2=80=99s say a typo) and disable CONSTRAINT_ASSERTs when you give a DEBUG build to the end user fo= r a more or less long-term usage.

Sorry for the long read, but I hope I answer= ed all the questions in consideration.

Best wishes,
Vitaly
--Apple-Mail=_2E89BF18-3237-4631-A601-F7469673CF5F-- -----------------------81644aced9a039435484881dad91fe38 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: ProtonMail wsBmBAEBCAAQBQJeR93LCRBPsoxt7Hy0xQAKCRBPsoxt7Hy0xXujCACU4bo6 K6H8sUc5Rhq/CwnaY3DLFgYlbJCRMDAo+za+9eBRh+C8rAnOvvmVH+AAO2FC ajvWwlus2PcCsbdA4jFo0F7+tumeXd053U/3aGTRbR1mjsBbIfk0eWiAvPXn AXCEBdIcOxt1EP3LXuvhnhtPixiBaqx1Y8m+RsNmFBel07Xw+093OzfLuM4V ow12zUXq6wKTmuaABBW5ed2ET69zOwcMa5scWIA1pu3ips9f78miF+RZYHiE caamkjqUm2WG5+Ce4UOX0oQh2aYJjMKkisGKMDEzCSD4npOx+lVRwsPtx19E Pncg6yHQ/1ZSws3JeNAQh4B/OkAgjWDNBhiP =Qb44 -----END PGP SIGNATURE----- -----------------------81644aced9a039435484881dad91fe38--