Re: Disabling safe string constraint assertions

[cheptsov@ispras.ru]

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

Hi everyone,

So, I believe that by now we mostly agreed to let the original proposition land as a short-term solution. We end up with:

1. A PCD condition within SAFE_STRING_COSTRAINT_CHECK macro.
2. Make this condition evaluate to TRUE by default (i.e. ASSERT).
3. Update documentation for BaseLib functions to include the information about this behaviour.

The only thing in question is whether this should be a separate PCD or an extra bit in PcdDebugPropertyMask. I believe that we almost agreed on two things:

1. Adding an extra bit to PcdDebugPropertyMask is cleaner.
2. Extending DebugLib interface with a new function is not a good idea.

Therefore I suggest:

1.Add #define DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40.
2. Create header-only macros to replace functions like DebugAssertEnabled. We can then use these macros in new code and deprecate the original functions.
3. Enable DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit in MdePkg by default.

I will submit the new version of the patch soon unless there is an immediate opposing opinion.

Best wishes,
Vitaly

>> On 4 Mar 2020, at 21:57, Andrew Fish <afish@apple.com> wrote:
> 
> 
>> On Mar 4, 2020, at 10:18 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>> 
>>> On 03/04/20 18:53, Andrew Fish wrote:
>>> 
>>> 
>>>> On Mar 4, 2020, at 5:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> 
>>>>> On 03/03/20 22:12, Vitaly Cheptsov wrote:
>>>>> Hello,
>>>>> 
>>>>> I am creating a new thread for this issue, as for some reason half of my messages do not display in the web-interface and some of e-mail clients in the previous one. It seems that somebody sent broken HTML code to groups.io <http://groups.io/>, and it did not like to be quoted. It will be quite nice if all the messages sent are plain-text to avoid such issues in the future.
>>>>> 
>>>>> Back to the issue, we want to make constraint assertions (SAFE_STRING_CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as they break our ability to use some of BaseLib interfaces to verify untrusted user data in UEFI Applications. The background of this problem is covered in a bugzilla[1], and was also extensively discussed in the last proposed patch discussion thread[2]. We want the solution to the problem to land in the next stable tag: edk2-stable202005, and can submit the patch.
>>>>> 
>>>>> Currently there is no clear decision on what approach to take, as there are three different proposals to implement:
>>>>> 
>>>>> 1) Andrew Fish’s approach with C11-like constraint handler[3].
>>>>> 2) Marvin Häuser’s approach with return codes and constraint assertions being moved to the caller side[4].
>>>>> 3) My approach suggesting DebugLib interface simplification, permitting us to resolve the problem and keep backwards compatibility[5].
>>>>> 
>>>>> My personal opinion is that:
>>>>> 
>>>>> — Marvin’s way is very interesting, but it would require implementing code on the caller side basically for every call, and this would requires extra programming effort. It may also be uneasy to teach people how to do this right, and this does not strictly provide a good solution for situations, where nesting is more than 2 (i.e. when user code calls library code, which itself calls library code). Because of these downsides I am afraid it is impractical to adopt it in EDK II.
>>>>> — Andrew’s way is reasonable from the C standard point of view, but it does not work quite right with reentrancy and I am not positive we need the dynamic handling, especially because of DEBUG/RELEASE build confusion. I explained this in my previous e-mail, which I attached to the end of this message as quote, since groups.io <http://groups.io/> ate the previous send. Other than that, I would say that I can implement this approach if there is no other option.
>>>>> — My own approach makes sense, as it reduces the amount of code in every DebugLib and actually simplifies the development in the future. I believe currently this is the most reasonable route we can take, and suggest other parties to consider it.
>>>>> 
>>>>> Since the three of us each have their own suggestions, I ask Mike and others to rejoin our discussions and give their opinions quickly, so that we can choose, perhaps vote, and proceed with the implementation. Thanks for your dedication.
>>>> 
>>>> I support the original proposal given in:
>>>> 
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2054#c0
>>>> 
>>>> also known as the v3 posting:
>>>> 
>>>> * [edk2-devel] [PATCH v3 1/1]
>>>> MdePkg: Add PCD to disable safe string constraint assertions
>>>> 
>>>> https://edk2.groups.io/g/devel/message/52838
>>>> http://mid.mail-archive.com/20200103171242.63839-2-vit9696@protonmail.com
>>>> 
>>>> With the following tweak: I think we should rather introduce a new bit
>>>> in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD.
>>> 
>>> Laszlo,
>>> 
>>> +1
>>> 
>>>> (Note that both PcdDebugPropertyMask and the propsed new PCD support
>>>> precisely the following PCD access methods: PcdsFixedAtBuild,
>>>> PcdsPatchableInModule.)
>>>> 
>>>> I don't feel too strongly about this part (i.e., whether we introduce a
>>>> new PCD, or a new bit in PcdDebugPropertyMask), as long as we make sure
>>>> that the access method can *never* become dynamic.
>>>> 
>>>> I do prefer quite strongly the original proposal, at a higher level.
>>>> 
>>>> Here's why I support the original proposal:
>>>> 
>>>> - In a brand new code base (or brand new set of APIs), I would fight
>>>> tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up to
>>>> the caller to check return values. Also, additional wrapper
>>>> functions/macros can be offered *on top* of the base APIs that
>>>> internalize the ASSERT()s for special use cases.
>>>> 
>>>> But this ship has sailed, for edk2.
>>> 
>>> Crazy idea but we could add versions of APIs that don't do the checking as new APIs? That would make life easier for untrusted code, Runtime Drivers, code running on the AP, and other places that the ASSERTs could have side effects. 
>> 
>> I'm not sure I understand "new APIs" correctly.
>> 
>> If it's an entirely new set of APIs, then I think it's not good for
>> Vitaly -- I understand Vitaly wants to use the existent codebase without
>> much churn, just with the ASSERTs removed.
>> 
>> If you mean a new library instance, that's different. If the package
>> owners do not mind the code duplication that's inherent in creating such
>> an alternative library instance, then sure, it can work for me.
>> 
> 
> Laszlo,
> 
> Sorry the new APIs would be in addition, and the simplest way to cleanly solve your concern that I share about the caller not having control. 
> 
> I don't want a long term goal to block getting the short term features needed by the community.
> 
> Thanks,
> 
> Andrew Fish
> 
>>> 
>>>> - I'm unfriendly towards callbacks. They make the behavior of code
>>>> implicit rather than explicit, and implicit is more difficult to reason
>>>> about. IMO callbacks should be considered a last resort only.
>>> 
>>> Not my 1st choice either as I agree with your first point about given the caller control. I was just wanted to go through the exercise of what it would take to make it like C11, and give the caller control. 
>>> 
>>>> Just my two cents, because I've been asked to comment.
>>> 
>>> Thanks for helping out.
>> 
>> Thanks!
>> Laszlo

[-- Attachment #2: Type: text/html, Size: 19618 bytes --]