* [PATCH v3 0/1] MdePkg: Add PCD to disable safe string constraint assertions @ 2019-10-22 7:20 Vitaly Cheptsov 2019-10-22 7:20 ` [PATCH v3 1/1] " Vitaly Cheptsov 0 siblings, 1 reply; 5+ messages in thread From: Vitaly Cheptsov @ 2019-10-22 7:20 UTC (permalink / raw) To: devel [-- Attachment #1: Type: text/plain, Size: 602 bytes --] REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 Requesting for merge in edk2-stable2019011. Changes since V1: - Enable assertions by default to preserve the original behaviour - Fix bugzilla reference link Vitaly Cheptsov (1): MdePkg: Add PCD to disable safe string constraint assertions 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(-) -- 2.21.0 (Apple Git-122) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions 2019-10-22 7:20 [PATCH v3 0/1] MdePkg: Add PCD to disable safe string constraint assertions Vitaly Cheptsov @ 2019-10-22 7:20 ` Vitaly Cheptsov 2019-10-22 7:52 ` [edk2-devel] " Yao, Jiewen 0 siblings, 1 reply; 5+ messages in thread From: Vitaly Cheptsov @ 2019-10-22 7:20 UTC (permalink / raw) To: devel [-- Attachment #1: Type: text/plain, Size: 4689 bytes --] 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|BOOLEAN|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) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions 2019-10-22 7:20 ` [PATCH v3 1/1] " Vitaly Cheptsov @ 2019-10-22 7:52 ` Yao, Jiewen 2019-10-22 8:02 ` Vitaly Cheptsov 0 siblings, 1 reply; 5+ messages in thread From: Yao, Jiewen @ 2019-10-22 7:52 UTC (permalink / raw) To: devel@edk2.groups.io, vit9696@protonmail.com; +Cc: Kinney, Michael D 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] > -=-=-=-=-=-= ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions 2019-10-22 7:52 ` [edk2-devel] " Yao, Jiewen @ 2019-10-22 8:02 ` Vitaly Cheptsov 0 siblings, 0 replies; 5+ messages in thread From: Vitaly Cheptsov @ 2019-10-22 8:02 UTC (permalink / raw) To: Yao, Jiewen; +Cc: devel@edk2.groups.io, Kinney, Michael D [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 0/1] Add PCD to disable safe string constraint assertions @ 2020-01-03 17:12 Vitaly Cheptsov 2020-01-03 17:12 ` [PATCH v3 1/1] MdePkg: " Vitaly Cheptsov 0 siblings, 1 reply; 5+ messages in thread From: Vitaly Cheptsov @ 2020-01-03 17:12 UTC (permalink / raw) To: devel [-- Attachment #1: Type: text/plain, Size: 687 bytes --] 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) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions 2020-01-03 17:12 [PATCH v3 0/1] " Vitaly Cheptsov @ 2020-01-03 17:12 ` Vitaly Cheptsov 0 siblings, 0 replies; 5+ messages in thread From: Vitaly Cheptsov @ 2020-01-03 17:12 UTC (permalink / raw) To: devel [-- Attachment #1: Type: text/plain, Size: 17597 bytes --] 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/Include/Library/BaseLib.h | 74 +++++++++++++------- MdePkg/Library/BaseLib/SafeString.c | 4 +- MdePkg/MdePkg.uni | 6 ++ 5 files changed, 71 insertions(+), 30 deletions(-) diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index d022cc5e3e..0191b7a08b 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|BOOLEAN|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/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h index 2a75bc023f..c413ca5f57 100644 --- a/MdePkg/Include/Library/BaseLib.h +++ b/MdePkg/Include/Library/BaseLib.h @@ -189,6 +189,8 @@ StrnSizeS ( If Destination is not aligned on a 16-bit boundary, then ASSERT(). If Source is not aligned on a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -225,6 +227,8 @@ StrCpyS ( If Length > 0 and Destination is not aligned on a 16-bit boundary, then ASSERT(). If Length > 0 and Source is not aligned on a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -263,6 +267,8 @@ StrnCpyS ( If Destination is not aligned on a 16-bit boundary, then ASSERT(). If Source is not aligned on a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -303,6 +309,8 @@ StrCatS ( If Destination is not aligned on a 16-bit boundary, then ASSERT(). If Source is not aligned on a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -350,9 +358,11 @@ StrnCatS ( be ignored. Then, the function stops at the first character that is a not a valid decimal character or a Null-terminator, whichever one comes first. + If String is not aligned in a 16-bit boundary, then ASSERT(). + + 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(). @@ -406,9 +416,11 @@ StrDecimalToUintnS ( be ignored. Then, the function stops at the first character that is a not a valid decimal character or a Null-terminator, whichever one comes first. + If String is not aligned in a 16-bit boundary, then ASSERT(). + + 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(). @@ -467,9 +479,11 @@ StrDecimalToUint64S ( the first character that is a not a valid hexadecimal character or NULL, whichever one comes first. + If String is not aligned in a 16-bit boundary, then ASSERT(). + + 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(). @@ -528,9 +542,11 @@ StrHexToUintnS ( the first character that is a not a valid hexadecimal character or NULL, whichever one comes first. + If String is not aligned in a 16-bit boundary, then ASSERT(). + + 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(). @@ -622,6 +638,7 @@ AsciiStrnSizeS ( This function is similar as strcpy_s defined in C11. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -656,6 +673,7 @@ AsciiStrCpyS ( This function is similar as strncpy_s defined in C11. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -692,6 +710,7 @@ AsciiStrnCpyS ( This function is similar as strcat_s defined in C11. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -730,6 +749,7 @@ AsciiStrCatS ( This function is similar as strncat_s defined in C11. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -777,6 +797,7 @@ AsciiStrnCatS ( be ignored. Then, the function stops at the first character that is a not a valid decimal character or a Null-terminator, whichever one comes first. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). If Data is NULL, then ASSERT(). If PcdMaximumAsciiStringLength is not zero, and String contains more than @@ -832,6 +853,7 @@ AsciiStrDecimalToUintnS ( be ignored. Then, the function stops at the first character that is a not a valid decimal character or a Null-terminator, whichever one comes first. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). If Data is NULL, then ASSERT(). If PcdMaximumAsciiStringLength is not zero, and String contains more than @@ -891,6 +913,7 @@ AsciiStrDecimalToUint64S ( character that is a not a valid hexadecimal character or Null-terminator, whichever on comes first. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). If Data is NULL, then ASSERT(). If PcdMaximumAsciiStringLength is not zero, and String contains more than @@ -950,6 +973,7 @@ AsciiStrHexToUintnS ( character that is a not a valid hexadecimal character or Null-terminator, whichever on comes first. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). If Data is NULL, then ASSERT(). If PcdMaximumAsciiStringLength is not zero, and String contains more than @@ -1506,12 +1530,11 @@ StrHexToUint64 ( "::" can be used to compress one or more groups of X when X contains only 0. The "::" can only appear once in the String. + If String is not aligned in a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). - If Address 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(). @@ -1567,12 +1590,11 @@ StrToIpv6Address ( When /P is in the String, the function stops at the first character that is not a valid decimal digit character after P is converted. + If String is not aligned in a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). - If Address 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(). @@ -1640,9 +1662,11 @@ StrToIpv4Address ( oo Data4[48:55] pp Data4[56:63] + If String is not aligned in a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). If Guid is NULL, then ASSERT(). - If String is not aligned in a 16-bit boundary, then ASSERT(). @param String Pointer to a Null-terminated Unicode string. @param Guid Pointer to the converted GUID. @@ -1676,15 +1700,12 @@ StrToGuid ( If String is not aligned in a 16-bit boundary, then ASSERT(). + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). - If Buffer is NULL, then ASSERT(). - If Length is not multiple of 2, then ASSERT(). - If PcdMaximumUnicodeStringLength is not zero and Length is greater than PcdMaximumUnicodeStringLength, then ASSERT(). - If MaxBufferSize is less than (Length / 2), then ASSERT(). @param String Pointer to a Null-terminated Unicode string. @@ -1775,8 +1796,9 @@ UnicodeStrToAsciiStr ( If any Unicode characters in Source contain non-zero value in the upper 8 bits, then ASSERT(). - If Source is not aligned on a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -1824,6 +1846,8 @@ UnicodeStrToAsciiStrS ( If any Unicode characters in Source contain non-zero value in the upper 8 bits, then ASSERT(). If Source is not aligned on a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -2388,8 +2412,8 @@ AsciiStrHexToUint64 ( "::" can be used to compress one or more groups of X when X contains only 0. The "::" can only appear once in the String. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). - If Address is NULL, then ASSERT(). If EndPointer is not NULL and Address is translated from String, a pointer @@ -2443,8 +2467,8 @@ AsciiStrToIpv6Address ( When /P is in the String, the function stops at the first character that is not a valid decimal digit character after P is converted. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). - If Address is NULL, then ASSERT(). If EndPointer is not NULL and Address is translated from String, a pointer @@ -2508,6 +2532,7 @@ AsciiStrToIpv4Address ( oo Data4[48:55] pp Data4[56:63] + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). If Guid is NULL, then ASSERT(). @@ -2541,15 +2566,12 @@ AsciiStrToGuid ( decoding stops after Length of characters and outputs Buffer containing (Length / 2) bytes. + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If String is NULL, then ASSERT(). - If Buffer is NULL, then ASSERT(). - If Length is not multiple of 2, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero and Length is greater than PcdMaximumAsciiStringLength, then ASSERT(). - If MaxBufferSize is less than (Length / 2), then ASSERT(). @param String Pointer to a Null-terminated ASCII string. @@ -2632,6 +2654,8 @@ AsciiStrToUnicodeStr ( equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. If Destination is not aligned on a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -2678,6 +2702,8 @@ AsciiStrToUnicodeStrS ( ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. If Destination is not aligned on a 16-bit boundary, then ASSERT(). + + Unless PcdAssertOnSafeStringConstraints is set to FALSE: If an error would be returned, then the function will also ASSERT(). If an error is returned, then Destination and DestinationLength are 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.2) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-03 17:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-22 7:20 [PATCH v3 0/1] MdePkg: Add PCD to disable safe string constraint assertions Vitaly Cheptsov 2019-10-22 7:20 ` [PATCH v3 1/1] " Vitaly Cheptsov 2019-10-22 7:52 ` [edk2-devel] " Yao, Jiewen 2019-10-22 8:02 ` Vitaly Cheptsov -- strict thread matches above, loose matches on Subject: below -- 2020-01-03 17:12 [PATCH v3 0/1] " Vitaly Cheptsov 2020-01-03 17:12 ` [PATCH v3 1/1] MdePkg: " Vitaly Cheptsov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox