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....@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 <vit9...@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+ow...@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen....@intel.com] >> -=-=-=-=-=-= > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49332): https://edk2.groups.io/g/devel/message/49332 Mute This Topic: https://groups.io/mt/36392351/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: OpenPGP digital signature
