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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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