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: 5914 bytes --]

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 <vit9696@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.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 --]