Michael, The suggested changes make sense to me. I will prepare the patch in the next days. I guess the only question left is whether disabling assertions also disables constraint assertions. I think this should be the case for backwards compatibility, despite being slightly unintuitive.
Best, Vitaly On Sat, Feb 15, 2020 at 01:50, Kinney, Michael D <michael.d.kin...@intel.com> wrote: > 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 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 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 > PcdDebugPropertyMask to disable these types of asserts. > > This approach does require all the DebugLib implementations to be updated > with the new DebugConstraintAssertDisabled() API. > > Mike > > From: vit9696 <vit9...@protonmail.com> > Sent: Friday, February 14, 2020 9:38 AM > To: Kinney, Michael D <michael.d.kin...@intel.com> > Cc: devel@edk2.groups.io; Gao, Liming <liming....@intel.com>; Gao, Zhichao > <zhichao....@intel.com>; Marvin Häuser <marvin.haeu...@outlook.com>; Laszlo > Ersek <ler...@redhat.com> > 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 unintentional > 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 > PcdDebugPropertyMask instead of PcdAssertOnSafeStringConstraints. > > - Introduce DebugAssertConstraintEnabled 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 there are no objections, > I can submit the patch in the beginning of next week. > > Best wishes, > > Vitaly > >> 14 февр. 2020 г., в 20:00, Kinney, Michael D <michael.d.kin...@intel.com> >> написал(а): >> >> Vitaly, >> >> I want to make sure a feature PCD can be used to disable ASSERT() behavior >> in more than just safe string functions in BaseLib. >> >> Can we consider changing the name and description of >> PcdAssertOnSafeStringConstraints 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 <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.kin...@intel.com>; Gao, Liming >> <liming....@intel.com>; Gao, Zhichao <zhichao....@intel.com>; >> devel@edk2.groups.io >> Cc: Marvin Häuser <marvin.haeu...@outlook.com>; Laszlo Ersek >> <ler...@redhat.com> >> 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-stable202002. >> >> On Mon, Feb 10, 2020 at 14:12, vit9696 <vit9...@protonmail.com> wrote: >> >>> Hello, >>> >>> 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. >>> >>> Best wishes, >>> Vitaly >>> >>>> 27 янв. 2020 г., в 12:47, vit9696 <vit9...@protonmail.com> написал(а): >>>> >>>> >>>> Hi Mike, >>>> >>>> Any progress with this? We would really benefit from this landing in the >>>> next stable release. >>>> >>>> Best, >>>> Vitaly >>>> >>>>> 8 янв. 2020 г., в 19:35, Kinney, Michael D <michael.d.kin...@intel.com> >>>>> написал(а): >>>>> >>>>> >>>>> Hi Vitaly, >>>>> >>>>> 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 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. >>>>> >>>>> Thanks, >>>>> >>>>> Mike >>>>> >>>>>> -----Original Message----- >>>>>> From: devel@edk2.groups.io <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.kin...@intel.com> >>>>>> Cc: devel@edk2.groups.io >>>>>> Subject: 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 Applications. We >>>>>> do not want to disable ASSERT’s 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 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. >>>>>> >>>>>> 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. >>>>>> >>>>>> 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. >>>>>> >>>>>> Best wishes, >>>>>> Vitaly >>>>>> >>>>>>> 6 янв. 2020 г., в 21:28, Kinney, Michael D >>>>>> <michael.d.kin...@intel.com> написал(а): >>>>>>> >>>>>>> >>>>>>> Hi Vitaly, >>>>>>> >>>>>>> Is the use case for UEFI Applications? >>>>>>> >>>>>>> There is a different mechanism to disable all >>>>>> ASSERT() >>>>>>> statements within a UEFI Application. >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> Mike >>>>>>> >>>>>>> >>>>>>>> -----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 >>>>>>>> To: devel@edk2.groups.io >>>>>>>> Subject: [edk2-devel] [PATCH v3 0/1] Add PCD to >>>>>> disable >>>>>>>> safe string constraint assertions >>>>>>>> >>>>>>>> REF: >>>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2054 >>>>>>>> >>>>>>>> Requesting for merge in edk2-stable202002. >>>>>>>> >>>>>>>> Changes since V1: >>>>>>>> - Enable assertions by default to preserve the >>>>>> original >>>>>>>> behaviour >>>>>>>> - Fix bugzilla reference link >>>>>>>> - Update documentation in BaseLib.h >>>>>>>> >>>>>>>> Vitaly Cheptsov (1): >>>>>>>> MdePkg: Add PCD to disable safe string constraint >>>>>>>> assertions >>>>>>>> >>>>>>>> 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(-) >>>>>>>> >>>>>>>> -- >>>>>>>> 2.21.0 (Apple Git-122.2) >>>>>>>> >>>>>>>> >>>>>>>> -=-=-=-=-=-= >>>>>>>> [Groups.io](http://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 Owner: devel+ow...@edk2.groups.io >>>>>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>>>>>>> [michael.d.kin...@intel.com] >>>>>>>> -=-=-=-=-=-= >>>>>>> >>>>>> >>>>>> >>>>>> >>>>> >>>> >> >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54481): https://edk2.groups.io/g/devel/message/54481 Mute This Topic: https://groups.io/mt/69401948/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
