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 написал(а): > > > Use the safe string function StrCpyS() in BaseLib to test the > SAFE_STRING_CONSTRAINT_CHECK() macro. > > Cc: Andrew Fish > Cc: Ard Biesheuvel > Cc: Bret Barkelew > Cc: Brian J. Johnson > Cc: Chasel Chiu > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Leif Lindholm > Cc: Liming Gao > Cc: Marvin H?user > Cc: Michael D Kinney > Cc: Vincent Zimmer > Cc: Zhichao Gao > Cc: Jiewen Yao > Cc: Vitaly Cheptsov > Signed-off-by: Michael D Kinney > --- > .../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 >