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

Good day Andrew,

First of all, thank you 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 
needs control of the behaviour, however your proposal needs "support" 
from both the caller (choose the handler) and the callee (call the 
handler) and is neither thread-safe nor event-safe as Vitaly already 
pointed out. Fixing these problems would only worsen the complexity 
situation in my opinion.

Diverging from both of your 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 would be 
completely different from Vitaly's proposal. It would be a caller macro 
to ASSERT based on a pre-defined list of return statuses.
To achieve this, we would order all statuses by types (classes). At the 
moment, I can only think of two: "environmental" and "constraint". For 
example, "Out Of Resources" would be environmental and "Invalid 
Parameter" would be constraint. We could define environment statuses 
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 appropiate error codes and correctly propagate 
them 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 
cases) I might actually prefer a function. But it would not be 
backwards-compatible with the current functionality, which none of the 
"caller-based" concepts can be anyway (the caller needs explicit code).

However, CONSTRAINT_ASSERT might actually be too specific. Maybe we want 
something like "ASSERT_RETURN" and have the classes "ASSERT_CONSTRAINT" 
and "ASSERT_ENVIRONMENT" (bitfield, like with DEBUG). The reason for 
this is embedded systems where the environment is trusted/known and the 
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 ASSERTs 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 involvement, for example to 
tell DxeCore whether to 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 specific 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 context[1] which I will most 
definitely be getting 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 have all RETURN_STATUS 
macros apply to those special return values too? We'd need to define 
them 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 override library 
classes from the inc by declaring them 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 defaults 
when something specific (debugging, performance-optimized versions, ...) 
is required - easier to read, easier to maintain. The content is 
needed/there anyway as the libraries are declared in the package's own DSC.

It would be nice if you had comments regarding every aspect I just 
mentioned, it was just something coming to my mind this morning. Thanks 
for the input so far, it's nice to see some movement now!

Best regards,
Marvin

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

Am 16.02.2020 um 22:25 schrieb Andrew Fish via Groups.Io:

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 
<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 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 systemcan 
notcontinue.
Best regards,
Mike
*From:*afish@apple.com <mailto:afish@apple.com><afish@apple.com 
<mailto:afish@apple.com>>
*Sent:*Friday, February 14, 2020 10:27 PM
*To:*vit9696 <vit9696@protonmail.com <mailto:vit9696@protonmail.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@intel.com <mailto:liming.gao@intel.com>>; Gao, Zhichao 
<zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>; Marvin Häuser 
<marvin.haeuser@outlook.com <mailto:marvin.haeuser@outlook.com>>; 
Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
*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 <vit9696@protonmail.com
   <mailto:vit9696@protonmail.com>> 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 <afish@apple.com
   <mailto:afish@apple.com>> wrote:



           On Feb 14, 2020, at 2:50 PM, Michael D Kinney
           <michael.d.kinney@intel.com
           <mailto:michael.d.kinney@intel.com>> 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() 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 <vit9696@protonmail.com
           <mailto:vit9696@protonmail.com>>
           *Sent:*Friday, February 14, 2020 9:38 AM
           *To:*Kinney, Michael D <michael.d.kinney@intel.com
           <mailto:michael.d.kinney@intel.com>>
           *Cc:*devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
           Gao, Liming <liming.gao@intel.com
           <mailto:liming.gao@intel.com>>; Gao, Zhichao
           <zhichao.gao@intel.com <mailto:zhichao.gao@intel.com>>;
           Marvin Häuser <marvin.haeuser@outlook.com
           <mailto:marvin.haeuser@outlook.com>>; Laszlo Ersek
           <lersek@redhat.com <mailto:lersek@redhat.com>>
           *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
               <michael.d.kinney@intel.com
               <mailto:michael.d.kinney@intel.com>> написал(а):
               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
               <mailto:devel@edk2.groups.io><devel@edk2.groups.io
               <mailto:devel@edk2.groups.io>>*On Behalf Of*Vitaly
               Cheptsov via Groups.Io
               *Sent:*Friday, February 14, 2020 3:55 AM
               *To:*Kinney, Michael D <michael.d.kinney@intel.com
               <mailto:michael.d.kinney@intel.com>>; Gao, Liming
               <liming.gao@intel.com <mailto:liming.gao@intel.com>>;
               Gao, Zhichao <zhichao.gao@intel.com
               <mailto:zhichao.gao@intel.com>>;devel@edk2.groups.io
               <mailto:devel@edk2.groups.io>
               *Cc:*Marvin Häuser <marvin.haeuser@outlook.com
               <mailto:marvin.haeuser@outlook.com>>; Laszlo Ersek
               <lersek@redhat.com <mailto:lersek@redhat.com>>
               *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
               <vit9696@protonmail.com
               <mailto:vit9696@protonmail.com>> 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

                   <vit9696@protonmail.com
                   <mailto:vit9696@protonmail.com>> написал(а):

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

                   <michael.d.kinney@intel.com
                   <mailto:michael.d.kinney@intel.com>> написал(а):

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

                   <mailto:devel@edk2.groups.io><devel@edk2.groups.io
                   <mailto:devel@edk2.groups.io>> On

Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Monday, January 6, 2020 10:44 AM
To: Kinney, Michael D

                   <michael.d.kinney@intel.com
                   <mailto:michael.d.kinney@intel.com>>

Cc:devel@edk2.groups.io

                   <mailto: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

<michael.d.kinney@intel.com

                   <mailto:michael.d.kinney@intel.com>> написал(а):

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

                   <mailto:devel@edk2.groups.io><devel@edk2.groups.io
                   <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

                   <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=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
Mute This Topic:

https://groups.io/mt/69401948/1643496

Group Owner:devel+owner@edk2.groups.io

                   <mailto:devel+owner@edk2.groups.io>

Unsubscribe:https://edk2.groups.io/g/devel/unsub
[michael.d.kinney@intel.com

                   <mailto:michael.d.kinney@intel.com>]

-=-=-=-=-=-=