> On Mar 4, 2020, at 10:18 AM, Laszlo Ersek <ler...@redhat.com> wrote: > > On 03/04/20 18:53, Andrew Fish wrote: >> >> >>> On Mar 4, 2020, at 5:33 AM, Laszlo Ersek <ler...@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 >>> 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. >>> >> >> 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55443): https://edk2.groups.io/g/devel/message/55443 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] -=-=-=-=-=-=-=-=-=-=-=-
