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

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