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

Replying as per Liming's request for this to be merged into edk2-stable202002.

On Mon, Feb 10, 2020 at 14:12, vit9696 <vit9696@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 <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: Type: text/html, Size: 8648 bytes --]