From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from nwk-aaemail-lapp01.apple.com (nwk-aaemail-lapp01.apple.com [17.151.62.66]) by mx.groups.io with SMTP id smtpd.web09.2027.1582156507343886926 for ; Wed, 19 Feb 2020 15:55:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=jTTpQ9wb; spf=pass (domain: apple.com, ip: 17.151.62.66, mailfrom: afish@apple.com) Received: from pps.filterd (nwk-aaemail-lapp01.apple.com [127.0.0.1]) by nwk-aaemail-lapp01.apple.com (8.16.0.27/8.16.0.27) with SMTP id 01JNqBX2021115; Wed, 19 Feb 2020 15:55:05 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=hb+l6BNF9gFrXMDDu4BPLJ6rWyBY8zXI7Wy1ONrQzMY=; b=jTTpQ9wbWEvfwPHgOyZMptErG8APesDLTWYUmHJ1AcyowTPFpxy8wyoxuUhwp0sTEsaX ptWQ0QOvggkjDwbQR8qiQAEdnNc7/X10SKbmolp1+QYSqAQTFcJ/TvweBSB+PBqS6Wqd OlUjP+QUOxiaUPlwB0whyua6bUMztm8eL0TLOAPI6GC6B+/LqjHU+inZDTGClaa4jpY6 TzpaYyMeoGn8aimOLKCRN0l1Z71vOdT5I4skKjkZfK6+q9Dm7KP8rD8+GB96rDhwl0cK Iht0En+rn7IgM0tdXmqOEP+mSfHsErs0VjuZn/jauYl+TKiR/LCX4E/CTh4lWaU55rE4 rg== Received: from rn-mailsvcp-mta-lapp04.rno.apple.com (rn-mailsvcp-mta-lapp04.rno.apple.com [10.225.203.152]) by nwk-aaemail-lapp01.apple.com with ESMTP id 2y6gf534ur-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 19 Feb 2020 15:55:05 -0800 Received: from nwk-mmpp-sz10.apple.com (nwk-mmpp-sz10.apple.com [17.128.115.122]) by rn-mailsvcp-mta-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) with ESMTPS id <0Q5Z00B2L3RT79B0@rn-mailsvcp-mta-lapp04.rno.apple.com>; Wed, 19 Feb 2020 15:55:05 -0800 (PST) Received: from process_milters-daemon.nwk-mmpp-sz10.apple.com by nwk-mmpp-sz10.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0Q5Z00L003P3FE00@nwk-mmpp-sz10.apple.com>; Wed, 19 Feb 2020 15:55:05 -0800 (PST) X-Va-A: X-Va-T-CD: 5ba617998c06f710a1787aac0b6b40c1 X-Va-E-CD: ce6e59d19e41f9f978af9e8b590c623d X-Va-R-CD: 4ad19a74e8868beceae55112f7f03bd9 X-Va-CD: 0 X-Va-ID: ba018517-39b7-44b9-9066-86ad3096bdb2 X-V-A: X-V-T-CD: 5ba617998c06f710a1787aac0b6b40c1 X-V-E-CD: ce6e59d19e41f9f978af9e8b590c623d X-V-R-CD: 4ad19a74e8868beceae55112f7f03bd9 X-V-CD: 0 X-V-ID: 02606fd1-718b-4240-b6b1-996725127f04 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2020-02-19_07:,, signatures=0 Received: from [17.235.22.242] by nwk-mmpp-sz10.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0Q5Z00K7M3RPW590@nwk-mmpp-sz10.apple.com>; Wed, 19 Feb 2020 15:55:04 -0800 (PST) Sender: afish@apple.com From: "Andrew Fish" Message-id: MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions Date: Wed, 19 Feb 2020 15:55:01 -0800 In-reply-to: Cc: Mike Kinney , vit9696 , "Gao, Liming" , "Gao, Zhichao" , =?utf-8?Q?Marvin_H=C3=A4user?= , Laszlo Ersek To: devel@edk2.groups.io, mhaeuser@outlook.de References: <20200103171242.63839-1-vit9696@protonmail.com> <492A300B-1CED-4FF0-98F8-20D1E116DCC0@protonmail.com> <2719704F-6DB4-444F-97FE-DBF71D39B698@apple.com> <3D7EB7E9-9B94-40AD-A0E9-BFDEB787F6DA@apple.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2020-02-19_07:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_BE7CCF4B-2763-4DA7-902A-782C2E74B5CF" --Apple-Mail=_BE7CCF4B-2763-4DA7-902A-782C2E74B5CF Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Feb 17, 2020, at 12:26 AM, Marvin H=C3=A4user w= rote: >=20 > Good day Andrew, >=20 > First of all, thank you very much for putting this amount of thought=20 > into the situation. I definitely agree with the problem you see, but I= =20 > could also live with Vitaly's proposal. However, I think you are=20 > overcomplicating the situation a little. So, we do agree the caller=20 > needs control of the behaviour, however your proposal needs "support"=20 > from both the caller (choose the handler) and the callee (call the=20 > handler) and is neither thread-safe nor event-safe as Vitaly already=20 > pointed out. Fixing these problems would only worsen the complexity=20 > situation in my opinion. Well EFI does not have Threads but it does have Events. I agree if an Even= t handler changes the constraint handler it would need to restore it so my = proposal is missing an API.=20 CONSTRAINT_HANDLER * GetConstraintHandler ( VOID ) { return gActiveConstraintHandler; } You an always use the standard EFI=20 It is important to remember that all the EFI images (drivers/applications)= are statically linked and they carry a unique instance of the Debug (or ne= w Constraint) lib. So in your driver or App is very self contained. Also a = lot of the events are phase related callbacks, and since there are no threa= ds your drivers main thread is only really running when your driver, or app= , dispatches. A lot of the callbacks are via protocols your driver publishe= s, but they have strict TPL rules so you can prevent reentrancy in general.= So there is a lot more determinism in EFI vs. generic threaded coding.=20 >=20 > Diverging from both of your concepts entirely and keeping it very=20 > simple, what keeps us from simply ASSERTing based on return value? We=20 > could introduce a CONSTRAINT_ASSERT or similar, but it would be=20 > completely different from Vitaly's proposal. It would be a caller macro= =20 > to ASSERT based on a pre-defined list of return statuses. > To achieve this, we would order all statuses by types (classes). At the= =20 > moment, I can only think of two: "environmental" and "constraint". For= =20 > example, "Out Of Resources" would be environmental and "Invalid=20 > Parameter" would be constraint. We could define environment statuses=20 > explicitly (as it is less likely new environment statuses are=20 > introduced) and define constraint statuses as "not environmental" (to=20 > silently support new constraint statuses, however of course it is a=20 > little error-prone with forwards-compatibility). As a bonus, it forces= =20 > developers to use truly appropiate error codes and correctly propagate= =20 > them from callees for this to work. :) > This solution would be backwards-compatible in a compilation sense as it= = =20 > can simply be a new macro, however for its possible complexity (switch= =20 > cases) I might actually prefer a function. But it would not be=20 > backwards-compatible with the current functionality, which none of the= =20 > "caller-based" concepts can be anyway (the caller needs explicit code). >=20 It is an interesting idea to add granularity, but at the end of the knowle= dge of the correct behavior of the lower level library code really lives in= code that calls this. I will admit I can see some value to making RETURN_I= NVALID_PARAMETER different than RETURN_BUFFER_TOO_SMALL.=20 I kind of went down the C11 path (thanks for mentioning the event issue) b= ut there are other ways to empower the caller.=20 We could let the caller decide the behavior. That implies passing CHECK_DE= FAULT (PCD), CHECK_ASSERT, or CHECK_NULL.=20 Then keep backward compatibility.=20 #define StrCpyS(Destination,DestMax,Source) StrCpuSC(Destination, DestMax,= Source, CHECK_DEFAULT) But that seems like a lot of thrash to the BaseLib and a lot of new magic = to teach developers.=20 I would guess that the most common usage of my library would be to turn Co= nstraint Checking off in the entire driver or app and then handle the error= s manually.=20 On driver/app entry do: SetConstraintHandler (IgnoreConstraintHandler); Then handle the errors you care about.=20 Status =3D StrCpyS (Destination,DestMax,Source); if (EFI_ERROR (Status)) { ASSERT_EFI_ERROR (Status); return Status; } or=20 Status =3D StrCpyS (Destination,DestMax,Source); if (Status =3D=3D RETURN_INVALID_PARAMETER) { ASSERT_EFI_ERROR (Status); return Status; } At the end of the day I've code my driver and I know the rules I coded to = and I want predictable behavior.=20 I can see a Si vendor developing with ASSERT Constraint off and then when = the customer turns it on stuff starts randomly breaking.=20 > However, CONSTRAINT_ASSERT might actually be too specific. Maybe we want= = =20 > something like "ASSERT_RETURN" and have the classes "ASSERT_CONSTRAINT"= =20 > and "ASSERT_ENVIRONMENT" (bitfield, like with DEBUG). The reason for=20 > this is embedded systems where the environment is trusted/known and the= =20 > firmware is supposed to be well-matched. Think of a SoC with soldered=20 > RAM - the firmware can very well make assumption about memory capacity= =20 > (considering usage across all modules in the worst case) and might=20 > actually want ASSERTs on something like "Out Of Resources" because it's= =20 > supposed to be impossible within the bounds of this specific design.=20 > It's possible this would need a new PCD's involvement, for example to=20 > tell DxeCore whether to ASSERT on environmental aspects too. There could= = =20 > be an argument-less macro that uses PCD config as base, and an=20 > argument-based macro that uses only the parameters. Of course this=20 > cannot cover everyone's very specific preferences as it's a per-module= =20 > switch, but it's the best I can think of right now. >=20 > Something a bit unrelated now, but it would make this easier. You=20 > mentioned PeCoffLib as example, and I think it's a good one, however it= =20 > has its own return status type within the context[1] which I will most= =20 > definitely be getting rid of as part of my upcoming (in the far-ish=20 > future :) ) RFC. Could we expand RETURN_STATUS to have (very high?)=20 > reserved values for caller-defined error codes to have all RETURN_STATUS= = =20 > macros apply to those special return values too? We'd need to define=20 > them categorically as "constraint status", but I don't see how a library= = =20 > would declare a new environment status anyway. >=20 > Regarding MdePkgCompatability.dsc.inc, I think one can override library= =20 > classes from the inc by declaring them after the !include statement,=20 > please correct me if I'm wrong. If so, I strongly agree and think it=20 > should be the case for all packages, so one only overrides the defaults= =20 > when something specific (debugging, performance-optimized versions, ...)= = =20 > is required - easier to read, easier to maintain. The content is=20 > needed/there anyway as the libraries are declared in the package's own D= SC. >=20 Yes it the new .inc DSC file could be overridden by the platform DSC that = includes it.=20 I think this is a more general idea than something for this given patch. I= t is something we could make sure we update every stable tag or so, but I g= uess someone has to go 1st :). It would make it easier on platforms when th= ey update the edk2 version.=20 > It would be nice if you had comments regarding every aspect I just=20 > mentioned, it was just something coming to my mind this morning. Thanks= =20 > for the input so far, it's nice to see some movement now! >=20 Sorry for the delay=20 Thanks, Andrew Fish > Best regards, > Marvin >=20 > [1]=20 > https://github.com/tianocore/edk2/blob/f1d78c489a39971b5aac5d2fc8a39bfa9= 25c3c5d/MdePkg/Include/Library/PeCoffLib.h#L20-L31 > https://github.com/tianocore/edk2/blob/f1d78c489a39971b5aac5d2fc8a39bfa9= 25c3c5d/MdePkg/Include/Library/PeCoffLib.h#L159 >=20 > Am 16.02.2020 um 22:25 schrieb Andrew Fish via Groups.Io: >> Mike, >>=20 >> Sorry I don't think I totally understood what felt wrong to me, so I di= d=20 >> a bad job explaining my concerns. Also I don't think I was thinking=20 >> enough in the context of the C11 StdLib. >>=20 >> I think my concern is still the global scope of the constraint, even if= = =20 >> we tune it per module. For example the DXE Core can interact with=20 >> PE/COFF images and other data that could have indirectly come from the= =20 >> disk. So conceptually you may want to ASSERT on some constraints and no= t=20 >> on others. I think that is why I ratholed on expanding the error=20 >> handling macros as that was more in the vein of having the caller deal= =20 >> with it, so that is kind of like what you would do pre C11. Also the=20 >> DebugLib is probably one of the MdePkg Libs that has the most instances= = =20 >> floating around, so I think we should change it in a non backwards way= =20 >> very carefully. >>=20 >> So after reading up on the C11 implementation of Constraints I think my= = =20 >> alternate proposal is for us to add a ConstraintLib modeled after C11= =20 >> vs. updating the DebugLib. This would solve the 2 things that made me= =20 >> uncomfortable: 1) Extending the DebugLib API; 2) Giving the caller=20 >> control of the ASSERT behavior. It would still have the down side of=20 >> breaking builds as the BaseLib would get a new dependency, so we could= =20 >> talk about adding these functions to the DebugLib as the cost of=20 >> replicating code. >>=20 >> C11 defines constraint_handler_t and set_constraint_handler_s as a way= =20 >> for the caller to configure the behavior for bounds checked functions. = I=20 >> think that is the behavior I prefer. So if we are going to make a chang= e=20 >> that impacts DebugLib compatibility I just want to make sure we have a= =20 >> conversation about all the options. My primary goal is we have the=20 >> conversation, and if folks don't agree with me that is fine at least we= = =20 >> talked about it. >>=20 >> What I'm thinking about is as simply exposing an API to control the=20 >> Constraint handler like C11. This could be done via an ConstrainLib or= =20 >> extending the DebugLib. >>=20 >> The basic implementation of the lib would look like: >>=20 >> typedef >> VOID >> (EFIAPI *CONSTRAINT_HANDLER) ( >> IN CONST CHAR8 *FileName, >> IN UINTN LineNumber, >> IN CONST CHAR8 *Description, >> IN EFI_STATUS Status >> ); >>=20 >>=20 >> // Default to AssertConstraintHandler to make it easier to implement=20 >> Base and XIP libs. >> // We could have a PCD that also sets the default handler in a Lib=20 >> Constructor. The default handler is implementation defined in C11. >> CONSTRAINT_HANDLER gDefaultConstraintHandler =3D AssertConstraintHandle= r; >> CONSTRAINT_HANDLER gActiveConstraintHandler =3D gDefaultConstraintHandl= er; >>=20 >> BOOLEAN >> EFIAPI >> ConstraintAssertEnabled ( >> VOID >> ) >> { >> return (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) &=20 >> DEBUG_PROPERTY_DEBUG_CONSTRAINT_ENABLED) !=3D 0); >> } >>=20 >> EFI_STATUS >> EFIAPI >> SetConstraintHandler ( >> IN CONSTRAINT_HANDLER Handler >> ) >> { >> if (Handler =3D=3D NULL) { >> gActiveConstraintHandler =3D gDefaultConstraintHandler; >> } else { >> gActiveConstraintHandler =3D Handler; >> } >> } >>=20 >> VOID >> AssertConstraintHandler ( >> IN CONST CHAR8 *FileName, >> IN UINTN LineNumber, >> IN CONST CHAR8 *Description, >> IN EFI_STATUS Status >> ) >> { >> if (ConstraintAssertEnabled ()) { >> DEBUG ((EFI_D_ERROR, "\Constraint ASSERT (Status =3D %r): ", Statu= s)); >> DebugAssert (FileName, LineNumber, Description) >> } >>=20 >> return; >> } >>=20 >> VOID >> IgnoreConstraintHandler ( >> IN CONST CHAR8 *FileName, >> IN UINTN LineNumber, >> IN CONST CHAR8 *Description, >> IN EFI_STATUS Status >> ) >> { >> return; >> } >>=20 >> We could add macros for the code in the lib to call: >>=20 >> #define CONSTRAINT_CHECK(Expression, Status) \ >> do { \ >> if (!(Expression)) { \ >> gActiveConstraintHandler (__FILE__, __LINE__, Expression, Status)= ; \ >> return Status; \ >> } \ >> } while (FALSE) >>=20 >> #define CONSTRAINT_REQUIRE(Expression, Status, Label) \ >> do { \ >> if (!(Expression)) { \ >> gActiveConstraintHandler (__FILE__, __LINE__, Expression, Status)= ; \ >> goto Label; \ >> } \ >> } while (FALSE) >>=20 >>=20 >> As a caller we have now have control: >> EFI_STATUS Status; >> CHAR16 Dst[2]; >>=20 >> SetConstraintHandler (IgnoreConstraintHandler); >> Status =3D StrCpyS (Dst, sizeof (Dst), L"Too Long"); >> Print (L"Dst =3D%s (%r)\n", Dst, Status); >>=20 >> SetConstraintHandler (AssertConstraintHandler); >> StrCpyS (Dst, sizeof (Dst), L"Too Long"); >>=20 >> Thanks, >>=20 >> Andrew Fish >>=20 >> PS Since I'm on a crazy idea roll another idea would be to add a=20 >> MdePkgCompatability.dsc.inc file that could be used to future proof=20 >> adding dependent libs to existing MdePkg libs. So a platform could=20 >> include this .DSC and that would give them the default library mapping= =20 >> to keep code compiling. It will only work after other platforms start= =20 >> including it, but after that it would give default mappings for=20 >> dependent libs. >>=20 >> In our above example we could have added this and then existing builds= =20 >> that included MdePkgCompatability.dsc.inc would keep compiling. >>=20 >> [LibraryClasses] >>=20 >> DebugConstraintLib|MdePkg/Library/DebugConstraintLib/DebugConstraintLib= .inf >>=20 >>=20 >>=20 >>> On Feb 15, 2020, at 11:38 AM, Michael D Kinney=20 >>> >> wrote: >>>=20 >>> Andrew, >>> I do not think of this as a globally scoped PCD.It can be set in=20 >>> global scope in DSC.But is can also be set to values based on module= =20 >>> type or for specific modules.In the case of the safe string functions,= = =20 >>> I think there is a desire to disable the constraint asserts when=20 >>> building a UEFI App or UEFI Driver and implement those modules to=20 >>> handle the error return values.Enabling the constraint asserts for PEI= = =20 >>> Core, DXE Core, SMM/MM Core, PEIM, DXE, SMM/MM modules makes sense to= =20 >>> find incorrect input to these functions from modules that can=20 >>> guarantee the inputs would never return an error and catch these as=20 >>> part of dev/debug/validation builds. >>> I would not expect disabling on a module by module basis to be common. >>> I think the rule for API implementations is to only use=20 >>> CONSTRAINT_ASSERT() for conditions that are also checked and return an= = =20 >>> error or fail with predicable behavior that allows the system to=20 >>> continue to function.ASSERT() is for conditions that the systemcan=20 >>> notcontinue. >>> Best regards, >>> Mike >>> *From:*afish@apple.com >=20 >>> >> >>> *Sent:*Friday, February 14, 2020 10:27 PM >>> *To:*vit9696 <= mailto:vit9696@protonmail.com >> >>> *Cc:*devel@edk2.groups.io >; Kinney,=20 >>> Michael D =20 >>> = >>; Gao, Liming=20 >>> >>; Gao, Zhichao=20 >>> >>; Marvin H=C3=A4user=20 >>> >>;=20 >>> Laszlo Ersek >> >>> *Subject:*Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe=20 >>> string constraint assertions >>> Vitaly, >>> Sorry after I sent the mail I realized it may come across as me=20 >>> asking you to do work and that was not my intent. >>> I will point out though that a non backward compatible change to=20 >>> something as fundamental as the DebugLib is a very big deal. I've got= =20 >>> a few different custom implementations that would break with this=20 >>> change as Mike proposed. Given breaking every one's debug lib is such= =20 >>> a big deal maybe it is something that we should do as a long term plan= = =20 >>> vs. some incremental fix. So my intent was to start a conversation=20 >>> about what else we might want to change if we are going to break the= =20 >>> world. The only think worse than breaking the world is breaking the=20 >>> world frequently. >>> I'm also a little worried that we are taking things that are today=20 >>> locally scoped like SAFE_STRING_CONSTRAINT_CHECK and=20 >>> SAFE_PRINT_CONSTRAINT_CHECK and making them global constructs. I think= = =20 >>> the way others have dealt with things like this is to make them be=20 >>> DEBUG prints vs. ASSERTs. Also even something as simple as=20 >>> SAFE_STRING_CONSTRAINT_CHECK could be called from code that wants=20 >>> ASSERT and CONSTRAINT_ASSERT behavior. It is not clear to me that the= =20 >>> low level code knows the right thing to do in a global sense even if= =20 >>> there is a PCD. It almost seems like we should have wrappers for the= =20 >>> Safe string functions that implement the behavior you want as a=20 >>> caller. I'm not sure about that, but it seems like it is worth talking= = =20 >>> about? >>> Thanks, >>> Andrew Fish >>>=20 >>>=20 >>> On Feb 14, 2020, at 7:31 PM, vit9696 >>> >> wr= ote: >>> Hi Andrew, >>> While your suggestions look interesting, I am afraid they are not >>> particularly what we want to cover with this discussion at the mome= nt. >>> Making assertions go through DEBUG printing functions/macros is >>> what we have to strongly disagree about. Assertions and debug >>> prints are separate things configurable by separate PCDs. We >>> should not mix them. Introducing constraint assertions is a >>> logical step forward in understanding that different software >>> works in different environments. >>>=20 >>> * There are normal, or, as I call them, invariant >>> assertions (e.g. preconditions), for places where the function >>> cannot work unless the assertion is satisfied. This is where >>> we ASSERT. >>> * There are constraint assertions, which signalise that bad data >>> came through the function, even though the function was called >>> from a trusted source. This is where we call CONSTRAINT_ASSERT. >>>=20 >>> The thing we need is to have the latter separable >>> and configurable, because not every piece of software works in a >>> trusted environment. Other than that, constraint assertions, when >>> enabled, are not anyhow different from normal assertions in the >>> sense of action taken. Assertions have configurable breakpoints >>> and deadloops, and DEBUG prints go through a different route in >>> DebugLib that may cause entirely different effects. For example, >>> we halt execution upon printing to DEBUG_ERROR in our DebugLib >>> even in release builds. >>>=20 >>> =3DTo make it clear, currently I plan to add the following interfac= e: >>> #define CONSTRAINT_ASSERT(Expression) \ >>> do { \ >>> if (DebugConstraintAssertEnabled ()) { \ >>> if (!(Expression)) { \ >>> _ASSERT (Expression); \ >>> ANALYZER_UNREACHABLE (); \ >>> } \ >>> } \ >>> } while (FALSE) >>> with DebugConstraintAssertEnabled implemented as >>> (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & >>> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED | >>> DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED) =3D=3D >>> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) >>> Your suggestion with require macros looks interesting indeed, but >>> I believe it is in fact parallel to this discussion. The change we >>> discuss introduces a new assertion primitive =E2=80=94 constraint >>> assertions, while REQUIRE macros are mostly about advanced syntax >>> sugar and higher level assertion primitives on top of existing >>> ones. Perhaps we can have this and make a good use of it, >>> especially given that it brought some practical benefit in Apple, >>> but I would rather discuss this later once constraint assertions >>> are merged into EDK II tree. >>> Best wishes, >>> Vitaly >>> On Sat, Feb 15, 2020 at 03:02, Andrew Fish >>> >> wrote: >>>=20 >>>=20 >>>=20 >>> On Feb 14, 2020, at 2:50 PM, Michael D Kinney >>> >>> >> wrote: >>> Hi Vitaly, >>> I agree that this proposal makes a lot of sense. We >>> recently added a new assert type called STATIC_ASSERT() >>> for assert conditions that can be tested at build time. >>> A new assert type for checks that can be removed and the >>> API still guarantees that it fails gracefully with a >>> proper return code is a good idea. Given we have >>> STATIC_ASSERT(), how about naming the new macro >>> CONSTRAINT_ASSERT()? >>> We also want the default to be enabled. The current use of >>> bit 0x40 inPcdDebugPropertyMask is always clear, so we >>> want the asserts enabled when 0x40 is clear. We can change >>> the name of the define bit to >>> DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED so bit 0x40 needs >>> to be set inPcdDebugPropertyMaskto disable these types of >>> asserts. >>> This approach does require all theDebugLibimplementations >>> to be updated with the newDebugConstraintAssertDisabled() A= PI. >>>=20 >>> Mike, >>> If you wanted to be backward compatible you could just >>> use DebugAssertEnabled () but in place of _ASSERT() use >>> _CONSTRAINT_ASSERT >>> #define _ASSERT(Expression) DebugAssert (__FILE__, __LINE__, >>> #Expression) >>> #define _CONSTRAINT_ASSERT(Expression) DebugPrint >>> (DEBUG_ERROR, "ASSERT %a(%d): %a\n",, __FILE__, __LINE__, >>> #Expression) >>> Not as elegant as the non backward compatible change, but I >>> thought I'd throw it out there. >>> There are some ancient Apple C ASSERT macros [AssertMacros.h] >>> that also have the concept of require. Require includes an >>> exception label (goto label). It is like a CONSTRAINT_ASSERT() >>> but with the label. On release builds the DEBUG prints are >>> skipped. >>> So we could do something like: >>> EFI_STATUS Status =3D EFI_INVALID_PARAMETER; >>> REQUIRE(Arg1 !=3D NULL, ErrorExit); >>> REQUIRE(Arg2 !=3D NULL, ErrorExit); >>> REQUIRE(Arg3 !=3D NULL, ErrorExit); >>> ErrorExit: >>> return Status; >>> There is another form that allows an ACTION (a statement to >>> execute. So you could have: >>> EFI_STATUS Status; >>> REQUIRE_ACTION(Arg1 !=3D NULL, ErrorExit, Status =3D >>> EFI_INVALID_PARAMETER); >>> REQUIRE_ACTION(Arg2 !=3D NULL, ErrorExit, Status =3D >>> EFI_INVALID_PARAMETER); >>> REQUIRE_ACTION(Arg3 !=3D NULL, ErrorExit, Status =3D >>> EFI_INVALID_PARAMETER); >>> ErrorExit: >>> return Status; >>> If you use CONSTRAINT_ASSERT(); >>> if (Arg1 =3D=3D NULL || Arg2 =3D=3D NULL || Arg3 =3D=3D NULL)= { >>> CONSTRAINT_ASSERT (Arg1 !=3D NULL); >>> CONSTRAINT_ASSERT (Arg2 !=3D NULL); >>> CONSTRAINT_ASSERT (Arg3 !=3D NULL); >>> return EFI_INVALID_PARAMETER; >>> } >>> I'd note error processing args on entry is the simplest case. >>> In a more complex case when cleanup is required the goto >>> label is more useful. >>> I guess we could argue for less typing and more symmetry and >>> say use ASSERT, CONSTRAINT, and REQUIRE. I guess you could add >>> an ASSERT_ACTION too. >>> The AssertMacros.h versions also support _quiet (skip the >>> print) and _string (add a string to the print) so you end up wi= th: >>> REQUIRE >>> REQUIRE_STRING >>> REQUIRE_QUIET >>> REQUIRE_ACTION >>> REQUIRE_ACTION_STRING >>> REQUIRE_ACTION_QUIET >>> We could also end up with >>> CONSTRAINT >>> CONSTRAINT_STRING >>> CONSTRAINT_QUIET >>> I think the main idea behind _QUIET is you can silence things >>> that are too noisy, and you can easily make noise things show >>> up by removing the _QUIET to debug. >>> I'd thought I throw out the other forms for folks to think >>> about. I'm probably biased as I used to looking at code and >>> seeing things like require_action_string(Arg1 !=3D NULL, >>> ErrorExit, Status =3D EFI_INVALID_PARAMETER, "1st Arg1 check"); >>> Thanks, >>> Andrew Fish >>> PS The old debug macros had 2 versions of CONSTRAINT check and >>> verify. The check version was compiled out on a release build, >>> the verify version always does the check and just skips the >>> DEBUG print. >>>=20 >>> Mike >>> *From:*vit9696 >>> >> >>> *Sent:*Friday, February 14, 2020 9:38 AM >>> *To:*Kinney, Michael D >>> >> >>> *Cc:*devel@edk2.groups.io >; >>> Gao, Liming >>> >= >; Gao, Zhichao >>> >>; >>> Marvin H=C3=A4user >>> >>; Laszlo Ersek >>> >> >>> *Subject:*Re: [edk2-devel] [PATCH v3 0/1] Add PCD to >>> disable safe string constraint assertions >>> Michael, >>> Generalising the approach makes good sense to me, but we >>> need to make an obvious distinguishable difference between: >>> - precondition and invariant assertions (i.e. assertions, >>> where function will NOT work if they are violated) >>> - constraint asserts (i.e. assertions, which allow us to >>> spot unintentional behaviour when parsing untrusted data, >>> but which do not break function behaviour). >>> As we want to use this outside of SafeString, I suggest >>> the following: >>> - Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 >>> bit for PcdDebugPropertyMask instead >>> of PcdAssertOnSafeStringConstraints. >>> - Introduce DebugAssertConstraintEnabled DebugLib function >>> to check for DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED. >>> - Introduce ASSERT_CONSTRAINT macro, that will assert only >>> if DebugConstraintAssertEnabled returns true. >>> - Change SafeString ASSERTS >>> in SAFE_STRING_CONSTRAINT_CHECK to ASSERT_CONSTRAINTs. >>> - Use ASSERT_CONSTRAINT in other places where necessary. >>>=20 >>> I believe this way lines best with EDK II design. If there >>> are no objections, I can submit the patch in the beginning >>> of next week. >>>=20 >>> Best wishes, >>> Vitaly >>>=20 >>> 14 =D1=84=D0=B5=D0=B2=D1=80. 2020 =D0=B3., =D0=B2 20:00= , Kinney, Michael D >>> >>> >> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >>> Vitaly, >>> I want to make sure a feature PCD can be used to >>> disable ASSERT() behavior in more than just safe >>> string functions inBaseLib. >>> Can we consider changing the name and description >>> ofPcdAssertOnSafeStringConstraintsto be more generic, >>> so if we find other lib APIs, the name will make sense? >>> Maybe something like:PcdEnableLibraryAssertChecks? >>> Default is TRUE. Can set to FALSE in DSC file to >>> disable ASSERT() checks. >>> Thanks, >>> Mike >>> *From:*devel@edk2.groups.io >>> > >>> >>*On Behalf Of*Vitaly >>> Cheptsov via Groups.Io >>> *Sent:*Friday, February 14, 2020 3:55 AM >>> *To:*Kinney, Michael D >>> >>; Gao, Liming >>> >>; >>> Gao, Zhichao >>> >>;devel@edk2.groups.io >>> > >>> *Cc:*Marvin H=C3=A4user >>> >>; Laszlo Ersek >>> >> >>> *Subject:*Re: [edk2-devel] [PATCH v3 0/1] Add PCD to >>> disable safe string constraint assertions >>> Replying as per Liming's request for this to be merged >>> into edk2-stable202002. >>> On Mon, Feb 10, 2020 at 14:12, vit9696 >>> >>> >> wrote: >>>=20 >>> Hello, >>>=20 >>> It has been quite some time since we submitted the >>> patch with so far no negative response. As I >>> mentioned previously, my team will strongly >>> benefit from its landing in EDK II mainline. Since >>> it does not add any regressions and can be viewed >>> as a feature implementation for the rest of EDK II >>> users, I request this to be merged upstream in >>> edk2-stable202002. >>>=20 >>> Best wishes, >>> Vitaly >>>=20 >>>> 27 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 12:47, vit9696 >>> >>> >> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >>>>=20 >>>>=20 >>>> Hi Mike, >>>>=20 >>>> Any progress with this? We would really benefit >>> from this landing in the next stable release. >>>>=20 >>>> Best, >>>> Vitaly >>>>=20 >>>>> 8 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 19:35, Kinney, Michael D >>> >>> >> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >>>>>=20 >>>>>=20 >>>>> Hi Vitaly, >>>>>=20 >>>>> Thanks for the additional background. I would like >>>>> a couple extra day to review the PCD name and >>> the places >>>>> the PCD might potentially be used. >>>>>=20 >>>>> If we find other APIs where ASSERT() behavior >>> is only >>>>> valuable during dev/debug to quickly identify >>> misuse >>>>> with trusted data and the API provides predicable >>>>> return behavior when ASSERT() is disabled, then >>> I would >>>>> like to have a pattern we can potentially apply >>> to all >>>>> these APIs across all packages. >>>>>=20 >>>>> Thanks, >>>>>=20 >>>>> Mike >>>>>=20 >>>>>> -----Original Message----- >>>>>> From:devel@edk2.groups.io >>> > >>> >> On >>>>>> Behalf Of Vitaly Cheptsov via Groups.Io >>>>>> Sent: Monday, January 6, 2020 10:44 AM >>>>>> To: Kinney, Michael D >>> >>> >> >>>>>> Cc:devel@edk2.groups.io >>> > >>>>>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add >>> PCD to >>>>>> disable safe string constraint assertions >>>>>>=20 >>>>>> Hi Mike, >>>>>>=20 >>>>>> Yes, the primary use case is for UEFI >>> Applications. We >>>>>> do not want to disable ASSERT=E2=80=99s completely, as >>>>>> assertions that make sense, i.e. the ones >>> signalising >>>>>> about interface misuse, are helpful for debugging. >>>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> 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. >>>>>>=20 >>>>>> Best wishes, >>>>>> Vitaly >>>>>>=20 >>>>>>> 6 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 21:28, Kinney, Michael = D >>>>>> >>> >> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >>>>>>>=20 >>>>>>>=20 >>>>>>> Hi Vitaly, >>>>>>>=20 >>>>>>> Is the use case for UEFI Applications? >>>>>>>=20 >>>>>>> There is a different mechanism to disable all >>>>>> ASSERT() >>>>>>> statements within a UEFI Application. >>>>>>>=20 >>>>>>> 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. >>>>>>>=20 >>>>>>> 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. >>>>>>>=20 >>>>>>> Mike >>>>>>>=20 >>>>>>>=20 >>>>>>>> -----Original Message----- >>>>>>>> From: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 >>>>>>>>=20 >>>>>>>> REF: >>>>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054 >>>>>>>>=20 >>>>>>>> Requesting for merge in edk2-stable202002. >>>>>>>>=20 >>>>>>>> Changes since V1: >>>>>>>> - Enable assertions by default to preserve the >>>>>> original >>>>>>>> behaviour >>>>>>>> - Fix bugzilla reference link >>>>>>>> - Update documentation in BaseLib.h >>>>>>>>=20 >>>>>>>> Vitaly Cheptsov (1): >>>>>>>> MdePkg: Add PCD to disable safe string >>> constraint >>>>>>>> assertions >>>>>>>>=20 >>>>>>>> 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(-) >>>>>>>>=20 >>>>>>>> -- >>>>>>>> 2.21.0 (Apple Git-122.2) >>>>>>>>=20 >>>>>>>>=20 >>>>>>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>>>>>> Groups.io >Links: You >>> receive all messages sent to >>>>>> this >>>>>>>> group. >>>>>>>>=20 >>>>>>>> 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 >>> >] >>>>>>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>=20 >>>>=20 >>>=20 >>=20 >>=20 >=20 >=20 --Apple-Mail=_BE7CCF4B-2763-4DA7-902A-782C2E74B5CF Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8


On Feb 17, 2020, at 12:26 AM, Marvin = H=C3=A4user <mhaeuser@= outlook.de> wrote:

Good day Andrew,

First of all, thank yo= u very much for putting this amount of thought 
= into the situation. I definitely agree with the problem you see, but I 
could also live with Vitaly's proposal. However, I = think you are 
overcomplicating the situation a = little. So, we do agree the caller&nb= sp;
needs contro= l of the behaviour, however your proposal needs "support" 
from both the caller (choose the handler) and the callee (cal= l the 
handler) and is neither thread-safe nor e= vent-safe as Vitaly already 
pointed out. Fixing= these problems would only worsen the complexity 
situation in my opinion.

Well EFI does not have Threa= ds but it does have Events. I agree if an Event handler changes the constra= int handler it would need to restore it so my proposal is missing an API.&n= bsp;

CONSTRAINT_HANDLER *
GetConstraintHandler (
  VOID
  )
{
  return gActiveConstraintHandler;
}

You an always use the stand= ard EFI 

It = is important to remember that all the EFI images (drivers/applications) are= statically linked and they carry a unique instance of the Debug (or new Co= nstraint) lib. So in your driver or App is very self contained. Also a lot = of the events are phase related callbacks, and since there are no threads y= our drivers main thread is only really running when your driver, or app, di= spatches. A lot of the callbacks are via protocols your driver publishes, b= ut they have strict TPL rules so you can prevent reentrancy in general. So = there is a lot more determinism in EFI vs. generic threaded coding. 


Diverging from both of you= r concepts entirely and keeping it very 
simple,= what keeps us from simply ASSERTing based on return value? We 
could introduce a CONSTRAINT_ASSERT or similar, but it w= ould be 
completely different from Vitaly's pr= oposal. It would be a caller macro&nb= sp;
to ASSERT ba= sed on a pre-defined list of return statuses.
To achieve this, we would order all statuses by type= s (classes). At the 
moment, I can only think of= two: "environmental" and "constraint". For 
exa= mple, "Out Of Resources" would be environmental and "Invalid 
Parameter" would be constraint. We could define environment s= tatuses 
explicitly (as it is less likely new = environment statuses are =
introduced) and define= constraint statuses as "not environmental" (to 
silently support new constraint statuses, however of course it is a 
little error-prone with forwards-compatibility). As a= bonus, it forces =
developers to use truly appro= piate error codes and correctly propagate 
the= m from callees for this to work. :)
This solution would be backwards-compatible in a compilation = sense as it 
can simply be a new macro, however = for its possible complexity (switch&n= bsp;
cases) I mi= ght actually prefer a function. But it would not be 
backwards-compatible with the current functionality, which none of t= he 
"caller-based" concepts can be anyway (the c= aller needs explicit code).

It is an interesting idea to add granularity, but at the end of= the knowledge of the correct behavior of the lower level library code real= ly lives in code that calls this. I will admit I can see some value to maki= ng RETURN_INVALID_PARAMETER different than RETURN_BUFFER_TOO_SMAL= L. 

I kind of went down the C11 pa= th (thanks for mentioning the event issue) but there are other ways to empo= wer the caller. 

We could let the = caller decide the behavior. That implies passing CHECK_DEFAULT (PCD), CHECK= _ASSERT, or CHECK_NULL. 

Then keep= backward compatibility. 
#define StrCpyS(Destination,DestMa= x,Source) S= trCpuSC(Destination, DestMax, Source, CHECK_DEFAULT)

But that seems like a lot of thrash to the BaseLib and a lot = of new magic to teach developers. 

I would guess that the most common usage of my library would be to turn Co= nstraint Checking off in the entire driver or app and then handle the error= s manually. 

On driver/app entry d= o:

SetConstraintHandler (IgnoreConstrai= ntHandler);

Then handle the errors you = care about. 

Status =3D StrCpyS (D= estination,DestMax,Source);
if (EFI_ERROR (Status)) {
<= span class=3D"Apple-tab-span" style=3D"white-space:pre"> ASSERT_EFI_= ERROR (Status);
return Status;
}

<= div>or 

Status =3D StrCpyS (D= estination,DestMax,Source);
if (Status =3D=3D RETURN_INVALID_PARA= METER) {
ASSERT_EFI_ERROR (Status);
return Status;
}

At the end of the day I've cod= e my driver and I know the rules I coded to and I want predictable behavior= . 

I can see a Si vendor developin= g with ASSERT Constraint off and then when the customer turns it on stuff s= tarts randomly breaking. 

However, CONSTRAINT_ASSERT might actual= ly be too specific. Maybe we want&nbs= p;
something lik= e "ASSERT_RETURN" and have the classes "ASSERT_CONSTRAINT" 
and "ASSERT_ENVIRONMENT" (bitfield, like with DEBUG). The rea= son for 
this is embedded systems where the en= vironment is trusted/known and the&nb= sp;
firmware is = supposed to be well-matched. Think of a SoC with soldered 
RAM - the firmware can very well make assumption about memory= capacity 
(considering usage across all modules= in the worst case) and might 
actually want ASS= ERTs on something like "Out Of Resources" because it's 
supposed to be impossible within the bounds of this specific design= . 
It's possible this would need a new PCD's inv= olvement, for example to =
tell DxeCore whether t= o ASSERT on environmental aspects too. There could 
be an argument-less macro that uses PCD config as base, and an 
argument-based macro that uses only the parameters. = Of course this cannot cover everyone's very spe= cific preferences as it's a per-module 
switch, = but it's the best I can think of right now.

Something a bit= unrelated now, but it would make this easier. You 
mentioned PeCoffLib as example, and I think it's a good one, however= it 
has its own return status type within the c= ontext[1] which I will most 
definitely be getti= ng rid of as part of my upcoming (in the far-ish 
future :) ) RFC. Could we expand RETURN_STATUS to have (very high?) 
reserved values for caller-defined error codes to ha= ve all RETURN_STATUS 
macros apply to those spec= ial return values too? We'd need to define 
the= m categorically as "constraint status", but I don't see how a library 
would declare a new environment status anyway.

Regarding MdePkgCompatability.dsc.inc, I think one can overri= de library 
classes from the inc by declaring th= em after the !include statement, = ;
please correct= me if I'm wrong. If so, I strongly agree and think it 
should be the case for all packages, so one only overrides the defa= ults 
when something specific (debugging, perfor= mance-optimized versions, ...) <= /span>
is required - ea= sier to read, easier to maintain. The content is 
needed/there anyway as the libraries are declared in the package's own DS= C.


Yes it the ne= w .inc DSC file could be overridden by the platform DSC that includes it.&n= bsp;

I think this is a more general ide= a than something for this given patch. It is something we could make sure w= e update every stable tag or so, but I guess someone has to go 1st :). It w= ould make it easier on platforms when they update the edk2 version. 




= It would be nice if you had comments regarding every aspect I just 
mentioned, it was just something coming to my mi= nd this morning. Thanks <= /span>
for the input so far, i= t's nice to see some movement now!


Sorry for the delay 

Thanks,

Andrew Fish

Bes= t regards,
Marvin

[1] =
https= ://github.com/tianocore/edk2/blob/f1d78c489a39971b5aac5d2fc8a39bfa925c3c5d/= MdePkg/Include/Library/PeCoffLib.h#L20-L31
https://github.com/tianocore/edk2/blo= b/f1d78c489a39971b5aac5d2fc8a39bfa925c3c5d/MdePkg/Include/Library/PeCoffLib= .h#L159

Am 16.02.2020 um 22:25 schrieb Andrew Fish via Groups.= Io:
Mike,<= br class=3D"">
Sorry I don't think I totally understood what = felt wrong to me, so I did 
a bad job explaining my concerns. Also I don't think I was= thinking 
= enough in the context of the C11 StdLib.

I thi= nk my concern is still the global scope of the constraint, even if 
we tune it per mod= ule. For example the DXE Core can interact with 
PE/COFF images and other data that co= uld have indirectly come from the&nbs= p;
disk. So conceptually you may want to ASSERT on som= e constraints and not on others. I think that is why I ratholed on expanding the erro= r 
handling= macros as that was more in the vein of having the caller deal 
with it, so that is= kind of like what you would do pre C11. Also the 
DebugLib is probably one of the Mde= Pkg Libs that has the most instances&= nbsp;
floating around, so I think we should change it = in a non backwards way very carefully.

So after reading = up on the C11 implementation of Constraints I think my 
alternate proposal is for us t= o add a ConstraintLib modeled after C11 
vs. updating the DebugLib. This would solve t= he 2 things that made me =
uncomfortable: 1) Extending the DebugLib API; 2) Giving the = caller 
con= trol of the ASSERT behavior. It would still have the down side of 
breaking builds as = the BaseLib would get a new dependency, so we could 
talk about adding these functions= to the DebugLib as the cost of =
replicating code.

C11 de= fines constraint_handler_t and set_constraint_handler_s as a way<= span class=3D"Apple-converted-space"> 
for the ca= ller to configure the behavior for bounds checked functions. I 
think that is the b= ehavior I prefer. So if we are going to make a change 
that impacts DebugLib compatibi= lity I just want to make sure we have a 
conversation about all the options. My primar= y goal is we have the conversation, and if folks don't agree with me that is fine at = least we 
t= alked about it.

What I'm thinking about is as = simply exposing an API to control the=  
Constraint handler like C11. This could be done= via an ConstrainLib or <= br class=3D"">extending the DebugLib.

The basi= c implementation of the lib would look like:

t= ypedef
VOID
(EFIAPI *CONSTRAINT_HANDLER) (
  IN CONST= CHAR8  *FileName,
  IN UINTN             &nbs= p;   LineNumber,
  IN CONST CHAR8  *Description,
  IN EFI_STATUS    = ;  Status
 &n= bsp;);


// Default to Ass= ertConstraintHandler to make it easier to implement 
Base and XIP libs.
= // We could have a PCD that also sets the default handler in a Lib 
Constructor. The d= efault handler is implementation defined in C11.
CONSTRAINT_H= ANDLER gDefaultConstraintHandler =3D AssertConstraintHandler;
CONSTRAINT_HANDLER gActiveConstraintHandler =3D gDefaultConstra= intHandler;

BOOLEAN
EFIAPI
ConstraintAssertEnabled (
  VOID
  )
{
  = ;return (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & 
DEBUG_PROPERTY_DEBUG_CONSTRAI= NT_ENABLED) !=3D 0);
}

EFI_STATU= S
EFIAPI
SetConstraintHandler (
&= nbsp; IN CONSTRAINT_HANDL= ER Handler
  = )
{
  if (Handler =3D=3D NULL) {
   =  gActiveConstraintHandler= =3D gDefaultConstraintHandler;
  } else {
    gActiveConstraintHandler =3D H= andler;
  }
}

VOID
Asser= tConstraintHandler (
  IN CONST CHAR8  *FileName,
  IN UINTN       =           LineNumber,
  IN CONST CHAR8  *Descriptio= n,
  I= N EFI_STATUS      Status
  )
{
 &= nbsp;if (ConstraintAssertEnabled ()) {
     DE= BUG ((EFI_D_ERROR, "\Constraint ASSERT (Status =3D %r): ", Status));
     DebugAssert (FileName, LineNumber, De= scription)
  = }

 return;
}

VOID
IgnoreConstraintHandler (
  IN CONST CH= AR8  *FileName,
  IN UINTN               =   LineNumber,
  IN CONST CHAR8  *Description,
  IN EFI_STATUS     &= nbsp;Status
  = ;)
{
  return;
}

= We could add macros for the code in the lib to call:

#define CONSTRAINT_CHECK(Expression, Status)  \
&= nbsp; do { \
    if (!(Exp= ression)) { \
      gActiveConstraintHandler (__FILE__, __LINE__, = Expression, Status); \
      return Status; \
  &nbs= p; } \
&nbs= p; } while (FALSE)

#define CONSTRAINT_REQUIRE(Expression, Status, Label= )  \
  <= /span>do { \
    if (!(Expression)) { \
      gActiveConstraintHandler&n= bsp;(__FILE__, __LINE__, Expression, Status); \
   =   goto Label; = \
    } \
  } while (FALSE)


As a calle= r we have now have control:
  EFI_STATUS Status;
  CHAR16        = Dst[2];

  SetConstraintHandler (IgnoreConstraintHandler);
  Status =3D = StrCpyS (Dst, sizeof (Dst), L"Too Long");
  Print (L"Dst =3D%s (%r)\n",  = Dst, Status);

  SetConstraintHandler (AssertConstraintHandler);  StrCpy= S (Dst, sizeof (Dst), L"Too Long");

Thanks,
Andrew Fish

PS Sinc= e I'm on a crazy idea roll another idea would be to add a 
MdePkgCompatability.dsc.inc= file that could be used to future proof 
adding dependent libs to existing MdePkg lib= s. So a platform could include this .DSC and that would give them the default library= mapping 
t= o keep code compiling. It will only work after other platforms start 
including it, bu= t after that it would give default mappings for 
dependent libs.

In our above example we could have added this and then existing bui= lds 
that i= ncluded MdePkgCompatability.dsc.inc would keep compiling.
 [LibraryClasses]

DebugConstr= aintLib|MdePkg/Library/DebugConstraintLib/DebugConstraintLib.inf



On Feb 15, 2020, at 11:38 AM, Michael D Kinney 
<michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> = wrote:

Andrew,
I do not think of= this as a globally scoped PCD.It can be set in 
global scope in DSC.But is can also b= e set to values based on module =
type or for specific modules.In the case of the safe = string functions, 
I think there is a desire to disable the constraint asserts when 
building a U= EFI App or UEFI Driver and implement those modules to 
handle the error return values.= Enabling the constraint asserts for PEI 
Core, DXE Core, SMM/MM Core, PEIM, DXE, SMM/M= M modules makes sense to =
find incorrect input to these functions from modules that ca= n 
guarante= e the inputs would never return an error and catch these as 
part of dev/debug/validat= ion builds.
I would not expect disabling on a module by modul= e basis to be common.
I think the rule for API implementation= s is to only use 
CONSTRAINT_ASSERT() for conditions that are also checked and return = an 
error o= r fail with predicable behavior that allows the system to 
continue to function.ASSERT= () is for conditions that the systemcan 
notcontinue.
Best regards,
Mike
*From:*afish@apple.com <mailto:afish@apple.com<= /a>><afish@apple.com 
<mailto:afish@apple.com>>= ;
*Sent:*Friday, February 14, 2020 10:27 PM
*To= :*vit9696 <vit9696@= protonmail.com <mailto:vit9696@protonmai= l.com>>
*Cc:*devel@edk2.groups.io=  <mailto:= devel@edk2.groups.io>; Kinney,=  
Michael D <michael.d.kinney@intel.com 
<mailto:michael.d.kinney@intel.com>>= ;; Gao, Liming 
<liming.gao@in= tel.com <mailto:liming.gao@intel.com>>; Gao, Zhichao <= br class=3D""><zhich= ao.gao@intel.com <= mailto:zhichao.gao@inte= l.com>>; Marvin H=C3=A4user=  
<marvin.haeuser@outlook.com <mailto:marvin.haeuser@outlook.com>>; 
Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
*Subject:*Re: [e= dk2-devel] [PATCH v3 0/1] Add PCD to disable safe 
string constraint assertions
Vitaly,
Sorry after I sent the mail I realized it may= come  across as me =
asking you to do work and that was not my intent.
I will point out though that a non backward compatible change to 
something as = fundamental as the DebugLib is a very big deal. I've got 
a few different custom imple= mentations that would break with this=  
change as Mike proposed. Given breaking every o= ne's debug lib is such a big deal maybe it is something that we should do as a long t= erm plan 
v= s. some incremental fix. So my intent was to start a conversation 
about what else we = might want to change if we are going to break the 
world. The only think worse than br= eaking the world is breaking the = ;
world frequently.
I'm also a little wo= rried that we are taking things that are today 
locally scoped like SAFE_STRING_C= ONSTRAINT_CHECK and 
SAFE_PRINT_CONSTRAINT_CHECK and making them global constructs. I = think 
the = way others have dealt with things like this is to make them be 
DEBUG prints vs. AS= SERTs. Also even something as simple as 
SAFE_STRING_CONSTRAINT_CHECK could be called = from code that wants 
ASSERT and CONSTRAINT_ASSERT behavior. It is not clear to me tha= t the 
low = level code knows the right thing to do in a global sense even if 
there is a PCD. &nb= sp;It almost seems like we should have wrappers for the 
Safe string functions that im= plement the behavior you want as a&nb= sp;
caller. I'm not sure about that, but it seems like= it is worth talking 
about?
Thanks,
Andrew Fish


   On Feb 14, 2020, at = 7:31 PM, vit9696 <v= it9696@protonmail.com
   <mailto:vit9696@protonmail.com&g= t;> wrote:
   Hi Andrew,
&nbs= p;  While your suggestions look interesting, I am afraid the= y are not
   particularly what we want to= cover with this discussion at the moment.
  &= nbsp;Making assertions go through DEBUG printing functions/macros is   what we have to strongly disagree about. Ass= ertions and debug
   prints are separate thing= s configurable by separate PCDs. We
  &nb= sp;should not mix them. Introducing constraint assertions is a
   logical step forward in understanding that different so= ftware
   works in different environments.

     * There are normal,= or, as I call them, invariant
     =   assertions (e.g. preconditions), for places where the func= tion
       cannot work un= less the assertion is satisfied. This is where
  &n= bsp;    we ASSERT.
   &nbs= p; * There are constraint assertions, which signalise that bad data       came through the func= tion, even though the function was called
   &= nbsp;   from a trusted source. This is where we call CONSTRA= INT_ASSERT.

   The thing we nee= d is to have the latter separable
   and = configurable, because not every piece of software works in a
   trusted environment. Other than that, constraint a= ssertions, when
   enabled, are not anyhow dif= ferent from normal assertions in the
   sense = of action taken. Assertions have configurable breakpoints
&nb= sp;  and deadloops, and DEBUG prints go through a different = route in
   DebugLib that may cause = entirely different effects. For example,
  &nb= sp;we halt execution upon printing to DEBUG_ERROR in our DebugLib
   even in release builds.

   =3DTo make it clear, currently I plan to add the follo= wing interface:
   #define CONSTRAINT_ASSERT(E= xpression) \
   do { \
 &nb= sp; if (DebugConstraintAssertEnabled ()) { \
  = ; if (!(Expression)) { \
   _ASSERT (Expr= ession); \
   ANALYZER_UNREACHABLE (); \
   } \
   } \
   } while (FALSE)
   wi= th DebugConstraintAssertEnabled implemented as
  &n= bsp;(BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) &
 &nb= sp; DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED |
  &n= bsp;DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED) =3D=3D
 &n= bsp; DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED)
  &n= bsp;Your suggestion with require macros looks interesting indeed, but
   I believe it is in fact parallel to this discuss= ion. The change we
   discuss introduces a new= assertion primitive =E2=80=94 constraint
   a= ssertions, while REQUIRE macros are mostly about advanced syntax
   sugar and higher level assertion primitives on top= of existing
   ones. Perhaps we can have this= and make a good use of it,
   especially give= n that it brought some practical benefit in Apple,
 &nbs= p; but I would rather discuss this later once constraint assertions   are merged into EDK II tree.
&n= bsp;  Best wishes,
   Vitaly
   On Sat, Feb 15, 2020 at 03:02, Andrew Fish <afish@apple.com
   <mailt= o:afish@apple.com>> wrote:



         = ;  On Feb 14, 2020, at 2:50 PM, Michael D Kinney
&n= bsp;          <michael.d.kinney@intel.c= om
         =   <ma= ilto:michael.d.kinney@intel.com>> wrote:
 &nbs= p;         Hi Vitaly,
           I = agree that this proposal makes a lot of sense. We
  = ;         recently added a new= assert type called STATIC_ASSERT()
    &= nbsp;      for assert conditions that can be = tested at build time.
      &nb= sp;    A new assert type for checks that can be removed= and the
        &nbs= p;  API still guarantees that it fails gracefully with a
           pr= oper return code is a good idea. Given we have
  &n= bsp;        STATIC_ASSERT(), how ab= out naming the new macro
      =      CONSTRAINT_ASSERT()?
 &nbs= p;         We also want the de= fault to be enabled. The current use of
   &nb= sp;       bit 0x40 inPcdDebugPropertyMas= k is always clear, so we
      =      want the asserts enabled when 0x40 is clear. = We can change
        = ;   the name of the define bit to
  =          DEBUG_PROPERTY_CONTRA= INT_ASSERT_DISABLED so bit 0x40 needs
    = ;       to be set inPcdDebugPropertyMask= to disable these types of
      = ;     asserts.
   &nb= sp;       This approach does require all= theDebugLibimplementations
     &nb= sp;     to be updated with the newDebugConstraintA= ssertDisabled() API.

    &= nbsp;  Mike,
      &n= bsp;If you wanted to be backward compatible you could just
&n= bsp;      use DebugAssertEnabled () but = in place of _ASSERT() use
      = ; _CONSTRAINT_ASSERT
      = ; #define _ASSERT(Expression)  DebugAssert (__FILE__, __LINE__,       #Expression)
       #define _CONSTRAINT_A= SSERT(Expression)  DebugPrint
    &n= bsp;  (DEBUG_ERROR,  "ASSERT %a(%d): %a\n",, __FILE__, __LIN= E__,
       #Expression)       Not as elegant as th= e non backward compatible change, but I
   &nb= sp;   thought I'd throw it out there.
 &n= bsp;     There are some ancient Apple C ASSERT mac= ros [AssertMacros.h]
      &nbs= p; that also have the concept of require. Require includes an
       exception label (goto = ;label). It is like a CONSTRAINT_ASSERT()
   &= nbsp;   but with the label. On release builds the DEBUG prin= ts are
       skipped.
       So we could do somethi= ng like:
         EFI= _STATUS Status =3D  EFI_INVALID_PARAMETER;
  &= nbsp;      REQUIRE(Arg1 !=3D NULL, ErrorExit);
         REQUIRE(Arg2 != =3D NULL, ErrorExit);
      &n= bsp;  REQUIRE(Arg3 !=3D NULL, ErrorExit);
  &n= bsp;    ErrorExit:
   &nbs= p;     return Status;
   &= nbsp;   There is another form that allows an ACTION (a state= ment to
       execute. So= you could have:
       &n= bsp; EFI_STATUS Status;
      &= nbsp;  REQUIRE_ACTION(Arg1 !=3D NULL, ErrorExit, Status =3D
       EFI_INVALID_PARAMETER);         REQUIRE_ACTION= (Arg2 !=3D NULL, ErrorExit, Status =3D
   &nbs= p;   EFI_INVALID_PARAMETER);
   = ;      REQUIRE_ACTION(Arg3 !=3D NULL, ErrorExit, S= tatus =3D
       EFI_INVAL= ID_PARAMETER);
       Erro= rExit:
         retur= n Status;
       If you us= e CONSTRAINT_ASSERT();
      &n= bsp;  if (Arg1 =3D=3D NULL || Arg2 =3D=3D NULL || Arg3 =3D=3D NULL) {<= br class=3D"">          CONSTR= AINT_ASSERT (Arg1 !=3D NULL);
     &= nbsp;    CONSTRAINT_ASSERT (Arg2 !=3D NULL);
&= nbsp;         CONSTRAINT_ASSERT (Ar= g3 !=3D NULL);
       &nbs= p;  return EFI_INVALID_PARAMETER;
   &nbs= p;    }
     &nb= sp; I'd note error processing args on entry is the simplest case.
        In a more complex= case when cleanup is required the goto
   &nb= sp;   label is more useful.
   =     I guess we could argue for less typing and more sym= metry and
       say use A= SSERT, CONSTRAINT, and REQUIRE. I guess you could add
 &= nbsp;     an ASSERT_ACTION too.
&nbs= p;      The AssertMacros.h versions also supp= ort _quiet (skip the
      &nbs= p;print) and _string (add a string to the print) so you end up with:
       REQUIRE
&nb= sp;      REQUIRE_STRING
 &= nbsp;     REQUIRE_QUIET
  =      REQUIRE_ACTION
  &nbs= p;    REQUIRE_ACTION_STRING
  &= nbsp;    REQUIRE_ACTION_QUIET
  = ;     We could also end up with
&nbs= p;      CONSTRAINT
  =      CONSTRAINT_STRING
  &= nbsp;    CONSTRAINT_QUIET
  &nb= sp;    I think the main idea behind _QUIET is you can s= ilence things
       that = are too noisy, and you can easily make noise things show
&nbs= p;      up by removing the _QUIET to debug.       I'd thought I throw = out the other forms for folks to think
   &nbs= p;   about. I'm probably biased as I used to looking at code= and
       seeing things = like require_action_string(Arg1 !=3D NULL,
   =     ErrorExit, Status =3D EFI_INVALID_PARAMETER, "1st A= rg1 check");
       Thanks= ,
       Andrew Fish
       PS The old debug macros = had 2 versions of CONSTRAINT check and
   &nbs= p;   verify. The check version was compiled out on a release= build,
       the verify = version always does the check and just skips the
  =      DEBUG print.

&nb= sp;          Mike
           *F= rom:*vit9696 <vit96= 96@protonmail.com
      &nb= sp;    <mailto:vit9696@protonmail.com>>
 &nbs= p;         *Sent:*Friday, Febr= uary 14, 2020 9:38 AM
      &nb= sp;    *To:*Kinney, Michael D <michael.d.kinney@intel.com
           &l= t;mailto:michael.d= .kinney@intel.com>>
     &= nbsp;     *Cc:*devel@edk2.groups.io <mail= to:devel@edk2.groups.io>;
    &nbs= p;      Gao, Liming <liming.gao@intel.com
 =           <mailto:liming.gao@intel.com>= >; Gao, Zhichao
       =     <zhichao.gao@intel.com = ;<mailto:zhic= hao.gao@intel.com>>;
     =       Marvin H=C3=A4user <marvin.haeuser@outlook.com
          &nbs= p;<mailto:marvi= n.haeuser@outlook.com>>; Laszlo Ersek
  &= nbsp;        <lersek@redhat.com <mailto:lersek@redhat.com>>
   = ;        *Subject:*Re: [edk2-devel]= [PATCH v3 0/1] Add PCD to
     &nbs= p;     disable safe string constraint assertions          &n= bsp;Michael,
        =    Generalising the approach makes good sense to me, but we<= br class=3D"">          &= nbsp;need to make an obvious distinguishable difference between:
           - = precondition and invariant assertions (i.e. assertions,
 = ;          where function= will NOT work if they are violated)
    =        - constraint asserts (i.e. assert= ions, which allow us to
      &= nbsp;    spot unintentional behaviour when parsing untr= usted data,
        &= nbsp;  but which do not break function behaviour).
=            As we wan= t to use this outside of SafeString,  I suggest
&nb= sp;          the followin= g:
         &nbs= p; - Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40
           bi= t for PcdDebugPropertyMask instead
   &nb= sp;       of PcdAssertOnSafeStringC= onstraints.
        &= nbsp;  - Introduce DebugAssertConstraintEnabled DebugLib function=
          =  to check for DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED.
           - = Introduce ASSERT_CONSTRAINT macro, that will assert only
&nbs= p;          if Debug= ConstraintAssertEnabled returns true.
   =         - Change SafeString ASSERTS=
          =  in SAFE_STRING_CONSTRAINT_CHECK to ASSERT_CONSTRAINTs.
           = ;- Use ASSERT_CONSTRAINT in other places where necessary.

         = ;  I believe this way lines best with EDK II design. If there
          &nbs= p;are no objections, I can submit the patch in the beginning
=            of next w= eek.

       = ;    Best wishes,
    = ;       Vitaly

           &n= bsp;   14 =D1=84=D0=B5=D0=B2=D1=80. 2020 =D0=B3., =D0=B2 20:= 00, Kinney, Michael D
      &nb= sp;        <michael.d.kinney@intel.com
           &n= bsp;   <mailto:michael.d.kinney@intel.com>> =D0=BD=D0=B0=D0=BF=D0= = =B8=D1=81=D0=B0=D0=BB(=D0=B0):
     = ;          Vitaly,
           &= nbsp;   I want to make sure a feature PCD can be used to
          &nbs= p;    disable ASSERT() behavior in more than just safe<= br class=3D"">          &= nbsp;    string functions inBaseLib.
&nbs= p;            &= nbsp; Can we consider changing the name and description
=             &nb= sp;  ofPcdAssertOnSafeStringConstraintsto be more generic,
           &= nbsp;   so if we find other lib APIs, the name will make sen= se?
         &nb= sp;     Maybe something like:PcdEnableLibraryAsser= tChecks?
        &nbs= p;      Default is TRUE. Can set to FALSE in = DSC file to
        &= nbsp;      disable ASSERT() checks.
           &n= bsp;   Thanks,
     &= nbsp;         Mike
            =    *From:*devel@edk2.groups.io
      = ;         <mailto:devel@edk2.groups.io><<= a href=3D"mailto:devel@edk2.groups.io" class=3D"">devel@edk2.groups.io<= br class=3D"">          &= nbsp;    <mailto:devel@edk2.groups.io>>*On Behalf Of*Vitaly
           &= nbsp;   Cheptsov via Groups.Io
  &nb= sp;            = *Sent:*Friday, February 14, 2020 3:55 AM
   &n= bsp;           *To:*= Kinney, Michael D <michael.d.kinney@intel.com
    = ;           <mailto:michael.d.kinne= y@intel.com>>; Gao, Liming
    =            <liming.gao@intel.com <mailto:liming.gao@intel.com>>;
           &n= bsp;   Gao, Zhichao <zhichao.gao@intel.com
   &= nbsp;           <= mailto:zhichao.gao@inte= l.com>>;devel@= edk2.groups.io
       =         <mailto:devel@edk2.groups.io>
            =    *Cc:*Marvin H=C3=A4user <marvin.haeuser@outlook.com
&n= bsp;            = ;  <m= ailto:marvin.haeuser@outlook.com>>; Laszlo Ersek
&n= bsp;            = ;  <lersek@red= hat.com <mailto:lersek@redhat.com>&= gt;
         &nb= sp;     *Subject:*Re: [edk2-devel] [PATCH v3 0/1] = Add PCD to
        &n= bsp;      disable safe string constraint asse= rtions
         =       Replying as per Liming's request for th= is to be merged
       &nb= sp;       into edk2-stable202002.
           &= nbsp;   On Mon, Feb 10, 2020 at 14:12, vit9696
            &n= bsp;  <vi= t9696@protonmail.com
      =          <mailto:vit9696@protonmail.com>&g= t; wrote:

      =             &nb= sp;Hello,

      =             &nb= sp;It has been quite some time since we submitted the
 &= nbsp;           &nbs= p;     patch with so far no negative response. As = I
          = ;         mentioned previously= , my team will strongly
      &= nbsp;           &nbs= p;benefit from its landing in EDK II mainline. Since
 &n= bsp;            = ;     it does not add any regressions and can be v= iewed
         &= nbsp;         as a feature imp= lementation for the rest of EDK II
    &n= bsp;            = ;  users, I request this to be merged upstream in
&= nbsp;           &nbs= p;      edk2-stable202002.

          &nbs= p;        Best wishes,
            &= nbsp;      Vitaly

27 =D1=8F=D0=BD=D0=B2. 2020 =D0=B3.,= =D0=B2 12:47, vit9696
    &= nbsp;           &nbs= p;  <vit9= 696@protonmail.com
      &n= bsp;            = ;<mailto:vit9696@pr= otonmail.com>> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0)= :


Hi Mike,

Any progress with this? We wo= uld really benefit
     = ;            &n= bsp; from this landing in the next stable release.

Best,
Vitaly
8 =D1=8F= =D0=BD=D0=B2. 2020 =D0=B3., =D0=B2 19:35, Kinney, Michael D
=
        &= nbsp;          <michael.d.kinney@intel.c= om
         =           <mailto:michael.d.kinney@intel= .com>> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):


Hi Vitaly,

Thanks for the additional background. I would like
a couple= extra day to review the PCD name and
            =        the places
the PCD m= ight potentially be used.

If we find other API= s where ASSERT() behavior
 &nb= sp;            =      is only
valuable during dev/debu= g to quickly identify
  &= nbsp;           &nbs= p;    misuse
with trusted data and the API= provides predicable
return behavior when ASSERT() is disable= d, then
    &nb= sp;            =   I would
like to have a pattern we can potentially = apply
     = ;            &n= bsp; to all
these APIs across all packages.

Thanks,

Mike
-----Original Message-----=
From:deve= l@edk2.groups.io
&= nbsp;           &nbs= p;      <mailto:devel@edk2.groups.io><devel@edk2.groups.io
&nb= sp;            =       <mailto:devel@edk2.groups.io>> On
Behalf Of Vitaly Cheptsov via Groups.IoSent: Monday, January 6, 2020 10:44 AM
To: Kinne= y, Michael D
 &nb= sp;            =      <michael.d.kinney@intel.com
  &n= bsp;            = ;    <mailto:michael.d.kinney@intel.com>>
Cc:devel@edk2.groups.io
          &= nbsp;        <mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATC= H v3 0/1] Add
 &n= bsp;            = ;     PCD to
disable safe string constraint assertions

Hi Mike,

Yes, the primary use case is= for UEFI
  =             &nb= sp;    Applications. We
do not want to disable ASSERT=E2=80=99s completely, a= s
assertions that make sense, i.e. the ones
      &nb= sp;            = signalising
about interfa= ce misuse, are helpful for debugging.

I have a= lready explained in the BZ that
           = ;        basically all
safe string constraint assertions mak= e no
   = ;            &n= bsp;   sense for
handling untrusted data. We find this use case
       &= nbsp;           very=
logical, as these functi= ons behave properly with
assertions disabled and cover all th= ese error
conditions by the return statuses. In such
     = ;            &n= bsp; situation is
<= blockquote type=3D"cite" class=3D"">
no= t useful for these functions to assert, as
         =           we end up
inefficiently reimplementing t= he logic. I
 &nbs= p;            &= nbsp;    would have
liked the approach of discussing the interfaces
individually, but I struggle to find any that
=
        &= nbsp;          makes
sense from this point of view= .

AsciiStrToGuid will ASSERT when the length o= f the
passed string is odd. Functions that cannot, ahem,
parse, for us are pretty much useless.
AsciiStrCatS= will ASSERT when the appended
           =         string does
<= blockquote type=3D"cite" class=3D"">
not fit the buffer. For us this logic ma= kes this
function pretty much equivalent to deprecated
    &nbs= p;            &= nbsp; and thus
unava= ilable AsciiStrCat, except it is also slower.

= My original suggestion was to remove the
         &n= bsp;         assertions
entirely, but several people he= re said that
 &nb= sp;            =      they use
them to verify usage errors when handling
       = ;            tr= usted data.
This makes go= od sense to me, so we suggest to
          &nbs= p;        support
both cases by introducing a PCD in this pa= tch.

Best wishes,
Vitaly

6 =D1=8F=D0=BD= =D0=B2. 2020 =D0=B3., =D0=B2 21:28, Kinney, Michael D
<michael= .d.kinney@intel.com
            &= nbsp;      <mailto:michael.d.kinney@intel.com>> = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):


Hi Vitaly,

Is the use ca= se 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
  &= nbsp;           &nbs= p;    untrusted
source,
then = that component is required to verify the
untrust= ed
data before passing i= t to a function that clearly
documents
is input requirements. If this = approach is
            =        followed,
then
the BaseLib functions can be used "as is" as
     &= nbsp;           &nbs= p; long as
the
ASSERT() conditions are verif= ied before calling.

If there are some APIs tha= t currently
            =        document their
ASSERT()
behavior and we think that ASSERT() behavior is
incorrect and
should be handled by an existing error return
    &nbs= p;            &= nbsp; value,
then we=
should discuss each of = those APIs individually.

Mike

-----Origi= nal Message-----
From:devel@edk2.groups.io
     &nbs= p;            &= nbsp;<mailto:devel@ed= k2.groups.io><= devel@edk2.groups.io
      =             &nb= sp;<mailto: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
=
      =             &nb= sp;<mailto: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=3D2054

Requesting for merge in edk= 2-stable202002.

Changes since V1:
- Enable assertions by default to preserve the
original
<= blockquote type=3D"cite" class=3D"">behaviour
- Fix bugzilla = reference link
- Update documentation in BaseLib.h

Vitaly Cheptsov (1):
MdePkg: Add PCD to= disable safe string
<= /blockquote>
        &n= bsp;          constraint<= br class=3D"">
assertions
<= br class=3D"">MdePkg/MdePkg.dec | 6 ++
MdePkg/Library/BaseLib= /BaseLib.inf | 11 +--
MdePkg/Include/Library/BaseLib.h | 74+++++++++++++-------
MdePkg/Library/BaseLib/Safe= String.c | 4 +-
MdePkg/MdePkg.uni | 6 ++
5 file= s changed, 71 insertions(+), 30
      &nb= sp;            = deletions(-)

--
2.21.0 (Apple Git-122.2)

-=3D-=3D-=3D-=3D-=3D-=3D
Groups.io = <http://groups.io/&g= t;Links: You
         &nbs= p;         receive all message= s 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
     &n= bsp;            = ; <mailto:= devel+owner@edk2.groups.io>
Unsubscribe:https://edk2.groups.io/g/devel/unsub
[michael.d.kinney@intel.com=
           &nbs= p;       <mailto:michael.d.kinney@intel.com>]
-=3D-=3D-=3D-=3D-=3D-=3D









<= /blockquote>

--Apple-Mail=_BE7CCF4B-2763-4DA7-902A-782C2E74B5CF--