> On Feb 14, 2020, at 2:50 PM, Michael D Kinney <[email protected]>
> 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 <[email protected] <mailto:[email protected]>>
> Sent: Friday, February 14, 2020 9:38 AM
> To: Kinney, Michael D <[email protected]
> <mailto:[email protected]>>
> Cc: [email protected] <mailto:[email protected]>; Gao, Liming
> <[email protected] <mailto:[email protected]>>; Gao, Zhichao
> <[email protected] <mailto:[email protected]>>; Marvin Häuser
> <[email protected] <mailto:[email protected]>>; Laszlo
> Ersek <[email protected] <mailto:[email protected]>>
> 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 <[email protected]
> <mailto:[email protected]>> написал(а):
>
> 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: [email protected] <mailto:[email protected]>
> <[email protected] <mailto:[email protected]>> On Behalf Of Vitaly
> Cheptsov via Groups.Io
> Sent: Friday, February 14, 2020 3:55 AM
> To: Kinney, Michael D <[email protected]
> <mailto:[email protected]>>; Gao, Liming <[email protected]
> <mailto:[email protected]>>; Gao, Zhichao <[email protected]
> <mailto:[email protected]>>; [email protected]
> <mailto:[email protected]>
> Cc: Marvin Häuser <[email protected]
> <mailto:[email protected]>>; Laszlo Ersek <[email protected]
> <mailto:[email protected]>>
> 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 <[email protected]
> <mailto:[email protected]>> 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 <[email protected]
> > <mailto:[email protected]>> написал(а):
> >
> >
> > 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 <[email protected]
> >> <mailto:[email protected]>> написал(а):
> >>
> >>
> >> 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: [email protected] <mailto:[email protected]>
> >>> <[email protected] <mailto:[email protected]>> On
> >>> Behalf Of Vitaly Cheptsov via Groups.Io
> >>> Sent: Monday, January 6, 2020 10:44 AM
> >>> To: Kinney, Michael D <[email protected]
> >>> <mailto:[email protected]>>
> >>> Cc: [email protected] <mailto:[email protected]>
> >>> 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
> >>> <[email protected] <mailto:[email protected]>>
> >>> написал(а):
> >>>>
> >>>>
> >>>> 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: [email protected] <mailto:[email protected]>
> >>>>> <[email protected] <mailto:[email protected]>> On
> >>>>> Behalf Of Vitaly Cheptsov via Groups.Io
> >>>>> Sent: Friday, January 3, 2020 9:13 AM
> >>>>> To: [email protected] <mailto:[email protected]>
> >>>>> 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
> >>>>> <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 <http://groups.io/> Links: You receive all messages sent to
> >>> this
> >>>>> group.
> >>>>>
> >>>>> View/Reply Online (#52837):
> >>>>> https://edk2.groups.io/g/devel/message/52837
> >>>>> <https://edk2.groups.io/g/devel/message/52837>
> >>>>> Mute This Topic:
> >>> https://groups.io/mt/69401948/1643496
> >>> <https://groups.io/mt/69401948/1643496>
> >>>>> Group Owner: [email protected]
> >>>>> <mailto:[email protected]>
> >>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> >>>>> <https://edk2.groups.io/g/devel/unsub>
> >>>>> [[email protected] <mailto:[email protected]>]
> >>>>> -=-=-=-=-=-=
> >>>>
> >>>
> >>>
> >>>
> >>
> >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54488): https://edk2.groups.io/g/devel/message/54488
Mute This Topic: https://groups.io/mt/69401948/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
