* [Patch v8 0/2] Disable safe string constraint assertions @ 2020-05-20 3:01 Michael D Kinney 2020-05-20 3:01 ` [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks Michael D Kinney 2020-05-20 3:01 ` [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Michael D Kinney 0 siblings, 2 replies; 13+ messages in thread From: Michael D Kinney @ 2020-05-20 3:01 UTC (permalink / raw) To: devel REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 V8 Add DEBUG_VERBOSE message and unit test V7 addressed review comments (only documentation changes). Current implementation of SafeString does not let one parse untrusted data with its interfaces as they ASSERT on failing runtime checks and require one to effectively reimplement the function on the caller side. All the former proposals made it clear that the attempts to introduce a solution preserving this behavior require a lot of changes throughout the codebase including platform code (e.g. introducing constraint assertions and updating all DebugLib implementations) or require all sorts of hacks. Since the original code has roughly no benefit except in some very special cases and the effort required to preserve it is very high, I propose to remove it as agreed on with several other parties. Please note, that this patch does not change the behaviour of the functions in RELEASE builds. I.e. they will still check for NULL and return RETURN_INVALID_PARAMETER. In the future we may (or may not) want them to simply ASSERT in this case. Regardless this should be done in a separate BZ entry and a separate commit. For this reason I ask everyone not to touch this subject. Due to the amount of discussion this has already undergone after almost a year I kindly request everyone to reduce the communication to the minimum and abstain from proposing another approach. Requesting to merge this into edk2-stable202005. Cc: Andrew Fish afish@apple.com Cc: Ard Biesheuvel ard.biesheuvel@linaro.org Cc: Bret Barkelew bret.barkelew@microsoft.com Cc: Brian J. Johnson brian.johnson@hpe.com Cc: Chasel Chiu chasel.chiu@intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Laszlo Ersek lersek@redhat.com Cc: Leif Lindholm leif@nuviainc.com Cc: Liming Gao liming.gao@intel.com Cc: Marvin Häuser mhaeuser@outlook.de Cc: Michael D Kinney michael.d.kinney@intel.com Cc: Vincent Zimmer vincent.zimmer@intel.com Cc: Zhichao Gao zhichao.gao@intel.com Cc: Jiewen Yao jiewen.yao@intel.com Signed-off-by: Vitaly Cheptsov vit9696@protonmail.com Michael D Kinney (2): MdePkg: Fix SafeString performing assertions on runtime checks MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test MdePkg/Include/Library/BaseLib.h | 111 ----------------- MdePkg/Library/BaseLib/SafeString.c | 115 +----------------- .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++++++++++++ 3 files changed, 88 insertions(+), 223 deletions(-) -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks 2020-05-20 3:01 [Patch v8 0/2] Disable safe string constraint assertions Michael D Kinney @ 2020-05-20 3:01 ` Michael D Kinney 2020-05-20 5:56 ` Vitaly Cheptsov 2020-05-20 14:50 ` Liming Gao 2020-05-20 3:01 ` [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Michael D Kinney 1 sibling, 2 replies; 13+ messages in thread From: Michael D Kinney @ 2020-05-20 3:01 UTC (permalink / raw) To: devel Cc: Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chasel Chiu, Jordan Justen, Laszlo Ersek, Leif Lindholm, Liming Gao, Marvin H?user, Vincent Zimmer, Zhichao Gao, Jiewen Yao, Vitaly Cheptsov REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 Runtime checks returned via status return code should not work as assertions to permit parsing not trusted data with SafeString interfaces. Replace ASSERT() with a DEBUG_VERBOSE message. Cc: Andrew Fish <afish@apple.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Bret Barkelew <bret.barkelew@microsoft.com> Cc: Brian J. Johnson <brian.johnson@hpe.com> Cc: Chasel Chiu <chasel.chiu@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Marvin H?user <mhaeuser@outlook.de> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Vincent Zimmer <vincent.zimmer@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com> --- MdePkg/Include/Library/BaseLib.h | 111 --------------------------- MdePkg/Library/BaseLib/SafeString.c | 115 +--------------------------- 2 files changed, 3 insertions(+), 223 deletions(-) diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h index b0bbe8cef8..8e7b87cbda 100644 --- a/MdePkg/Include/Library/BaseLib.h +++ b/MdePkg/Include/Library/BaseLib.h @@ -216,7 +216,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -252,7 +251,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -290,7 +288,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -330,7 +327,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -377,12 +373,7 @@ 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 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(). If String has no valid decimal digits in the above format, then 0 is stored at the location pointed to by Data. @@ -433,12 +424,7 @@ 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 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(). If String has no valid decimal digits in the above format, then 0 is stored at the location pointed to by Data. @@ -494,12 +480,7 @@ StrDecimalToUint64S ( the first character that is a not a valid hexadecimal character or NULL, whichever one comes first. - 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(). If String has no valid hexadecimal digits in the above format, then 0 is stored at the location pointed to by Data. @@ -555,12 +536,7 @@ StrHexToUintnS ( the first character that is a not a valid hexadecimal character or NULL, whichever one comes first. - 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(). If String has no valid hexadecimal digits in the above format, then 0 is stored at the location pointed to by Data. @@ -649,8 +625,6 @@ AsciiStrnSizeS ( This function is similar as strcpy_s defined in C11. - If an error would be returned, then the function will also ASSERT(). - If an error is returned, then the Destination is unmodified. @param Destination A pointer to a Null-terminated Ascii string. @@ -683,8 +657,6 @@ AsciiStrCpyS ( This function is similar as strncpy_s defined in C11. - If an error would be returned, then the function will also ASSERT(). - If an error is returned, then the Destination is unmodified. @param Destination A pointer to a Null-terminated Ascii string. @@ -719,8 +691,6 @@ AsciiStrnCpyS ( This function is similar as strcat_s defined in C11. - If an error would be returned, then the function will also ASSERT(). - If an error is returned, then the Destination is unmodified. @param Destination A pointer to a Null-terminated Ascii string. @@ -757,8 +727,6 @@ AsciiStrCatS ( This function is similar as strncat_s defined in C11. - If an error would be returned, then the function will also ASSERT(). - If an error is returned, then the Destination is unmodified. @param Destination A pointer to a Null-terminated Ascii string. @@ -804,12 +772,6 @@ 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. - If String is NULL, then ASSERT(). - If Data is NULL, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero, and String contains more than - PcdMaximumAsciiStringLength Ascii characters, not including the - Null-terminator, then ASSERT(). - If String has no valid decimal digits in the above format, then 0 is stored at the location pointed to by Data. If the number represented by String exceeds the range defined by UINTN, then @@ -859,12 +821,6 @@ 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. - If String is NULL, then ASSERT(). - If Data is NULL, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero, and String contains more than - PcdMaximumAsciiStringLength Ascii characters, not including the - Null-terminator, then ASSERT(). - If String has no valid decimal digits in the above format, then 0 is stored at the location pointed to by Data. If the number represented by String exceeds the range defined by UINT64, then @@ -918,12 +874,6 @@ AsciiStrDecimalToUint64S ( character that is a not a valid hexadecimal character or Null-terminator, whichever on comes first. - If String is NULL, then ASSERT(). - If Data is NULL, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero, and String contains more than - PcdMaximumAsciiStringLength Ascii characters, not including the - Null-terminator, then ASSERT(). - If String has no valid hexadecimal digits in the above format, then 0 is stored at the location pointed to by Data. If the number represented by String exceeds the range defined by UINTN, then @@ -977,12 +927,6 @@ AsciiStrHexToUintnS ( character that is a not a valid hexadecimal character or Null-terminator, whichever on comes first. - If String is NULL, then ASSERT(). - If Data is NULL, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero, and String contains more than - PcdMaximumAsciiStringLength Ascii characters, not including the - Null-terminator, then ASSERT(). - If String has no valid hexadecimal digits in the above format, then 0 is stored at the location pointed to by Data. If the number represented by String exceeds the range defined by UINT64, then @@ -1533,16 +1477,8 @@ 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 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(). - If EndPointer is not NULL and Address is translated from String, a pointer to the character that stopped the scan is stored at the location pointed to by EndPointer. @@ -1594,16 +1530,8 @@ 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 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(). - If EndPointer is not NULL and Address is translated from String, a pointer to the character that stopped the scan is stored at the location pointed to by EndPointer. @@ -1667,8 +1595,6 @@ StrToIpv4Address ( oo Data4[48:55] pp Data4[56:63] - 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. @@ -1703,17 +1629,6 @@ StrToGuid ( If String is not aligned in a 16-bit boundary, then ASSERT(). - 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. @param Length The number of Unicode characters to decode. @param Buffer Pointer to the converted bytes array. @@ -1804,7 +1719,6 @@ UnicodeStrToAsciiStr ( the upper 8 bits, then ASSERT(). If Source is not aligned on a 16-bit boundary, then ASSERT(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -1851,7 +1765,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -2415,10 +2328,6 @@ 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. - 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 to the character that stopped the scan is stored at the location pointed to by EndPointer. @@ -2470,10 +2379,6 @@ 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. - 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 to the character that stopped the scan is stored at the location pointed to by EndPointer. @@ -2535,9 +2440,6 @@ AsciiStrToIpv4Address ( oo Data4[48:55] pp Data4[56:63] - If String is NULL, then ASSERT(). - If Guid is NULL, then ASSERT(). - @param String Pointer to a Null-terminated ASCII string. @param Guid Pointer to the converted GUID. @@ -2568,17 +2470,6 @@ AsciiStrToGuid ( decoding stops after Length of characters and outputs Buffer containing (Length / 2) bytes. - 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. @param Length The number of ASCII characters to decode. @param Buffer Pointer to the converted bytes array. @@ -2659,7 +2550,6 @@ AsciiStrToUnicodeStr ( equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. If Destination is not aligned on a 16-bit boundary, then ASSERT(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -2705,7 +2595,6 @@ AsciiStrToUnicodeStrS ( ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. If Destination is not aligned on a 16-bit boundary, then ASSERT(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then Destination and DestinationLength are unmodified. diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c index 7dc03d2caa..3bb23ca1a1 100644 --- a/MdePkg/Library/BaseLib/SafeString.c +++ b/MdePkg/Library/BaseLib/SafeString.c @@ -14,8 +14,10 @@ #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status) \ do { \ - ASSERT (Expression); \ if (!(Expression)) { \ + DEBUG ((DEBUG_VERBOSE, \ + "%a(%d) %a: SAFE_STRING_CONSTRAINT_CHECK(%a) failed. Return %r\n", \ + __FILE__, __LINE__, __FUNCTION__, #Expression, Status)); \ return Status; \ } \ } while (FALSE) @@ -197,7 +199,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -279,7 +280,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -372,7 +372,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -473,7 +472,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -590,12 +588,7 @@ 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 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(). If String has no valid decimal digits in the above format, then 0 is stored at the location pointed to by Data. @@ -705,12 +698,7 @@ 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 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(). If String has no valid decimal digits in the above format, then 0 is stored at the location pointed to by Data. @@ -825,12 +813,7 @@ StrDecimalToUint64S ( the first character that is a not a valid hexadecimal character or NULL, whichever one comes first. - 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(). If String has no valid hexadecimal digits in the above format, then 0 is stored at the location pointed to by Data. @@ -956,12 +939,7 @@ StrHexToUintnS ( the first character that is a not a valid hexadecimal character or NULL, whichever one comes first. - 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(). If String has no valid hexadecimal digits in the above format, then 0 is stored at the location pointed to by Data. @@ -1091,16 +1069,8 @@ StrHexToUint64S ( "::" 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 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(). - If EndPointer is not NULL and Address is translated from String, a pointer to the character that stopped the scan is stored at the location pointed to by EndPointer. @@ -1317,16 +1287,8 @@ 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 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(). - If EndPointer is not NULL and Address is translated from String, a pointer to the character that stopped the scan is stored at the location pointed to by EndPointer. @@ -1482,8 +1444,6 @@ StrToIpv4Address ( oo Data4[48:55] pp Data4[56:63] - 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. @@ -1589,17 +1549,6 @@ StrToGuid ( If String is not aligned in a 16-bit boundary, then ASSERT(). - 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. @param Length The number of Unicode characters to decode. @param Buffer Pointer to the converted bytes array. @@ -1779,8 +1728,6 @@ AsciiStrnSizeS ( This function is similar as strcpy_s defined in C11. - If an error would be returned, then the function will also ASSERT(). - If an error is returned, then the Destination is unmodified. @param Destination A pointer to a Null-terminated Ascii string. @@ -1856,8 +1803,6 @@ AsciiStrCpyS ( This function is similar as strncpy_s defined in C11. - If an error would be returned, then the function will also ASSERT(). - If an error is returned, then the Destination is unmodified. @param Destination A pointer to a Null-terminated Ascii string. @@ -1944,8 +1889,6 @@ AsciiStrnCpyS ( This function is similar as strcat_s defined in C11. - If an error would be returned, then the function will also ASSERT(). - If an error is returned, then the Destination is unmodified. @param Destination A pointer to a Null-terminated Ascii string. @@ -2040,8 +1983,6 @@ AsciiStrCatS ( This function is similar as strncat_s defined in C11. - If an error would be returned, then the function will also ASSERT(). - If an error is returned, then the Destination is unmodified. @param Destination A pointer to a Null-terminated Ascii string. @@ -2154,12 +2095,6 @@ 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. - If String is NULL, then ASSERT(). - If Data is NULL, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero, and String contains more than - PcdMaximumAsciiStringLength Ascii characters, not including the - Null-terminator, then ASSERT(). - If String has no valid decimal digits in the above format, then 0 is stored at the location pointed to by Data. If the number represented by String exceeds the range defined by UINTN, then @@ -2266,12 +2201,6 @@ 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. - If String is NULL, then ASSERT(). - If Data is NULL, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero, and String contains more than - PcdMaximumAsciiStringLength Ascii characters, not including the - Null-terminator, then ASSERT(). - If String has no valid decimal digits in the above format, then 0 is stored at the location pointed to by Data. If the number represented by String exceeds the range defined by UINT64, then @@ -2382,12 +2311,6 @@ AsciiStrDecimalToUint64S ( character that is a not a valid hexadecimal character or Null-terminator, whichever on comes first. - If String is NULL, then ASSERT(). - If Data is NULL, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero, and String contains more than - PcdMaximumAsciiStringLength Ascii characters, not including the - Null-terminator, then ASSERT(). - If String has no valid hexadecimal digits in the above format, then 0 is stored at the location pointed to by Data. If the number represented by String exceeds the range defined by UINTN, then @@ -2509,12 +2432,6 @@ AsciiStrHexToUintnS ( character that is a not a valid hexadecimal character or Null-terminator, whichever on comes first. - If String is NULL, then ASSERT(). - If Data is NULL, then ASSERT(). - If PcdMaximumAsciiStringLength is not zero, and String contains more than - PcdMaximumAsciiStringLength Ascii characters, not including the - Null-terminator, then ASSERT(). - If String has no valid hexadecimal digits in the above format, then 0 is stored at the location pointed to by Data. If the number represented by String exceeds the range defined by UINT64, then @@ -2635,7 +2552,6 @@ AsciiStrHexToUint64S ( the upper 8 bits, then ASSERT(). If Source is not aligned on a 16-bit boundary, then ASSERT(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -2735,7 +2651,6 @@ 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(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then Destination and DestinationLength are unmodified. @@ -2855,7 +2770,6 @@ UnicodeStrnToAsciiStrS ( equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. If Destination is not aligned on a 16-bit boundary, then ASSERT(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then the Destination is unmodified. @@ -2948,7 +2862,6 @@ AsciiStrToUnicodeStrS ( ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. If Destination is not aligned on a 16-bit boundary, then ASSERT(). - If an error would be returned, then the function will also ASSERT(). If an error is returned, then Destination and DestinationLength are unmodified. @@ -3072,10 +2985,6 @@ AsciiStrnToUnicodeStrS ( "::" 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 NULL, then ASSERT(). - - If Address is NULL, then ASSERT(). - If EndPointer is not NULL and Address is translated from String, a pointer to the character that stopped the scan is stored at the location pointed to by EndPointer. @@ -3291,10 +3200,6 @@ 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. - 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 to the character that stopped the scan is stored at the location pointed to by EndPointer. @@ -3448,9 +3353,6 @@ AsciiStrToIpv4Address ( oo Data4[48:55] pp Data4[56:63] - If String is NULL, then ASSERT(). - If Guid is NULL, then ASSERT(). - @param String Pointer to a Null-terminated ASCII string. @param Guid Pointer to the converted GUID. @@ -3550,17 +3452,6 @@ AsciiStrToGuid ( decoding stops after Length of characters and outputs Buffer containing (Length / 2) bytes. - 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. @param Length The number of ASCII characters to decode. @param Buffer Pointer to the converted bytes array. -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks 2020-05-20 3:01 ` [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks Michael D Kinney @ 2020-05-20 5:56 ` Vitaly Cheptsov 2020-05-20 17:18 ` [EXTERNAL] Re: [edk2-devel] " Bret Barkelew 2020-05-20 14:50 ` Liming Gao 1 sibling, 1 reply; 13+ messages in thread From: Vitaly Cheptsov @ 2020-05-20 5:56 UTC (permalink / raw) To: Michael D Kinney, Laszlo Ersek Cc: devel, Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chasel Chiu, Jordan Justen, Leif Lindholm, Liming Gao, Marvin H?user, Vincent Zimmer, Zhichao Gao, Jiewen Yao [-- Attachment #1: Type: text/plain, Size: 30152 bytes --] Mike, Looks perfect to me. For everyone: the only change from V7 is an addition of DEBUG_VERBOSE message, which can indeed be useful. Best wishes, Vitaly > 20 мая 2020 г., в 06:01, Michael D Kinney <michael.d.kinney@intel.com> написал(а): > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 > > Runtime checks returned via status return code should not work as > assertions to permit parsing not trusted data with SafeString > interfaces. Replace ASSERT() with a DEBUG_VERBOSE message. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Bret Barkelew <bret.barkelew@microsoft.com> > Cc: Brian J. Johnson <brian.johnson@hpe.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif@nuviainc.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Marvin H?user <mhaeuser@outlook.de> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Vincent Zimmer <vincent.zimmer@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com> > --- > MdePkg/Include/Library/BaseLib.h | 111 --------------------------- > MdePkg/Library/BaseLib/SafeString.c | 115 +--------------------------- > 2 files changed, 3 insertions(+), 223 deletions(-) > > diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h > index b0bbe8cef8..8e7b87cbda 100644 > --- a/MdePkg/Include/Library/BaseLib.h > +++ b/MdePkg/Include/Library/BaseLib.h > @@ -216,7 +216,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -252,7 +251,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -290,7 +288,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -330,7 +327,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -377,12 +373,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -433,12 +424,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -494,12 +480,7 @@ StrDecimalToUint64S ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -555,12 +536,7 @@ StrHexToUintnS ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -649,8 +625,6 @@ AsciiStrnSizeS ( > > This function is similar as strcpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -683,8 +657,6 @@ AsciiStrCpyS ( > > This function is similar as strncpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -719,8 +691,6 @@ AsciiStrnCpyS ( > > This function is similar as strcat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -757,8 +727,6 @@ AsciiStrCatS ( > > This function is similar as strncat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -804,12 +772,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -859,12 +821,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -918,12 +874,6 @@ AsciiStrDecimalToUint64S ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -977,12 +927,6 @@ AsciiStrHexToUintnS ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -1533,16 +1477,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1594,16 +1530,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1667,8 +1595,6 @@ StrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - 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. > @@ -1703,17 +1629,6 @@ StrToGuid ( > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > - 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. > @param Length The number of Unicode characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -1804,7 +1719,6 @@ UnicodeStrToAsciiStr ( > the upper 8 bits, then ASSERT(). > > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -1851,7 +1765,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2415,10 +2328,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -2470,10 +2379,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -2535,9 +2440,6 @@ AsciiStrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - If String is NULL, then ASSERT(). > - If Guid is NULL, then ASSERT(). > - > @param String Pointer to a Null-terminated ASCII string. > @param Guid Pointer to the converted GUID. > > @@ -2568,17 +2470,6 @@ AsciiStrToGuid ( > decoding stops after Length of characters and outputs Buffer containing > (Length / 2) bytes. > > - 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. > @param Length The number of ASCII characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -2659,7 +2550,6 @@ AsciiStrToUnicodeStr ( > equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2705,7 +2595,6 @@ AsciiStrToUnicodeStrS ( > ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c > index 7dc03d2caa..3bb23ca1a1 100644 > --- a/MdePkg/Library/BaseLib/SafeString.c > +++ b/MdePkg/Library/BaseLib/SafeString.c > @@ -14,8 +14,10 @@ > > #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status) \ > do { \ > - ASSERT (Expression); \ > if (!(Expression)) { \ > + DEBUG ((DEBUG_VERBOSE, \ > + "%a(%d) %a: SAFE_STRING_CONSTRAINT_CHECK(%a) failed. Return %r\n", \ > + __FILE__, __LINE__, __FUNCTION__, #Expression, Status)); \ > return Status; \ > } \ > } while (FALSE) > @@ -197,7 +199,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -279,7 +280,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -372,7 +372,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -473,7 +472,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -590,12 +588,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -705,12 +698,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -825,12 +813,7 @@ StrDecimalToUint64S ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -956,12 +939,7 @@ StrHexToUintnS ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -1091,16 +1069,8 @@ StrHexToUint64S ( > "::" 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1317,16 +1287,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1482,8 +1444,6 @@ StrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - 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. > @@ -1589,17 +1549,6 @@ StrToGuid ( > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > - 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. > @param Length The number of Unicode characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -1779,8 +1728,6 @@ AsciiStrnSizeS ( > > This function is similar as strcpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -1856,8 +1803,6 @@ AsciiStrCpyS ( > > This function is similar as strncpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -1944,8 +1889,6 @@ AsciiStrnCpyS ( > > This function is similar as strcat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -2040,8 +1983,6 @@ AsciiStrCatS ( > > This function is similar as strncat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -2154,12 +2095,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -2266,12 +2201,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -2382,12 +2311,6 @@ AsciiStrDecimalToUint64S ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -2509,12 +2432,6 @@ AsciiStrHexToUintnS ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -2635,7 +2552,6 @@ AsciiStrHexToUint64S ( > the upper 8 bits, then ASSERT(). > > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2735,7 +2651,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > @@ -2855,7 +2770,6 @@ UnicodeStrnToAsciiStrS ( > equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2948,7 +2862,6 @@ AsciiStrToUnicodeStrS ( > ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > @@ -3072,10 +2985,6 @@ AsciiStrnToUnicodeStrS ( > "::" 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 NULL, then ASSERT(). > - > - If Address is NULL, then ASSERT(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -3291,10 +3200,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -3448,9 +3353,6 @@ AsciiStrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - If String is NULL, then ASSERT(). > - If Guid is NULL, then ASSERT(). > - > @param String Pointer to a Null-terminated ASCII string. > @param Guid Pointer to the converted GUID. > > @@ -3550,17 +3452,6 @@ AsciiStrToGuid ( > decoding stops after Length of characters and outputs Buffer containing > (Length / 2) bytes. > > - 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. > @param Length The number of ASCII characters to decode. > @param Buffer Pointer to the converted bytes array. > -- > 2.21.0.windows.1 > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [EXTERNAL] Re: [edk2-devel] [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks 2020-05-20 5:56 ` Vitaly Cheptsov @ 2020-05-20 17:18 ` Bret Barkelew 0 siblings, 0 replies; 13+ messages in thread From: Bret Barkelew @ 2020-05-20 17:18 UTC (permalink / raw) To: devel@edk2.groups.io, vit9696@protonmail.com, Kinney, Michael D, Laszlo Ersek Cc: devel@edk2.groups.io, Andrew Fish, Ard Biesheuvel, Brian J . Johnson, Chasel Chiu, Jordan Justen, Leif Lindholm, liming.gao, Marvin H?user, Zimmer, Vincent, Zhichao Gao, Yao, Jiewen [-- Attachment #1: Type: text/plain, Size: 31019 bytes --] Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com> - Bret From: Vitaly Cheptsov via groups.io<mailto:vit9696=protonmail.com@groups.io> Sent: Tuesday, May 19, 2020 10:56 PM To: Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Laszlo Ersek<mailto:lersek@redhat.com> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Andrew Fish<mailto:afish@apple.com>; Ard Biesheuvel<mailto:ard.biesheuvel@linaro.org>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Brian J . Johnson<mailto:brian.johnson@hpe.com>; Chasel Chiu<mailto:chasel.chiu@intel.com>; Jordan Justen<mailto:jordan.l.justen@intel.com>; Leif Lindholm<mailto:leif@nuviainc.com>; liming.gao<mailto:liming.gao@intel.com>; Marvin H?user<mailto:mhaeuser@outlook.de>; Zimmer, Vincent<mailto:vincent.zimmer@intel.com>; Zhichao Gao<mailto:zhichao.gao@intel.com>; Yao, Jiewen<mailto:jiewen.yao@intel.com> Subject: [EXTERNAL] Re: [edk2-devel] [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks Mike, Looks perfect to me. For everyone: the only change from V7 is an addition of DEBUG_VERBOSE message, which can indeed be useful. Best wishes, Vitaly > 20 мая 2020 г., в 06:01, Michael D Kinney <michael.d.kinney@intel.com> написал(а): > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 > > Runtime checks returned via status return code should not work as > assertions to permit parsing not trusted data with SafeString > interfaces. Replace ASSERT() with a DEBUG_VERBOSE message. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Bret Barkelew <bret.barkelew@microsoft.com> > Cc: Brian J. Johnson <brian.johnson@hpe.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif@nuviainc.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Marvin H?user <mhaeuser@outlook.de> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Vincent Zimmer <vincent.zimmer@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com> > --- > MdePkg/Include/Library/BaseLib.h | 111 --------------------------- > MdePkg/Library/BaseLib/SafeString.c | 115 +--------------------------- > 2 files changed, 3 insertions(+), 223 deletions(-) > > diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h > index b0bbe8cef8..8e7b87cbda 100644 > --- a/MdePkg/Include/Library/BaseLib.h > +++ b/MdePkg/Include/Library/BaseLib.h > @@ -216,7 +216,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -252,7 +251,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -290,7 +288,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -330,7 +327,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -377,12 +373,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -433,12 +424,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -494,12 +480,7 @@ StrDecimalToUint64S ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -555,12 +536,7 @@ StrHexToUintnS ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -649,8 +625,6 @@ AsciiStrnSizeS ( > > This function is similar as strcpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -683,8 +657,6 @@ AsciiStrCpyS ( > > This function is similar as strncpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -719,8 +691,6 @@ AsciiStrnCpyS ( > > This function is similar as strcat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -757,8 +727,6 @@ AsciiStrCatS ( > > This function is similar as strncat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -804,12 +772,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -859,12 +821,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -918,12 +874,6 @@ AsciiStrDecimalToUint64S ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -977,12 +927,6 @@ AsciiStrHexToUintnS ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -1533,16 +1477,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1594,16 +1530,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1667,8 +1595,6 @@ StrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - 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. > @@ -1703,17 +1629,6 @@ StrToGuid ( > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > - 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. > @param Length The number of Unicode characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -1804,7 +1719,6 @@ UnicodeStrToAsciiStr ( > the upper 8 bits, then ASSERT(). > > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -1851,7 +1765,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2415,10 +2328,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -2470,10 +2379,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -2535,9 +2440,6 @@ AsciiStrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - If String is NULL, then ASSERT(). > - If Guid is NULL, then ASSERT(). > - > @param String Pointer to a Null-terminated ASCII string. > @param Guid Pointer to the converted GUID. > > @@ -2568,17 +2470,6 @@ AsciiStrToGuid ( > decoding stops after Length of characters and outputs Buffer containing > (Length / 2) bytes. > > - 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. > @param Length The number of ASCII characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -2659,7 +2550,6 @@ AsciiStrToUnicodeStr ( > equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2705,7 +2595,6 @@ AsciiStrToUnicodeStrS ( > ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c > index 7dc03d2caa..3bb23ca1a1 100644 > --- a/MdePkg/Library/BaseLib/SafeString.c > +++ b/MdePkg/Library/BaseLib/SafeString.c > @@ -14,8 +14,10 @@ > > #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status) \ > do { \ > - ASSERT (Expression); \ > if (!(Expression)) { \ > + DEBUG ((DEBUG_VERBOSE, \ > + "%a(%d) %a: SAFE_STRING_CONSTRAINT_CHECK(%a) failed. Return %r\n", \ > + __FILE__, __LINE__, __FUNCTION__, #Expression, Status)); \ > return Status; \ > } \ > } while (FALSE) > @@ -197,7 +199,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -279,7 +280,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -372,7 +372,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -473,7 +472,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -590,12 +588,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -705,12 +698,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -825,12 +813,7 @@ StrDecimalToUint64S ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -956,12 +939,7 @@ StrHexToUintnS ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -1091,16 +1069,8 @@ StrHexToUint64S ( > "::" 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1317,16 +1287,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1482,8 +1444,6 @@ StrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - 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. > @@ -1589,17 +1549,6 @@ StrToGuid ( > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > - 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. > @param Length The number of Unicode characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -1779,8 +1728,6 @@ AsciiStrnSizeS ( > > This function is similar as strcpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -1856,8 +1803,6 @@ AsciiStrCpyS ( > > This function is similar as strncpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -1944,8 +1889,6 @@ AsciiStrnCpyS ( > > This function is similar as strcat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -2040,8 +1983,6 @@ AsciiStrCatS ( > > This function is similar as strncat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -2154,12 +2095,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -2266,12 +2201,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -2382,12 +2311,6 @@ AsciiStrDecimalToUint64S ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -2509,12 +2432,6 @@ AsciiStrHexToUintnS ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -2635,7 +2552,6 @@ AsciiStrHexToUint64S ( > the upper 8 bits, then ASSERT(). > > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2735,7 +2651,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > @@ -2855,7 +2770,6 @@ UnicodeStrnToAsciiStrS ( > equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2948,7 +2862,6 @@ AsciiStrToUnicodeStrS ( > ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > @@ -3072,10 +2985,6 @@ AsciiStrnToUnicodeStrS ( > "::" 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 NULL, then ASSERT(). > - > - If Address is NULL, then ASSERT(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -3291,10 +3200,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -3448,9 +3353,6 @@ AsciiStrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - If String is NULL, then ASSERT(). > - If Guid is NULL, then ASSERT(). > - > @param String Pointer to a Null-terminated ASCII string. > @param Guid Pointer to the converted GUID. > > @@ -3550,17 +3452,6 @@ AsciiStrToGuid ( > decoding stops after Length of characters and outputs Buffer containing > (Length / 2) bytes. > > - 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. > @param Length The number of ASCII characters to decode. > @param Buffer Pointer to the converted bytes array. > -- > 2.21.0.windows.1 > [-- Attachment #2: Type: text/html, Size: 59700 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks 2020-05-20 3:01 ` [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks Michael D Kinney 2020-05-20 5:56 ` Vitaly Cheptsov @ 2020-05-20 14:50 ` Liming Gao 1 sibling, 0 replies; 13+ messages in thread From: Liming Gao @ 2020-05-20 14:50 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io Cc: Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chiu, Chasel, Justen, Jordan L, Laszlo Ersek, Leif Lindholm, Marvin H?user, Zimmer, Vincent, Gao, Zhichao, Yao, Jiewen, Vitaly Cheptsov Reviewed-by: Liming Gao <liming.gao@intel.com> > -----Original Message----- > From: Kinney, Michael D <michael.d.kinney@intel.com> > Sent: Wednesday, May 20, 2020 11:01 AM > To: devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Bret Barkelew <bret.barkelew@microsoft.com>; > Brian J . Johnson <brian.johnson@hpe.com>; Chiu, Chasel <chasel.chiu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; > Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Gao, Liming <liming.gao@intel.com>; Marvin H?user > <mhaeuser@outlook.de>; Zimmer, Vincent <vincent.zimmer@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com> > Subject: [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 > > Runtime checks returned via status return code should not work as > assertions to permit parsing not trusted data with SafeString > interfaces. Replace ASSERT() with a DEBUG_VERBOSE message. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Bret Barkelew <bret.barkelew@microsoft.com> > Cc: Brian J. Johnson <brian.johnson@hpe.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif@nuviainc.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Marvin H?user <mhaeuser@outlook.de> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Vincent Zimmer <vincent.zimmer@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com> > --- > MdePkg/Include/Library/BaseLib.h | 111 --------------------------- > MdePkg/Library/BaseLib/SafeString.c | 115 +--------------------------- > 2 files changed, 3 insertions(+), 223 deletions(-) > > diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h > index b0bbe8cef8..8e7b87cbda 100644 > --- a/MdePkg/Include/Library/BaseLib.h > +++ b/MdePkg/Include/Library/BaseLib.h > @@ -216,7 +216,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -252,7 +251,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -290,7 +288,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -330,7 +327,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -377,12 +373,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -433,12 +424,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -494,12 +480,7 @@ StrDecimalToUint64S ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -555,12 +536,7 @@ StrHexToUintnS ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -649,8 +625,6 @@ AsciiStrnSizeS ( > > This function is similar as strcpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -683,8 +657,6 @@ AsciiStrCpyS ( > > This function is similar as strncpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -719,8 +691,6 @@ AsciiStrnCpyS ( > > This function is similar as strcat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -757,8 +727,6 @@ AsciiStrCatS ( > > This function is similar as strncat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -804,12 +772,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -859,12 +821,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -918,12 +874,6 @@ AsciiStrDecimalToUint64S ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -977,12 +927,6 @@ AsciiStrHexToUintnS ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -1533,16 +1477,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1594,16 +1530,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1667,8 +1595,6 @@ StrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - 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. > @@ -1703,17 +1629,6 @@ StrToGuid ( > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > - 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. > @param Length The number of Unicode characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -1804,7 +1719,6 @@ UnicodeStrToAsciiStr ( > the upper 8 bits, then ASSERT(). > > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -1851,7 +1765,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2415,10 +2328,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -2470,10 +2379,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -2535,9 +2440,6 @@ AsciiStrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - If String is NULL, then ASSERT(). > - If Guid is NULL, then ASSERT(). > - > @param String Pointer to a Null-terminated ASCII string. > @param Guid Pointer to the converted GUID. > > @@ -2568,17 +2470,6 @@ AsciiStrToGuid ( > decoding stops after Length of characters and outputs Buffer containing > (Length / 2) bytes. > > - 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. > @param Length The number of ASCII characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -2659,7 +2550,6 @@ AsciiStrToUnicodeStr ( > equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2705,7 +2595,6 @@ AsciiStrToUnicodeStrS ( > ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c > index 7dc03d2caa..3bb23ca1a1 100644 > --- a/MdePkg/Library/BaseLib/SafeString.c > +++ b/MdePkg/Library/BaseLib/SafeString.c > @@ -14,8 +14,10 @@ > > #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status) \ > do { \ > - ASSERT (Expression); \ > if (!(Expression)) { \ > + DEBUG ((DEBUG_VERBOSE, \ > + "%a(%d) %a: SAFE_STRING_CONSTRAINT_CHECK(%a) failed. Return %r\n", \ > + __FILE__, __LINE__, __FUNCTION__, #Expression, Status)); \ > return Status; \ > } \ > } while (FALSE) > @@ -197,7 +199,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -279,7 +280,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -372,7 +372,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -473,7 +472,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -590,12 +588,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -705,12 +698,7 @@ 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 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(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -825,12 +813,7 @@ StrDecimalToUint64S ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -956,12 +939,7 @@ StrHexToUintnS ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - 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(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -1091,16 +1069,8 @@ StrHexToUint64S ( > "::" 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1317,16 +1287,8 @@ 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 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(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -1482,8 +1444,6 @@ StrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - 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. > @@ -1589,17 +1549,6 @@ StrToGuid ( > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > - 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. > @param Length The number of Unicode characters to decode. > @param Buffer Pointer to the converted bytes array. > @@ -1779,8 +1728,6 @@ AsciiStrnSizeS ( > > This function is similar as strcpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -1856,8 +1803,6 @@ AsciiStrCpyS ( > > This function is similar as strncpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -1944,8 +1889,6 @@ AsciiStrnCpyS ( > > This function is similar as strcat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -2040,8 +1983,6 @@ AsciiStrCatS ( > > This function is similar as strncat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > - > If an error is returned, then the Destination is unmodified. > > @param Destination A pointer to a Null-terminated Ascii string. > @@ -2154,12 +2095,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -2266,12 +2201,6 @@ 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. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -2382,12 +2311,6 @@ AsciiStrDecimalToUint64S ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINTN, then > @@ -2509,12 +2432,6 @@ AsciiStrHexToUintnS ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > - If PcdMaximumAsciiStringLength is not zero, and String contains more than > - PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > - > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > If the number represented by String exceeds the range defined by UINT64, then > @@ -2635,7 +2552,6 @@ AsciiStrHexToUint64S ( > the upper 8 bits, then ASSERT(). > > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2735,7 +2651,6 @@ 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(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > @@ -2855,7 +2770,6 @@ UnicodeStrnToAsciiStrS ( > equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2948,7 +2862,6 @@ AsciiStrToUnicodeStrS ( > ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > @@ -3072,10 +2985,6 @@ AsciiStrnToUnicodeStrS ( > "::" 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 NULL, then ASSERT(). > - > - If Address is NULL, then ASSERT(). > - > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -3291,10 +3200,6 @@ 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. > > - 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 > to the character that stopped the scan is stored at the location pointed to > by EndPointer. > @@ -3448,9 +3353,6 @@ AsciiStrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - If String is NULL, then ASSERT(). > - If Guid is NULL, then ASSERT(). > - > @param String Pointer to a Null-terminated ASCII string. > @param Guid Pointer to the converted GUID. > > @@ -3550,17 +3452,6 @@ AsciiStrToGuid ( > decoding stops after Length of characters and outputs Buffer containing > (Length / 2) bytes. > > - 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. > @param Length The number of ASCII characters to decode. > @param Buffer Pointer to the converted bytes array. > -- > 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test 2020-05-20 3:01 [Patch v8 0/2] Disable safe string constraint assertions Michael D Kinney 2020-05-20 3:01 ` [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks Michael D Kinney @ 2020-05-20 3:01 ` Michael D Kinney 2020-05-20 19:04 ` [edk2-devel] " Sean 1 sibling, 1 reply; 13+ messages in thread From: Michael D Kinney @ 2020-05-20 3:01 UTC (permalink / raw) To: devel Cc: Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chasel Chiu, Jordan Justen, Laszlo Ersek, Leif Lindholm, Liming Gao, Marvin H?user, Vincent Zimmer, Zhichao Gao, Jiewen Yao, Vitaly Cheptsov Use the safe string function StrCpyS() in BaseLib to test the SAFE_STRING_CONSTRAINT_CHECK() macro. Cc: Andrew Fish <afish@apple.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Bret Barkelew <bret.barkelew@microsoft.com> Cc: Brian J. Johnson <brian.johnson@hpe.com> Cc: Chasel Chiu <chasel.chiu@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Marvin H?user <mhaeuser@outlook.de> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Vincent Zimmer <vincent.zimmer@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Vitaly Cheptsov <vit9696@protonmail.com> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> --- .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c index 8952f9da6c..5aced69e0d 100644 --- a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c +++ b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c @@ -290,6 +290,77 @@ RfcDecodeTest( return UNIT_TEST_PASSED; } +#define SOURCE_STRING L"Hello" + +STATIC +UNIT_TEST_STATUS +EFIAPI +SafeStringContraintCheckTest ( + IN UNIT_TEST_CONTEXT Context + ) +{ + RETURN_STATUS Status; + CHAR16 Destination[20]; + + // + // Positive test case copy source unicode string to destination + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); + + // + // Positive test case with DestMax the same as Source size + // + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); + + // + // Negative test case with Destination NULL + // + Status = StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with Source NULL + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), NULL); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax too big + // + Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax 0 + // + Status = StrCpyS (Destination, 0, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax smaller than Source size + // + Status = StrCpyS (Destination, 1, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); + + // + // Negative test case with DestMax smaller than Source size by one character + // + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); + + // + // Negative test case with DestMax smaller than Source size + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_ACCESS_DENIED); + + return UNIT_TEST_PASSED; +} + /** Initialze the unit test framework, suite, and unit tests for the Base64 conversion APIs of BaseLib and run the unit tests. @@ -309,6 +380,7 @@ UnitTestingEntry ( UNIT_TEST_FRAMEWORK_HANDLE Fw; UNIT_TEST_SUITE_HANDLE b64EncodeTests; UNIT_TEST_SUITE_HANDLE b64DecodeTests; + UNIT_TEST_SUITE_HANDLE SafeStringTests; Fw = NULL; @@ -367,6 +439,19 @@ UnitTestingEntry ( AddTestCase (b64DecodeTests, "Incorrectly placed padding character", "Error4", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError4); AddTestCase (b64DecodeTests, "Too small of output buffer", "Error5", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError5); + // + // Populate the safe string Unit Test Suite. + // + Status = CreateUnitTestSuite (&SafeStringTests, Fw, "Safe String", "BaseLib.SafeString", NULL, NULL); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for SafeStringTests\n")); + Status = EFI_OUT_OF_RESOURCES; + goto EXIT; + } + + // --------------Suite-----------Description--------------Class Name----------Function--------Pre---Post-------------------Context----------- + AddTestCase (SafeStringTests, "SAFE_STRING_CONSTRAINT_CHECK", "SafeStringContraintCheckTest", SafeStringContraintCheckTest, NULL, NULL, NULL); + // // Execute the tests. // -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test 2020-05-20 3:01 ` [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Michael D Kinney @ 2020-05-20 19:04 ` Sean 2020-05-20 20:11 ` Michael D Kinney 0 siblings, 1 reply; 13+ messages in thread From: Sean @ 2020-05-20 19:04 UTC (permalink / raw) To: devel, michael.d.kinney Cc: Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chasel Chiu, Jordan Justen, Laszlo Ersek, Leif Lindholm, Liming Gao, Marvin H?user, Vincent Zimmer, Zhichao Gao, Jiewen Yao, Vitaly Cheptsov Mike, I would have thought the SafeString tests would have gone in a different c file. Base64UnitTest.c seems by its title to be targeted at the base64 encode/decode test. Looking at this i do see that would require some restructuring as there is no BaseLibUnitTest.c file for the common test part. As the author of this test originally, I can see that i didn't set it up to scale to the entire baselib very well. Sorry. Thanks Sean On 5/19/2020 8:01 PM, Michael D Kinney wrote: > Use the safe string function StrCpyS() in BaseLib to test the > SAFE_STRING_CONSTRAINT_CHECK() macro. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Bret Barkelew <bret.barkelew@microsoft.com> > Cc: Brian J. Johnson <brian.johnson@hpe.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif@nuviainc.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Marvin H?user <mhaeuser@outlook.de> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Vincent Zimmer <vincent.zimmer@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Vitaly Cheptsov <vit9696@protonmail.com> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> > --- > .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > index 8952f9da6c..5aced69e0d 100644 > --- a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > +++ b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > @@ -290,6 +290,77 @@ RfcDecodeTest( > return UNIT_TEST_PASSED; > } > > +#define SOURCE_STRING L"Hello" > + > +STATIC > +UNIT_TEST_STATUS > +EFIAPI > +SafeStringContraintCheckTest ( > + IN UNIT_TEST_CONTEXT Context > + ) > +{ > + RETURN_STATUS Status; > + CHAR16 Destination[20]; > + > + // > + // Positive test case copy source unicode string to destination > + // > + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); > + UT_ASSERT_NOT_EFI_ERROR (Status); > + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); > + > + // > + // Positive test case with DestMax the same as Source size > + // > + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); > + UT_ASSERT_NOT_EFI_ERROR (Status); > + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); > + > + // > + // Negative test case with Destination NULL > + // > + Status = StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with Source NULL > + // > + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), NULL); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax too big > + // > + Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax 0 > + // > + Status = StrCpyS (Destination, 0, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax smaller than Source size > + // > + Status = StrCpyS (Destination, 1, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); > + > + // > + // Negative test case with DestMax smaller than Source size by one character > + // > + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); > + > + // > + // Negative test case with DestMax smaller than Source size > + // > + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_ACCESS_DENIED); > + > + return UNIT_TEST_PASSED; > +} > + > /** > Initialze the unit test framework, suite, and unit tests for the > Base64 conversion APIs of BaseLib and run the unit tests. > @@ -309,6 +380,7 @@ UnitTestingEntry ( > UNIT_TEST_FRAMEWORK_HANDLE Fw; > UNIT_TEST_SUITE_HANDLE b64EncodeTests; > UNIT_TEST_SUITE_HANDLE b64DecodeTests; > + UNIT_TEST_SUITE_HANDLE SafeStringTests; > > Fw = NULL; > > @@ -367,6 +439,19 @@ UnitTestingEntry ( > AddTestCase (b64DecodeTests, "Incorrectly placed padding character", "Error4", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError4); > AddTestCase (b64DecodeTests, "Too small of output buffer", "Error5", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError5); > > + // > + // Populate the safe string Unit Test Suite. > + // > + Status = CreateUnitTestSuite (&SafeStringTests, Fw, "Safe String", "BaseLib.SafeString", NULL, NULL); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for SafeStringTests\n")); > + Status = EFI_OUT_OF_RESOURCES; > + goto EXIT; > + } > + > + // --------------Suite-----------Description--------------Class Name----------Function--------Pre---Post-------------------Context----------- > + AddTestCase (SafeStringTests, "SAFE_STRING_CONSTRAINT_CHECK", "SafeStringContraintCheckTest", SafeStringContraintCheckTest, NULL, NULL, NULL); > + > // > // Execute the tests. > // > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test 2020-05-20 19:04 ` [edk2-devel] " Sean @ 2020-05-20 20:11 ` Michael D Kinney 2020-05-20 20:16 ` Sean 0 siblings, 1 reply; 13+ messages in thread From: Michael D Kinney @ 2020-05-20 20:11 UTC (permalink / raw) To: devel@edk2.groups.io, spbrogan@outlook.com, Kinney, Michael D Cc: Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chiu, Chasel, Justen, Jordan L, Laszlo Ersek, Leif Lindholm, Gao, Liming, Marvin H?user, Zimmer, Vincent, Gao, Zhichao, Yao, Jiewen, Vitaly Cheptsov Hi Sean, I agree that tis unit test can be reorganized a bit to support unit tests for all APIs in BaseLib. Can you enter a BZ for this and we can work on cleaning this up after the stable tag? Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On > Behalf Of Sean > Sent: Wednesday, May 20, 2020 12:05 PM > To: devel@edk2.groups.io; Kinney, Michael D > <michael.d.kinney@intel.com> > Cc: Andrew Fish <afish@apple.com>; Ard Biesheuvel > <ard.biesheuvel@linaro.org>; Bret Barkelew > <bret.barkelew@microsoft.com>; Brian J . Johnson > <brian.johnson@hpe.com>; Chiu, Chasel > <chasel.chiu@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Laszlo Ersek > <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; > Gao, Liming <liming.gao@intel.com>; Marvin H?user > <mhaeuser@outlook.de>; Zimmer, Vincent > <vincent.zimmer@intel.com>; Gao, Zhichao > <zhichao.gao@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Vitaly Cheptsov > <vit9696@protonmail.com> > Subject: Re: [edk2-devel] [Patch v8 2/2] > MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK > unit test > > Mike, > > I would have thought the SafeString tests would have > gone in a different > c file. Base64UnitTest.c seems by its title to be > targeted at the > base64 encode/decode test. > > Looking at this i do see that would require some > restructuring as there > is no BaseLibUnitTest.c file for the common test part. > As the author of > this test originally, I can see that i didn't set it up > to scale to the > entire baselib very well. Sorry. > > > Thanks > Sean > > > > > On 5/19/2020 8:01 PM, Michael D Kinney wrote: > > Use the safe string function StrCpyS() in BaseLib to > test the > > SAFE_STRING_CONSTRAINT_CHECK() macro. > > > > Cc: Andrew Fish <afish@apple.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Bret Barkelew <bret.barkelew@microsoft.com> > > Cc: Brian J. Johnson <brian.johnson@hpe.com> > > Cc: Chasel Chiu <chasel.chiu@intel.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Leif Lindholm <leif@nuviainc.com> > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Marvin H?user <mhaeuser@outlook.de> > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > Cc: Vincent Zimmer <vincent.zimmer@intel.com> > > Cc: Zhichao Gao <zhichao.gao@intel.com> > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Vitaly Cheptsov <vit9696@protonmail.com> > > Signed-off-by: Michael D Kinney > <michael.d.kinney@intel.com> > > --- > > .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 > +++++++++++++++++++ > > 1 file changed, 85 insertions(+) > > > > diff --git > a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > > index 8952f9da6c..5aced69e0d 100644 > > --- > a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > > +++ > b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > > @@ -290,6 +290,77 @@ RfcDecodeTest( > > return UNIT_TEST_PASSED; > > } > > > > +#define SOURCE_STRING L"Hello" > > + > > +STATIC > > +UNIT_TEST_STATUS > > +EFIAPI > > +SafeStringContraintCheckTest ( > > + IN UNIT_TEST_CONTEXT Context > > + ) > > +{ > > + RETURN_STATUS Status; > > + CHAR16 Destination[20]; > > + > > + // > > + // Positive test case copy source unicode string > to destination > > + // > > + Status = StrCpyS (Destination, sizeof > (Destination) / sizeof (CHAR16), SOURCE_STRING); > > + UT_ASSERT_NOT_EFI_ERROR (Status); > > + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, > sizeof (SOURCE_STRING)); > > + > > + // > > + // Positive test case with DestMax the same as > Source size > > + // > > + Status = StrCpyS (Destination, sizeof > (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); > > + UT_ASSERT_NOT_EFI_ERROR (Status); > > + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, > sizeof (SOURCE_STRING)); > > + > > + // > > + // Negative test case with Destination NULL > > + // > > + Status = StrCpyS (NULL, sizeof (Destination) / > sizeof (CHAR16), SOURCE_STRING); > > + UT_ASSERT_STATUS_EQUAL (Status, > RETURN_INVALID_PARAMETER); > > + > > + // > > + // Negative test case with Source NULL > > + // > > + Status = StrCpyS (Destination, sizeof > (Destination) / sizeof (CHAR16), NULL); > > + UT_ASSERT_STATUS_EQUAL (Status, > RETURN_INVALID_PARAMETER); > > + > > + // > > + // Negative test case with DestMax too big > > + // > > + Status = StrCpyS (Destination, MAX_UINTN, > SOURCE_STRING); > > + UT_ASSERT_STATUS_EQUAL (Status, > RETURN_INVALID_PARAMETER); > > + > > + // > > + // Negative test case with DestMax 0 > > + // > > + Status = StrCpyS (Destination, 0, SOURCE_STRING); > > + UT_ASSERT_STATUS_EQUAL (Status, > RETURN_INVALID_PARAMETER); > > + > > + // > > + // Negative test case with DestMax smaller than > Source size > > + // > > + Status = StrCpyS (Destination, 1, SOURCE_STRING); > > + UT_ASSERT_STATUS_EQUAL (Status, > RETURN_BUFFER_TOO_SMALL); > > + > > + // > > + // Negative test case with DestMax smaller than > Source size by one character > > + // > > + Status = StrCpyS (Destination, sizeof > (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING); > > + UT_ASSERT_STATUS_EQUAL (Status, > RETURN_BUFFER_TOO_SMALL); > > + > > + // > > + // Negative test case with DestMax smaller than > Source size > > + // > > + Status = StrCpyS (Destination, sizeof > (Destination) / sizeof (CHAR16), Destination); > > + UT_ASSERT_STATUS_EQUAL (Status, > RETURN_ACCESS_DENIED); > > + > > + return UNIT_TEST_PASSED; > > +} > > + > > /** > > Initialze the unit test framework, suite, and > unit tests for the > > Base64 conversion APIs of BaseLib and run the > unit tests. > > @@ -309,6 +380,7 @@ UnitTestingEntry ( > > UNIT_TEST_FRAMEWORK_HANDLE Fw; > > UNIT_TEST_SUITE_HANDLE b64EncodeTests; > > UNIT_TEST_SUITE_HANDLE b64DecodeTests; > > + UNIT_TEST_SUITE_HANDLE SafeStringTests; > > > > Fw = NULL; > > > > @@ -367,6 +439,19 @@ UnitTestingEntry ( > > AddTestCase (b64DecodeTests, "Incorrectly placed > padding character", "Error4", RfcDecodeTest, NULL, > CleanUpB64TestContext, &mBasicDecodeError4); > > AddTestCase (b64DecodeTests, "Too small of output > buffer", "Error5", RfcDecodeTest, NULL, > CleanUpB64TestContext, &mBasicDecodeError5); > > > > + // > > + // Populate the safe string Unit Test Suite. > > + // > > + Status = CreateUnitTestSuite (&SafeStringTests, > Fw, "Safe String", "BaseLib.SafeString", NULL, NULL); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_ERROR, "Failed in > CreateUnitTestSuite for SafeStringTests\n")); > > + Status = EFI_OUT_OF_RESOURCES; > > + goto EXIT; > > + } > > + > > + // --------------Suite-----------Description------ > --------Class Name----------Function--------Pre---Post- > ------------------Context----------- > > + AddTestCase (SafeStringTests, > "SAFE_STRING_CONSTRAINT_CHECK", > "SafeStringContraintCheckTest", > SafeStringContraintCheckTest, NULL, NULL, NULL); > > + > > // > > // Execute the tests. > > // > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test 2020-05-20 20:11 ` Michael D Kinney @ 2020-05-20 20:16 ` Sean 0 siblings, 0 replies; 13+ messages in thread From: Sean @ 2020-05-20 20:16 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io Cc: Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chiu, Chasel, Justen, Jordan L, Laszlo Ersek, Leif Lindholm, Gao, Liming, Marvin H?user, Zimmer, Vincent, Gao, Zhichao, Yao, Jiewen, Vitaly Cheptsov Bug created. https://bugzilla.tianocore.org/show_bug.cgi?id=2729 thanks Sean On 5/20/2020 1:11 PM, Kinney, Michael D wrote: > Hi Sean, > > I agree that tis unit test can be reorganized a bit to support > unit tests for all APIs in BaseLib. > > Can you enter a BZ for this and we can work on cleaning this > up after the stable tag? > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On >> Behalf Of Sean >> Sent: Wednesday, May 20, 2020 12:05 PM >> To: devel@edk2.groups.io; Kinney, Michael D >> <michael.d.kinney@intel.com> >> Cc: Andrew Fish <afish@apple.com>; Ard Biesheuvel >> <ard.biesheuvel@linaro.org>; Bret Barkelew >> <bret.barkelew@microsoft.com>; Brian J . Johnson >> <brian.johnson@hpe.com>; Chiu, Chasel >> <chasel.chiu@intel.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; Laszlo Ersek >> <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; >> Gao, Liming <liming.gao@intel.com>; Marvin H?user >> <mhaeuser@outlook.de>; Zimmer, Vincent >> <vincent.zimmer@intel.com>; Gao, Zhichao >> <zhichao.gao@intel.com>; Yao, Jiewen >> <jiewen.yao@intel.com>; Vitaly Cheptsov >> <vit9696@protonmail.com> >> Subject: Re: [edk2-devel] [Patch v8 2/2] >> MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK >> unit test >> >> Mike, >> >> I would have thought the SafeString tests would have >> gone in a different >> c file. Base64UnitTest.c seems by its title to be >> targeted at the >> base64 encode/decode test. >> >> Looking at this i do see that would require some >> restructuring as there >> is no BaseLibUnitTest.c file for the common test part. >> As the author of >> this test originally, I can see that i didn't set it up >> to scale to the >> entire baselib very well. Sorry. >> >> >> Thanks >> Sean >> >> >> >> >> On 5/19/2020 8:01 PM, Michael D Kinney wrote: >>> Use the safe string function StrCpyS() in BaseLib to >> test the >>> SAFE_STRING_CONSTRAINT_CHECK() macro. >>> >>> Cc: Andrew Fish <afish@apple.com> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Cc: Bret Barkelew <bret.barkelew@microsoft.com> >>> Cc: Brian J. Johnson <brian.johnson@hpe.com> >>> Cc: Chasel Chiu <chasel.chiu@intel.com> >>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>> Cc: Laszlo Ersek <lersek@redhat.com> >>> Cc: Leif Lindholm <leif@nuviainc.com> >>> Cc: Liming Gao <liming.gao@intel.com> >>> Cc: Marvin H?user <mhaeuser@outlook.de> >>> Cc: Michael D Kinney <michael.d.kinney@intel.com> >>> Cc: Vincent Zimmer <vincent.zimmer@intel.com> >>> Cc: Zhichao Gao <zhichao.gao@intel.com> >>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>> Cc: Vitaly Cheptsov <vit9696@protonmail.com> >>> Signed-off-by: Michael D Kinney >> <michael.d.kinney@intel.com> >>> --- >>> .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 >> +++++++++++++++++++ >>> 1 file changed, 85 insertions(+) >>> >>> diff --git >> a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c >> b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c >>> index 8952f9da6c..5aced69e0d 100644 >>> --- >> a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c >>> +++ >> b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c >>> @@ -290,6 +290,77 @@ RfcDecodeTest( >>> return UNIT_TEST_PASSED; >>> } >>> >>> +#define SOURCE_STRING L"Hello" >>> + >>> +STATIC >>> +UNIT_TEST_STATUS >>> +EFIAPI >>> +SafeStringContraintCheckTest ( >>> + IN UNIT_TEST_CONTEXT Context >>> + ) >>> +{ >>> + RETURN_STATUS Status; >>> + CHAR16 Destination[20]; >>> + >>> + // >>> + // Positive test case copy source unicode string >> to destination >>> + // >>> + Status = StrCpyS (Destination, sizeof >> (Destination) / sizeof (CHAR16), SOURCE_STRING); >>> + UT_ASSERT_NOT_EFI_ERROR (Status); >>> + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, >> sizeof (SOURCE_STRING)); >>> + >>> + // >>> + // Positive test case with DestMax the same as >> Source size >>> + // >>> + Status = StrCpyS (Destination, sizeof >> (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); >>> + UT_ASSERT_NOT_EFI_ERROR (Status); >>> + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, >> sizeof (SOURCE_STRING)); >>> + >>> + // >>> + // Negative test case with Destination NULL >>> + // >>> + Status = StrCpyS (NULL, sizeof (Destination) / >> sizeof (CHAR16), SOURCE_STRING); >>> + UT_ASSERT_STATUS_EQUAL (Status, >> RETURN_INVALID_PARAMETER); >>> + >>> + // >>> + // Negative test case with Source NULL >>> + // >>> + Status = StrCpyS (Destination, sizeof >> (Destination) / sizeof (CHAR16), NULL); >>> + UT_ASSERT_STATUS_EQUAL (Status, >> RETURN_INVALID_PARAMETER); >>> + >>> + // >>> + // Negative test case with DestMax too big >>> + // >>> + Status = StrCpyS (Destination, MAX_UINTN, >> SOURCE_STRING); >>> + UT_ASSERT_STATUS_EQUAL (Status, >> RETURN_INVALID_PARAMETER); >>> + >>> + // >>> + // Negative test case with DestMax 0 >>> + // >>> + Status = StrCpyS (Destination, 0, SOURCE_STRING); >>> + UT_ASSERT_STATUS_EQUAL (Status, >> RETURN_INVALID_PARAMETER); >>> + >>> + // >>> + // Negative test case with DestMax smaller than >> Source size >>> + // >>> + Status = StrCpyS (Destination, 1, SOURCE_STRING); >>> + UT_ASSERT_STATUS_EQUAL (Status, >> RETURN_BUFFER_TOO_SMALL); >>> + >>> + // >>> + // Negative test case with DestMax smaller than >> Source size by one character >>> + // >>> + Status = StrCpyS (Destination, sizeof >> (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING); >>> + UT_ASSERT_STATUS_EQUAL (Status, >> RETURN_BUFFER_TOO_SMALL); >>> + >>> + // >>> + // Negative test case with DestMax smaller than >> Source size >>> + // >>> + Status = StrCpyS (Destination, sizeof >> (Destination) / sizeof (CHAR16), Destination); >>> + UT_ASSERT_STATUS_EQUAL (Status, >> RETURN_ACCESS_DENIED); >>> + >>> + return UNIT_TEST_PASSED; >>> +} >>> + >>> /** >>> Initialze the unit test framework, suite, and >> unit tests for the >>> Base64 conversion APIs of BaseLib and run the >> unit tests. >>> @@ -309,6 +380,7 @@ UnitTestingEntry ( >>> UNIT_TEST_FRAMEWORK_HANDLE Fw; >>> UNIT_TEST_SUITE_HANDLE b64EncodeTests; >>> UNIT_TEST_SUITE_HANDLE b64DecodeTests; >>> + UNIT_TEST_SUITE_HANDLE SafeStringTests; >>> >>> Fw = NULL; >>> >>> @@ -367,6 +439,19 @@ UnitTestingEntry ( >>> AddTestCase (b64DecodeTests, "Incorrectly placed >> padding character", "Error4", RfcDecodeTest, NULL, >> CleanUpB64TestContext, &mBasicDecodeError4); >>> AddTestCase (b64DecodeTests, "Too small of output >> buffer", "Error5", RfcDecodeTest, NULL, >> CleanUpB64TestContext, &mBasicDecodeError5); >>> >>> + // >>> + // Populate the safe string Unit Test Suite. >>> + // >>> + Status = CreateUnitTestSuite (&SafeStringTests, >> Fw, "Safe String", "BaseLib.SafeString", NULL, NULL); >>> + if (EFI_ERROR (Status)) { >>> + DEBUG ((DEBUG_ERROR, "Failed in >> CreateUnitTestSuite for SafeStringTests\n")); >>> + Status = EFI_OUT_OF_RESOURCES; >>> + goto EXIT; >>> + } >>> + >>> + // --------------Suite-----------Description------ >> --------Class Name----------Function--------Pre---Post- >> ------------------Context----------- >>> + AddTestCase (SafeStringTests, >> "SAFE_STRING_CONSTRAINT_CHECK", >> "SafeStringContraintCheckTest", >> SafeStringContraintCheckTest, NULL, NULL, NULL); >>> + >>> // >>> // Execute the tests. >>> // >>> >> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Patch v8 0/2] Disable safe string constraint assertions @ 2020-05-20 3:11 Michael D Kinney 2020-05-20 3:11 ` [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Michael D Kinney 0 siblings, 1 reply; 13+ messages in thread From: Michael D Kinney @ 2020-05-20 3:11 UTC (permalink / raw) To: devel REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 V8 Add DEBUG_VERBOSE message and unit test V7 addressed review comments (only documentation changes). Current implementation of SafeString does not let one parse untrusted data with its interfaces as they ASSERT on failing runtime checks and require one to effectively reimplement the function on the caller side. All the former proposals made it clear that the attempts to introduce a solution preserving this behavior require a lot of changes throughout the codebase including platform code (e.g. introducing constraint assertions and updating all DebugLib implementations) or require all sorts of hacks. Since the original code has roughly no benefit except in some very special cases and the effort required to preserve it is very high, I propose to remove it as agreed on with several other parties. Please note, that this patch does not change the behaviour of the functions in RELEASE builds. I.e. they will still check for NULL and return RETURN_INVALID_PARAMETER. In the future we may (or may not) want them to simply ASSERT in this case. Regardless this should be done in a separate BZ entry and a separate commit. For this reason I ask everyone not to touch this subject. Due to the amount of discussion this has already undergone after almost a year I kindly request everyone to reduce the communication to the minimum and abstain from proposing another approach. Requesting to merge this into edk2-stable202005. Cc: Andrew Fish afish@apple.com Cc: Ard Biesheuvel ard.biesheuvel@linaro.org Cc: Bret Barkelew bret.barkelew@microsoft.com Cc: Brian J. Johnson brian.johnson@hpe.com Cc: Chasel Chiu chasel.chiu@intel.com Cc: Jordan Justen jordan.l.justen@intel.com Cc: Laszlo Ersek lersek@redhat.com Cc: Leif Lindholm leif@nuviainc.com Cc: Liming Gao liming.gao@intel.com Cc: Marvin Häuser mhaeuser@outlook.de Cc: Michael D Kinney michael.d.kinney@intel.com Cc: Vincent Zimmer vincent.zimmer@intel.com Cc: Zhichao Gao zhichao.gao@intel.com Cc: Jiewen Yao jiewen.yao@intel.com Signed-off-by: Vitaly Cheptsov vit9696@protonmail.com Michael D Kinney (1): MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Vitaly Cheptsov (1): MdePkg: Fix SafeString performing assertions on runtime checks MdePkg/Include/Library/BaseLib.h | 111 ----------------- MdePkg/Library/BaseLib/SafeString.c | 115 +----------------- .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++++++++++++ 3 files changed, 88 insertions(+), 223 deletions(-) -- 2.21.0.windows.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test 2020-05-20 3:11 [Patch v8 0/2] Disable safe string constraint assertions Michael D Kinney @ 2020-05-20 3:11 ` Michael D Kinney 2020-05-20 6:07 ` Vitaly Cheptsov 2020-05-20 17:25 ` Laszlo Ersek 0 siblings, 2 replies; 13+ messages in thread From: Michael D Kinney @ 2020-05-20 3:11 UTC (permalink / raw) To: devel Cc: Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chasel Chiu, Jordan Justen, Laszlo Ersek, Leif Lindholm, Liming Gao, Marvin H?user, Vincent Zimmer, Zhichao Gao, Jiewen Yao, Vitaly Cheptsov Use the safe string function StrCpyS() in BaseLib to test the SAFE_STRING_CONSTRAINT_CHECK() macro. Cc: Andrew Fish <afish@apple.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Bret Barkelew <bret.barkelew@microsoft.com> Cc: Brian J. Johnson <brian.johnson@hpe.com> Cc: Chasel Chiu <chasel.chiu@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Marvin H?user <mhaeuser@outlook.de> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Vincent Zimmer <vincent.zimmer@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Vitaly Cheptsov <vit9696@protonmail.com> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> --- .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c index 8952f9da6c..5aced69e0d 100644 --- a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c +++ b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c @@ -290,6 +290,77 @@ RfcDecodeTest( return UNIT_TEST_PASSED; } +#define SOURCE_STRING L"Hello" + +STATIC +UNIT_TEST_STATUS +EFIAPI +SafeStringContraintCheckTest ( + IN UNIT_TEST_CONTEXT Context + ) +{ + RETURN_STATUS Status; + CHAR16 Destination[20]; + + // + // Positive test case copy source unicode string to destination + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); + + // + // Positive test case with DestMax the same as Source size + // + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); + + // + // Negative test case with Destination NULL + // + Status = StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with Source NULL + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), NULL); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax too big + // + Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax 0 + // + Status = StrCpyS (Destination, 0, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax smaller than Source size + // + Status = StrCpyS (Destination, 1, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); + + // + // Negative test case with DestMax smaller than Source size by one character + // + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); + + // + // Negative test case with DestMax smaller than Source size + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_ACCESS_DENIED); + + return UNIT_TEST_PASSED; +} + /** Initialze the unit test framework, suite, and unit tests for the Base64 conversion APIs of BaseLib and run the unit tests. @@ -309,6 +380,7 @@ UnitTestingEntry ( UNIT_TEST_FRAMEWORK_HANDLE Fw; UNIT_TEST_SUITE_HANDLE b64EncodeTests; UNIT_TEST_SUITE_HANDLE b64DecodeTests; + UNIT_TEST_SUITE_HANDLE SafeStringTests; Fw = NULL; @@ -367,6 +439,19 @@ UnitTestingEntry ( AddTestCase (b64DecodeTests, "Incorrectly placed padding character", "Error4", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError4); AddTestCase (b64DecodeTests, "Too small of output buffer", "Error5", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError5); + // + // Populate the safe string Unit Test Suite. + // + Status = CreateUnitTestSuite (&SafeStringTests, Fw, "Safe String", "BaseLib.SafeString", NULL, NULL); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for SafeStringTests\n")); + Status = EFI_OUT_OF_RESOURCES; + goto EXIT; + } + + // --------------Suite-----------Description--------------Class Name----------Function--------Pre---Post-------------------Context----------- + AddTestCase (SafeStringTests, "SAFE_STRING_CONSTRAINT_CHECK", "SafeStringContraintCheckTest", SafeStringContraintCheckTest, NULL, NULL, NULL); + // // Execute the tests. // -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test 2020-05-20 3:11 ` [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Michael D Kinney @ 2020-05-20 6:07 ` Vitaly Cheptsov 2020-05-20 20:13 ` Michael D Kinney 2020-05-20 17:25 ` Laszlo Ersek 1 sibling, 1 reply; 13+ messages in thread From: Vitaly Cheptsov @ 2020-05-20 6:07 UTC (permalink / raw) To: Michael D Kinney Cc: devel, Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chasel Chiu, Jordan Justen, Laszlo Ersek, Leif Lindholm, Liming Gao, Marvin H?user, Vincent Zimmer, Zhichao Gao, Jiewen Yao [-- Attachment #1.1: Type: text/plain, Size: 7041 bytes --] Mike, This looks generally good to me, but there are few issues. 1. There is a mistake in the description here: + // Negative test case with DestMax smaller than Source size + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination); This should read «Negative test case with Destination and Source matching» probably or something alike. 2. For completeness I would also add zeroing before running every StrCpyS test, like this: + ZeroMem (Destination, sizeof (Destination)); + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); Otherwise UT_ASSERT_MEM_EQUAL will miss the check of StrCpyS doing anything — Destination could remain unchanged since case 1. 3. I will also make sense to check the Destination string remains unchanged on failure for all the other cases, like this: + CHAR16 Destination[20]; + CHAR16 AllZero[20]; + + ZeroMem (AllZero, sizeof (AllZero)); + // + // Negative test case with DestMax too big + // + ZeroMem (Destination, sizeof (Destination)); + Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero)); This behaviour was very unintuitive to me when I met with EDK II BaseLib, because I expected something like strlcpy behaviour, where the string is truncated to fit, so it is definitely worth testing to ensure we do not break things. Best wishes, Vitaly > 20 мая 2020 г., в 06:11, Michael D Kinney <michael.d.kinney@intel.com> написал(а): > > > Use the safe string function StrCpyS() in BaseLib to test the > SAFE_STRING_CONSTRAINT_CHECK() macro. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Bret Barkelew <bret.barkelew@microsoft.com> > Cc: Brian J. Johnson <brian.johnson@hpe.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif@nuviainc.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Marvin H?user <mhaeuser@outlook.de> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Vincent Zimmer <vincent.zimmer@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Vitaly Cheptsov <vit9696@protonmail.com> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> > --- > .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > index 8952f9da6c..5aced69e0d 100644 > --- a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > +++ b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c > @@ -290,6 +290,77 @@ RfcDecodeTest( > return UNIT_TEST_PASSED; > } > > +#define SOURCE_STRING L"Hello" > + > +STATIC > +UNIT_TEST_STATUS > +EFIAPI > +SafeStringContraintCheckTest ( > + IN UNIT_TEST_CONTEXT Context > + ) > +{ > + RETURN_STATUS Status; > + CHAR16 Destination[20]; > + > + // > + // Positive test case copy source unicode string to destination > + // > + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); > + UT_ASSERT_NOT_EFI_ERROR (Status); > + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); > + > + // > + // Positive test case with DestMax the same as Source size > + // > + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); > + UT_ASSERT_NOT_EFI_ERROR (Status); > + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); > + > + // > + // Negative test case with Destination NULL > + // > + Status = StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with Source NULL > + // > + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), NULL); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax too big > + // > + Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax 0 > + // > + Status = StrCpyS (Destination, 0, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax smaller than Source size > + // > + Status = StrCpyS (Destination, 1, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); > + > + // > + // Negative test case with DestMax smaller than Source size by one character > + // > + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); > + > + // > + // Negative test case with DestMax smaller than Source size > + // > + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_ACCESS_DENIED); > + > + return UNIT_TEST_PASSED; > +} > + > /** > Initialze the unit test framework, suite, and unit tests for the > Base64 conversion APIs of BaseLib and run the unit tests. > @@ -309,6 +380,7 @@ UnitTestingEntry ( > UNIT_TEST_FRAMEWORK_HANDLE Fw; > UNIT_TEST_SUITE_HANDLE b64EncodeTests; > UNIT_TEST_SUITE_HANDLE b64DecodeTests; > + UNIT_TEST_SUITE_HANDLE SafeStringTests; > > Fw = NULL; > > @@ -367,6 +439,19 @@ UnitTestingEntry ( > AddTestCase (b64DecodeTests, "Incorrectly placed padding character", "Error4", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError4); > AddTestCase (b64DecodeTests, "Too small of output buffer", "Error5", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError5); > > + // > + // Populate the safe string Unit Test Suite. > + // > + Status = CreateUnitTestSuite (&SafeStringTests, Fw, "Safe String", "BaseLib.SafeString", NULL, NULL); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for SafeStringTests\n")); > + Status = EFI_OUT_OF_RESOURCES; > + goto EXIT; > + } > + > + // --------------Suite-----------Description--------------Class Name----------Function--------Pre---Post-------------------Context----------- > + AddTestCase (SafeStringTests, "SAFE_STRING_CONSTRAINT_CHECK", "SafeStringContraintCheckTest", SafeStringContraintCheckTest, NULL, NULL, NULL); > + > // > // Execute the tests. > // > -- > 2.21.0.windows.1 > [-- Attachment #1.2: Type: text/html, Size: 12428 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test 2020-05-20 6:07 ` Vitaly Cheptsov @ 2020-05-20 20:13 ` Michael D Kinney 0 siblings, 0 replies; 13+ messages in thread From: Michael D Kinney @ 2020-05-20 20:13 UTC (permalink / raw) To: vit9696, Kinney, Michael D Cc: devel@edk2.groups.io, Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chiu, Chasel, Justen, Jordan L, Laszlo Ersek, Leif Lindholm, Gao, Liming, Marvin H?user, Zimmer, Vincent, Gao, Zhichao, Yao, Jiewen [-- Attachment #1: Type: text/plain, Size: 8147 bytes --] Hi Vitaly, I agree with all the comments here. I have made these updates in V9. Mike From: vit9696 <vit9696@protonmail.com> Sent: Tuesday, May 19, 2020 11:08 PM To: Kinney, Michael D <michael.d.kinney@intel.com> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Bret Barkelew <bret.barkelew@microsoft.com>; Brian J . Johnson <brian.johnson@hpe.com>; Chiu, Chasel <chasel.chiu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Gao, Liming <liming.gao@intel.com>; Marvin H?user <mhaeuser@outlook.de>; Zimmer, Vincent <vincent.zimmer@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Subject: Re: [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Mike, This looks generally good to me, but there are few issues. 1. There is a mistake in the description here: + // Negative test case with DestMax smaller than Source size + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination); This should read «Negative test case with Destination and Source matching» probably or something alike. 2. For completeness I would also add zeroing before running every StrCpyS test, like this: + ZeroMem (Destination, sizeof (Destination)); + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); Otherwise UT_ASSERT_MEM_EQUAL will miss the check of StrCpyS doing anything — Destination could remain unchanged since case 1. 3. I will also make sense to check the Destination string remains unchanged on failure for all the other cases, like this: + CHAR16 Destination[20]; + CHAR16 AllZero[20]; + + ZeroMem (AllZero, sizeof (AllZero)); + // + // Negative test case with DestMax too big + // + ZeroMem (Destination, sizeof (Destination)); + Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero)); This behaviour was very unintuitive to me when I met with EDK II BaseLib, because I expected something like strlcpy behaviour, where the string is truncated to fit, so it is definitely worth testing to ensure we do not break things. Best wishes, Vitaly 20 мая 2020 г., в 06:11, Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а): Use the safe string function StrCpyS() in BaseLib to test the SAFE_STRING_CONSTRAINT_CHECK() macro. Cc: Andrew Fish <afish@apple.com<mailto:afish@apple.com>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Cc: Bret Barkelew <bret.barkelew@microsoft.com<mailto:bret.barkelew@microsoft.com>> Cc: Brian J. Johnson <brian.johnson@hpe.com<mailto:brian.johnson@hpe.com>> Cc: Chasel Chiu <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>> Cc: Jordan Justen <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> Cc: Leif Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>> Cc: Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>> Cc: Marvin H?user <mhaeuser@outlook.de<mailto:mhaeuser@outlook.de>> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> Cc: Vincent Zimmer <vincent.zimmer@intel.com<mailto:vincent.zimmer@intel.com>> Cc: Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> Cc: Vitaly Cheptsov <vit9696@protonmail.com<mailto:vit9696@protonmail.com>> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> --- .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c index 8952f9da6c..5aced69e0d 100644 --- a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c +++ b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c @@ -290,6 +290,77 @@ RfcDecodeTest( return UNIT_TEST_PASSED; } +#define SOURCE_STRING L"Hello" + +STATIC +UNIT_TEST_STATUS +EFIAPI +SafeStringContraintCheckTest ( + IN UNIT_TEST_CONTEXT Context + ) +{ + RETURN_STATUS Status; + CHAR16 Destination[20]; + + // + // Positive test case copy source unicode string to destination + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); + + // + // Positive test case with DestMax the same as Source size + // + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_NOT_EFI_ERROR (Status); + UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING)); + + // + // Negative test case with Destination NULL + // + Status = StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with Source NULL + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), NULL); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax too big + // + Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax 0 + // + Status = StrCpyS (Destination, 0, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); + + // + // Negative test case with DestMax smaller than Source size + // + Status = StrCpyS (Destination, 1, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); + + // + // Negative test case with DestMax smaller than Source size by one character + // + Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); + + // + // Negative test case with DestMax smaller than Source size + // + Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination); + UT_ASSERT_STATUS_EQUAL (Status, RETURN_ACCESS_DENIED); + + return UNIT_TEST_PASSED; +} + /** Initialze the unit test framework, suite, and unit tests for the Base64 conversion APIs of BaseLib and run the unit tests. @@ -309,6 +380,7 @@ UnitTestingEntry ( UNIT_TEST_FRAMEWORK_HANDLE Fw; UNIT_TEST_SUITE_HANDLE b64EncodeTests; UNIT_TEST_SUITE_HANDLE b64DecodeTests; + UNIT_TEST_SUITE_HANDLE SafeStringTests; Fw = NULL; @@ -367,6 +439,19 @@ UnitTestingEntry ( AddTestCase (b64DecodeTests, "Incorrectly placed padding character", "Error4", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError4); AddTestCase (b64DecodeTests, "Too small of output buffer", "Error5", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError5); + // + // Populate the safe string Unit Test Suite. + // + Status = CreateUnitTestSuite (&SafeStringTests, Fw, "Safe String", "BaseLib.SafeString", NULL, NULL); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for SafeStringTests\n")); + Status = EFI_OUT_OF_RESOURCES; + goto EXIT; + } + + // --------------Suite-----------Description--------------Class Name----------Function--------Pre---Post-------------------Context----------- + AddTestCase (SafeStringTests, "SAFE_STRING_CONSTRAINT_CHECK", "SafeStringContraintCheckTest", SafeStringContraintCheckTest, NULL, NULL, NULL); + // // Execute the tests. // -- 2.21.0.windows.1 [-- Attachment #2: Type: text/html, Size: 56263 bytes --] ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test 2020-05-20 3:11 ` [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Michael D Kinney 2020-05-20 6:07 ` Vitaly Cheptsov @ 2020-05-20 17:25 ` Laszlo Ersek 1 sibling, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2020-05-20 17:25 UTC (permalink / raw) To: Michael D Kinney, devel Cc: Andrew Fish, Ard Biesheuvel, Bret Barkelew, Brian J . Johnson, Chasel Chiu, Jordan Justen, Leif Lindholm, Liming Gao, Marvin H?user, Vincent Zimmer, Zhichao Gao, Jiewen Yao, Vitaly Cheptsov On 05/20/20 05:11, Michael D Kinney wrote: > Use the safe string function StrCpyS() in BaseLib to test the > SAFE_STRING_CONSTRAINT_CHECK() macro. > > Cc: Andrew Fish <afish@apple.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Bret Barkelew <bret.barkelew@microsoft.com> > Cc: Brian J. Johnson <brian.johnson@hpe.com> > Cc: Chasel Chiu <chasel.chiu@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif@nuviainc.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Marvin H?user <mhaeuser@outlook.de> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Vincent Zimmer <vincent.zimmer@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Vitaly Cheptsov <vit9696@protonmail.com> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com> > --- > .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) I'll defer to other reviewers on this patch. Thanks, Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-05-20 20:16 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-05-20 3:01 [Patch v8 0/2] Disable safe string constraint assertions Michael D Kinney 2020-05-20 3:01 ` [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks Michael D Kinney 2020-05-20 5:56 ` Vitaly Cheptsov 2020-05-20 17:18 ` [EXTERNAL] Re: [edk2-devel] " Bret Barkelew 2020-05-20 14:50 ` Liming Gao 2020-05-20 3:01 ` [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Michael D Kinney 2020-05-20 19:04 ` [edk2-devel] " Sean 2020-05-20 20:11 ` Michael D Kinney 2020-05-20 20:16 ` Sean -- strict thread matches above, loose matches on Subject: below -- 2020-05-20 3:11 [Patch v8 0/2] Disable safe string constraint assertions Michael D Kinney 2020-05-20 3:11 ` [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Michael D Kinney 2020-05-20 6:07 ` Vitaly Cheptsov 2020-05-20 20:13 ` Michael D Kinney 2020-05-20 17:25 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox