Re: [edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions

["Vitaly Cheptsov" <vit9696@protonmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7403 bytes --]

I agree that we want this PCD mentioned in BaseLib header, but most likely it should be done in a shorter form.

What about something like this?

  Unless PcdAssertOnSafeStringConstraints is set to FALSE:
  If String is NULL, then ASSERT().
  If Data is NULL, then ASSERT().
  If String is not aligned in a 16-bit boundary, then ASSERT().
  If PcdMaximumUnicodeStringLength is not zero, and String contains more than
  PcdMaximumUnicodeStringLength Unicode characters, not including the
  Null-terminator, then ASSERT().

Best wishes,
Vitaly

> 22 окт. 2019 г., в 10:52, Yao, Jiewen <jiewen.yao@intel.com> написал(а):
> 
> 
> In BaseLib.h, we have below comments in each safe string function header.
> ===============
>  If an error would be returned, then the function will also ASSERT().
> ===============
> 
> As I mentioned earlier, the original purpose is to let the caller guarantee the correctness of the input. Similar to Microsoft Visual Studio strcpy_s() behavior.
> 
> 
> If we decide to change the purpose and change the design, I recommend we add clarification in the function header as well.
> 
> For example:
> ===============================
> The PcdAssertOnSafeStringConstraints is used to control the behavior of the runtime violation.
> When PcdAssertOnSafeStringConstraints is TRUE, if an error would be returned, then the function will also ASSERT().
> When PcdAssertOnSafeStringConstraints is FALSE, if an error would be returned, then the function will NOT ASSERT().
> ===============================
> 
> 
> Hi Michael D Kinney
> Do you have some comment?
> 
> Thank you
> Yao Jiewen
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
>> Cheptsov via Groups.Io
>> Sent: Tuesday, October 22, 2019 3:20 PM
>> To: devel@edk2.groups.io
>> Subject: [edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string
>> constraint assertions
>> 
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054
>> 
>> Runtime data checks are not meant to cause debug assertions
>> unless explicitly needed by some debug code (thus the PCD)
>> as this breaks debug builds validating data with BaseLib.
>> 
>> Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com>>
>> ---
>> MdePkg/MdePkg.dec                   |  6 ++++++
>> MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++++++-----
>> MdePkg/Library/BaseLib/SafeString.c |  4 +++-
>> MdePkg/MdePkg.uni                   |  6 ++++++
>> 4 files changed, 21 insertions(+), 6 deletions(-)
>> 
>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>> index 3fd7d1634c..c1671333f6 100644
>> --- a/MdePkg/MdePkg.dec
>> +++ b/MdePkg/MdePkg.dec
>> @@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>>   # @Prompt Memory Address of GuidedExtractHandler Table.
>> 
>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x1000000
>> |UINT64|0x30001015
>> 
>> +  ## Indicates if safe string constraint violation should assert.<BR><BR>
>> +  #   TRUE  - Safe string constraint violation causes assertion.<BR>
>> +  #   FALSE - Safe string constraint violation does not cause assertion.<BR>
>> +  # @Prompt Enable safe string constraint violation assertions.
>> +
>> gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|TRUE|BOOLEA
>> N|0x0000002e
>> +
>> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>>   ## This value is used to set the base address of PCI express hierarchy.
>>   # @Prompt PCI Express Base Address.
>> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
>> b/MdePkg/Library/BaseLib/BaseLib.inf
>> index 3586beb0ab..bc98bc6134 100644
>> --- a/MdePkg/Library/BaseLib/BaseLib.inf
>> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
>> @@ -390,11 +390,12 @@ [LibraryClasses]
>>   BaseMemoryLib
>> 
>> [Pcd]
>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength      ##
>> SOMETIMES_CONSUMES
>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength     ##
>> SOMETIMES_CONSUMES
>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ##
>> SOMETIMES_CONSUMES
>> -  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ##
>> SOMETIMES_CONSUMES
>> -  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType       ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints       ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength             ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength            ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength          ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask  ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType              ##
>> SOMETIMES_CONSUMES
>> 
>> [FeaturePcd]
>>   gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
>> diff --git a/MdePkg/Library/BaseLib/SafeString.c
>> b/MdePkg/Library/BaseLib/SafeString.c
>> index 7dc03d2caa..56b5e34a8d 100644
>> --- a/MdePkg/Library/BaseLib/SafeString.c
>> +++ b/MdePkg/Library/BaseLib/SafeString.c
>> @@ -14,7 +14,9 @@
>> 
>> #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status)  \
>>   do { \
>> -    ASSERT (Expression); \
>> +    if (PcdGetBool (PcdAssertOnSafeStringConstraints)) { \
>> +      ASSERT (Expression); \
>> +    } \
>>     if (!(Expression)) { \
>>       return Status; \
>>     } \
>> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
>> index 5c1fa24065..425b66bb43 100644
>> --- a/MdePkg/MdePkg.uni
>> +++ b/MdePkg/MdePkg.uni
>> @@ -287,6 +287,12 @@
>> 
>> #string
>> STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_HELP
>> #language en-US "This value is used to set the available memory address to
>> store Guided Extract Handlers. The required memory space is decided by the
>> value of PcdMaximumGuidedExtractHandler."
>> 
>> +#string
>> STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_PROMPT
>> #language en-US "Enable safe string constraint violation assertions"
>> +
>> +#string
>> STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_HELP
>> #language en-US "Indicates if safe string constraint violation should
>> assert.<BR><BR>\n"
>> +                                                                                   "TRUE  - Safe string constraint
>> violation causes assertion.<BR>\n"
>> +                                                                                   "FALSE - Safe string constraint
>> violation does not cause assertion.<BR>"
>> +
>> #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT
>> #language en-US "PCI Express Base Address"
>> 
>> #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP
>> #language en-US "This value is used to set the base address of PCI express
>> hierarchy."
>> --
>> 2.21.0 (Apple Git-122)
>> 
>> 
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> 
>> View/Reply Online (#49330): https://edk2.groups.io/g/devel/message/49330
>> Mute This Topic: https://groups.io/mt/36392351/1772286
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jiewen.yao@intel.com]
>> -=-=-=-=-=-=
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 489 bytes --]