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

["Vitaly Cheptsov" <vit9696@protonmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5251 bytes --]

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.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]
>>>> -=-=-=-=-=-=
>>> 
>> 
>> 
>> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 489 bytes --]