Mike, 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 think my concern is still the global scope of the constraint, even if we tune it per module. For example the DXE Core can interact with PE/COFF images and other data that could have indirectly come from the disk. So conceptually you may want to ASSERT on some constraints and not on others. I think that is why I ratholed on expanding the error 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 MdePkg Libs that has the most instances 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 to add a ConstraintLib modeled after C11 vs. updating the DebugLib. This would solve the 2 things that made me uncomfortable: 1) Extending the DebugLib API; 2) Giving the caller control 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 defines constraint_handler_t and set_constraint_handler_s as a way for the caller to configure the behavior for bounds checked functions. I think that is the behavior I prefer. So if we are going to make a change that impacts DebugLib compatibility I just want to make sure we have a conversation about all the options. My primary goal is we have the conversation, and if folks don't agree with me that is fine at least we talked 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 extending the DebugLib. The basic implementation of the lib would look like: typedef VOID (EFIAPI *CONSTRAINT_HANDLER) ( IN CONST CHAR8 *FileName, IN UINTN LineNumber, IN CONST CHAR8 *Description, IN EFI_STATUS Status ); // Default to AssertConstraintHandler 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 default handler is implementation defined in C11. CONSTRAINT_HANDLER gDefaultConstraintHandler = AssertConstraintHandler; CONSTRAINT_HANDLER gActiveConstraintHandler = gDefaultConstraintHandler; BOOLEAN EFIAPI ConstraintAssertEnabled ( VOID ) { return (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_CONSTRAINT_ENABLED) != 0); } EFI_STATUS EFIAPI SetConstraintHandler ( IN CONSTRAINT_HANDLER Handler ) { if (Handler == NULL) { gActiveConstraintHandler = gDefaultConstraintHandler; } else { gActiveConstraintHandler = Handler; } } 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 = %r): ", Status)); DebugAssert (FileName, LineNumber, Description) } return; } VOID IgnoreConstraintHandler ( IN CONST CHAR8 *FileName, IN UINTN LineNumber, IN CONST CHAR8 *Description, IN EFI_STATUS Status ) { return; } We could add macros for the code in the lib to call: #define CONSTRAINT_CHECK(Expression, Status) \ do { \ if (!(Expression)) { \ gActiveConstraintHandler (__FILE__, __LINE__, Expression, Status); \ return Status; \ } \ } while (FALSE) #define CONSTRAINT_REQUIRE(Expression, Status, Label) \ do { \ if (!(Expression)) { \ gActiveConstraintHandler (__FILE__, __LINE__, Expression, Status); \ goto Label; \ } \ } while (FALSE) As a caller we have now have control: EFI_STATUS Status; CHAR16 Dst[2]; SetConstraintHandler (IgnoreConstraintHandler); Status = StrCpyS (Dst, sizeof (Dst), L"Too Long"); Print (L"Dst =%s (%r)\n", Dst, Status); SetConstraintHandler (AssertConstraintHandler); StrCpyS (Dst, sizeof (Dst), L"Too Long"); Thanks, Andrew Fish PS Since 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 libs. So a platform could include this .DSC and that would give them the default library mapping to keep code compiling. It will only work after other platforms start including it, but after that it would give default mappings for dependent libs. In our above example we could have added this and then existing builds that included MdePkgCompatability.dsc.inc would keep compiling. [LibraryClasses] DebugConstraintLib|MdePkg/Library/DebugConstraintLib/DebugConstraintLib.inf > On Feb 15, 2020, at 11:38 AM, Michael D Kinney 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 be 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 UEFI 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/MM modules makes sense to find incorrect input to these functions from modules that can guarantee the inputs would never return an error and catch these as 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 CONSTRAINT_ASSERT() for conditions that are also checked and return an error or fail with predicable behavior that allows the system to continue to function. ASSERT() is for conditions that the system can notcontinue. > > Best regards, > > Mike > > From: afish@apple.com > > Sent: Friday, February 14, 2020 10:27 PM > To: vit9696 > > Cc: devel@edk2.groups.io ; Kinney, Michael D >; Gao, Liming >; Gao, Zhichao >; Marvin Häuser >; Laszlo Ersek > > Subject: Re: [edk2-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 implementations that would break with this change as Mike proposed. Given breaking every one's debug lib is such a big deal maybe it is something that we should do as a long term plan vs. 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 breaking the world is breaking the world frequently. > > I'm also a little worried that we are taking things that are today locally scoped like SAFE_STRING_CONSTRAINT_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. ASSERTs. 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 that the low level code knows the right thing to do in a global sense even if there is a PCD. It almost seems like we should have wrappers for the Safe string functions that implement the behavior you want as a 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 > wrote: > > Hi Andrew, > > While your suggestions look interesting, I am afraid they are not particularly what we want to cover with this discussion at the moment. > > 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. > 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. > 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. > > =To make it clear, currently I plan to add the following interface: > > #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) == 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 — 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: > > > > 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 in PcdDebugPropertyMask 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 in PcdDebugPropertyMask to disable these types of asserts. > > This approach does require all the DebugLib implementations to be updated with the new DebugConstraintAssertDisabled() API. > > > 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 = EFI_INVALID_PARAMETER; > > REQUIRE(Arg1 != NULL, ErrorExit); > REQUIRE(Arg2 != NULL, ErrorExit); > REQUIRE(Arg3 != 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 != NULL, ErrorExit, Status = EFI_INVALID_PARAMETER); > REQUIRE_ACTION(Arg2 != NULL, ErrorExit, Status = EFI_INVALID_PARAMETER); > REQUIRE_ACTION(Arg3 != NULL, ErrorExit, Status = EFI_INVALID_PARAMETER); > > ErrorExit: > return Status; > > If you use CONSTRAINT_ASSERT(); > > if (Arg1 == NULL || Arg2 == NULL || Arg3 == NULL) { > CONSTRAINT_ASSERT (Arg1 != NULL); > CONSTRAINT_ASSERT (Arg2 != NULL); > CONSTRAINT_ASSERT (Arg3 != 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 with: > 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 != NULL, ErrorExit, Status = 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. > > 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äuser >; 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. > > > 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. > > > Best wishes, > Vitaly > > > 14 февр. 2020 г., в 20:00, Kinney, Michael D > написал(а): > > Vitaly, > > I want to make sure a feature PCD can be used to disable ASSERT() behavior in more than just safe string functions in BaseLib. > > Can we consider changing the name and description of PcdAssertOnSafeStringConstraints to 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äuser >; 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: > Hello, > > 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. > > Best wishes, > Vitaly > > > 27 янв. 2020 г., в 12:47, vit9696 > написал(а): > > > > > > Hi Mike, > > > > Any progress with this? We would really benefit from this landing in the next stable release. > > > > Best, > > Vitaly > > > >> 8 янв. 2020 г., в 19:35, Kinney, Michael D > написал(а): > >> > >> > >> Hi Vitaly, > >> > >> 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. > >> > >> 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. > >> > >> Thanks, > >> > >> Mike > >> > >>> -----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 > >>> > >>> Hi Mike, > >>> > >>> Yes, the primary use case is for UEFI Applications. We > >>> do not want to disable ASSERT’s completely, as > >>> assertions that make sense, i.e. the ones signalising > >>> about interface misuse, are helpful for debugging. > >>> > >>> 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. > >>> > >>> 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. > >>> > >>> 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. > >>> > >>> Best wishes, > >>> Vitaly > >>> > >>>> 6 янв. 2020 г., в 21:28, Kinney, Michael D > >>> > написал(а): > >>>> > >>>> > >>>> Hi Vitaly, > >>>> > >>>> Is the use case 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 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. > >>>> > >>>> 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. > >>>> > >>>> Mike > >>>> > >>>> > >>>>> -----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 > >>>>> > >>>>> REF: > >>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2054 > >>>>> > >>>>> Requesting for merge in edk2-stable202002. > >>>>> > >>>>> Changes since V1: > >>>>> - Enable assertions by default to preserve the > >>> original > >>>>> behaviour > >>>>> - Fix bugzilla reference link > >>>>> - Update documentation in BaseLib.h > >>>>> > >>>>> Vitaly Cheptsov (1): > >>>>> MdePkg: Add PCD to disable safe string constraint > >>>>> assertions > >>>>> > >>>>> 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(-) > >>>>> > >>>>> -- > >>>>> 2.21.0 (Apple Git-122.2) > >>>>> > >>>>> > >>>>> -=-=-=-=-=-= > >>>>> Groups.io Links: You receive all messages 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 > >>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub > >>>>> [michael.d.kinney@intel.com ] > >>>>> -=-=-=-=-=-= > >>>> > >>> > >>> > >>> > >> > > > > > > > > > > >