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.kin...@intel.com>
> написал(а):
>
>
> Use the safe string function StrCpyS() in BaseLib to test the
> SAFE_STRING_CONSTRAINT_CHECK() macro.
>
> Cc: Andrew Fish <af...@apple.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Bret Barkelew <bret.barke...@microsoft.com>
> Cc: Brian J. Johnson <brian.john...@hpe.com>
> Cc: Chasel Chiu <chasel.c...@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Leif Lindholm <l...@nuviainc.com>
> Cc: Liming Gao <liming....@intel.com>
> Cc: Marvin H?user <mhaeu...@outlook.de>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Vincent Zimmer <vincent.zim...@intel.com>
> Cc: Zhichao Gao <zhichao....@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Vitaly Cheptsov <vit9...@protonmail.com>
> Signed-off-by: Michael D Kinney <michael.d.kin...@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 (#59940): https://edk2.groups.io/g/devel/message/59940
Mute This Topic: https://groups.io/mt/74342114/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: OpenPGP digital signature
