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 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 (#53026): https://edk2.groups.io/g/devel/message/53026 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] -=-=-=-=-=-=-=-=-=-=-=-
