Re: Disabling safe string constraint assertions

["Laszlo Ersek" <lersek@redhat.com>]

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.

(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.

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

Just my two cents, because I've been asked to comment.

Thanks
Laszlo