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 20200103171242.63839-2-vit9696@protonmail.com">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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55413): https://edk2.groups.io/g/devel/message/55413 Mute This Topic: https://groups.io/mt/71711587/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
