Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

["Michael D Kinney" <michael.d.kinney@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.kinney@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.kinney@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+owner@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> >> [michael.d.kinney@intel.com]
> >> -=-=-=-=-=-=
> >
> 
> 
>