[edk2-devel] [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test
Philippe Mathieu-Daudé
philmd at redhat.com
Wed May 20 08:37:47 UTC 2020
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 at 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 at intel.com
>> <mailto:michael.d.kinney at intel.com>> написал(а):
>>
>>
>> Use the safe string function StrCpyS() in BaseLib to test the
>> SAFE_STRING_CONSTRAINT_CHECK() macro.
>>
>> Cc: Andrew Fish <afish at apple.com <mailto:afish at apple.com>>
>> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org
>> <mailto:ard.biesheuvel at linaro.org>>
>> Cc: Bret Barkelew <bret.barkelew at microsoft.com
>> <mailto:bret.barkelew at microsoft.com>>
>> Cc: Brian J. Johnson <brian.johnson at hpe.com
>> <mailto:brian.johnson at hpe.com>>
>> Cc: Chasel Chiu <chasel.chiu at intel.com <mailto:chasel.chiu at intel.com>>
>> Cc: Jordan Justen <jordan.l.justen at intel.com
>> <mailto:jordan.l.justen at intel.com>>
>> Cc: Laszlo Ersek <lersek at redhat.com <mailto:lersek at redhat.com>>
>> Cc: Leif Lindholm <leif at nuviainc.com <mailto:leif at nuviainc.com>>
>> Cc: Liming Gao <liming.gao at intel.com <mailto:liming.gao at intel.com>>
>> Cc: Marvin H?user <mhaeuser at outlook.de <mailto:mhaeuser at outlook.de>>
>> Cc: Michael D Kinney <michael.d.kinney at intel.com
>> <mailto:michael.d.kinney at intel.com>>
>> Cc: Vincent Zimmer <vincent.zimmer at intel.com
>> <mailto:vincent.zimmer at intel.com>>
>> Cc: Zhichao Gao <zhichao.gao at intel.com <mailto:zhichao.gao at intel.com>>
>> Cc: Jiewen Yao <jiewen.yao at intel.com <mailto:jiewen.yao at intel.com>>
>> Cc: Vitaly Cheptsov <vit9696 at protonmail.com
>> <mailto:vit9696 at protonmail.com>>
>> Signed-off-by: Michael D Kinney <michael.d.kinney at intel.com
>> <mailto:michael.d.kinney at 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
>>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59943): https://edk2.groups.io/g/devel/message/59943
Mute This Topic: https://groups.io/mt/74342114/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list