Re: [edk2-devel] [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/20/20 8:07 AM, Vitaly Cheptsov via groups.io wrote:
> 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.

Yes, I was also confused there.

With the description updated:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
> 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.

Good idea. Feel free to keep my R-b tag if you include Vitaly's suggestions.

> 
> 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
>>
> 
>