From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-40134.protonmail.ch (mail-40134.protonmail.ch [185.70.40.134]) by mx.groups.io with SMTP id smtpd.web11.7557.1589954875492627077 for ; Tue, 19 May 2020 23:07:56 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@protonmail.com header.s=protonmail header.b=lyTB3XJN; spf=pass (domain: protonmail.com, ip: 185.70.40.134, mailfrom: vit9696@protonmail.com) Date: Wed, 20 May 2020 06:07:48 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail; t=1589954873; bh=bnPB6vN++e+MxPXgLPtS6S6e/hibNlxZV+3xtSWEFyI=; h=Date:To:From:Cc:Reply-To:Subject:In-Reply-To:References:From; b=lyTB3XJN+V7ip7zla4lBNagKhAmh3TAYxjtGjrh0SyRcc497FirJ1r7RK7HforYPk tXVI3Nv+XHKrER8VtQqimeN7NDwc1ON5tMWLSW+RvmE3ex1/Fuv3cVsxehOD4/tIsI t9m6smJ2dJBhYzXrlllpRt1GVFVhHEz3j8kKA0pM= To: Michael D Kinney From: "Vitaly Cheptsov" Cc: devel@edk2.groups.io, Andrew Fish , Ard Biesheuvel , Bret Barkelew , "Brian J . Johnson" , Chasel Chiu , Jordan Justen , Laszlo Ersek , Leif Lindholm , Liming Gao , Marvin H?user , Vincent Zimmer , Zhichao Gao , Jiewen Yao Reply-To: vit9696 Subject: Re: [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test Message-ID: In-Reply-To: <20200520031108.9644-3-michael.d.kinney@intel.com> References: <20200520031108.9644-1-michael.d.kinney@intel.com> <20200520031108.9644-3-michael.d.kinney@intel.com> MIME-Version: 1.0 X-Spam-Status: No, score=-0.7 required=7.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,FREEMAIL_REPLYTO_END_DIGIT,HTML_FONT_LOW_CONTRAST, HTML_MESSAGE shortcircuit=no autolearn=disabled version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on mail.protonmail.ch X-Groupsio-MsgNum: 59940 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha256; boundary="---------------------64c42bfb4029bea4bc2357990b900ec2"; charset=UTF-8 -----------------------64c42bfb4029bea4bc2357990b900ec2 Cc: devel@edk2.groups.io, Andrew Fish , Ard Biesheuvel , Bret Barkelew , "Brian J . Johnson" , Chasel Chiu , Jordan Justen , Laszlo Ersek , Leif Lindholm , Liming Gao , Marvin H?user , Vincent Zimmer , Zhichao Gao , Jiewen Yao Content-Type: multipart/alternative; boundary="Apple-Mail=_BD6849C7-6DA0-45A3-A096-C198A5C0D5B1" Date: Wed, 20 May 2020 09:07:40 +0300 From: vit9696 In-Reply-To: <20200520031108.9644-3-michael.d.kinney@intel.com> Message-Id: Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) References: <20200520031108.9644-1-michael.d.kinney@intel.com> <41cnbbpFDiTFAjgZDS9GRN_zhcKpq1O1o3i6LrFJbHcAqG-ULURxvejEYcwhr6dauRHbla22izSq6e4j0PsEcw==@protonmail.internalid> <3BYWLp-i1xg6Wui96l0hv4VacCsG3FekD2i7uXhuaHbKm6CsOnV4l2o4_7YFXdCOvEbgtbfhLYZfd1i8Lc1mZA==@protonmail.conversationid> <20200520031108.9644-3-michael.d.kinney@intel.com> Subject: Re: [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test To: Michael D Kinney X-Mailer: Apple Mail (2.3608.80.23.2.2) --Apple-Mail=_BD6849C7-6DA0-45A3-A096-C198A5C0D5B1 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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 =3D StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16),= Destination); This should read =C2=ABNegative test case with Destination and Source match= ing=C2=BB probably or something alike. 2. For completeness I would also add zeroing before running every StrCpyS t= est, like this: + ZeroMem (Destination, sizeof (Destination)); + Status =3D 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= =E2=80=94 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 =3D 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, b= ecause I expected something like strlcpy behaviour, where the string is tru= ncated to fit, so it is definitely worth testing to ensure we do not break = things. Best wishes, Vitaly > 20 =D0=BC=D0=B0=D1=8F 2020 =D0=B3., =D0=B2 06:11, Michael D Kinney =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 >=20 > Use the safe string function StrCpyS() in BaseLib to test the > SAFE_STRING_CONSTRAINT_CHECK() macro. >=20 > 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(+) >=20 > diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c b/MdeP= kg/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; > } >=20 > +#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 =3D 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 =3D StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR= 16), 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 =3D StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOUR= CE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with Source NULL > + // > + Status =3D StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16= ), NULL); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax too big > + // > + Status =3D StrCpyS (Destination, MAX_UINTN, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax 0 > + // > + Status =3D StrCpyS (Destination, 0, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER); > + > + // > + // Negative test case with DestMax smaller than Source size > + // > + Status =3D 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 cha= racter > + // > + Status =3D StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR= 16) - 1, SOURCE_STRING); > + UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL); > + > + // > + // Negative test case with DestMax smaller than Source size > + // > + Status =3D 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; >=20 > Fw =3D NULL; >=20 > @@ -367,6 +439,19 @@ UnitTestingEntry ( > AddTestCase (b64DecodeTests, "Incorrectly placed padding character", "E= rror4", RfcDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError4); > AddTestCase (b64DecodeTests, "Too small of output buffer", "Error5", Rf= cDecodeTest, NULL, CleanUpB64TestContext, &mBasicDecodeError5); >=20 > + // > + // Populate the safe string Unit Test Suite. > + // > + Status =3D CreateUnitTestSuite (&SafeStringTests, Fw, "Safe String", "= BaseLib.SafeString", NULL, NULL); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSuite for SafeStringTe= sts\n")); > + Status =3D EFI_OUT_OF_RESOURCES; > + goto EXIT; > + } > + > + // --------------Suite-----------Description--------------Class Name--= --------Function--------Pre---Post-------------------Context----------- > + AddTestCase (SafeStringTests, "SAFE_STRING_CONSTRAINT_CHECK", "SafeStr= ingContraintCheckTest", SafeStringContraintCheckTest, NULL, NULL, NULL); > + > // > // Execute the tests. > // > -- > 2.21.0.windows.1 >=20 --Apple-Mail=_BD6849C7-6DA0-45A3-A096-C198A5C0D5B1 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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 =3D StrCpyS (Destination, sizeof (Destinatio= n) / sizeof (CHAR16), Destination);

This should read =C2=ABNegative test case with Destinati= on and Source matching=C2=BB probably or something alike.

2. For completeness I would also= add zeroing before running every StrCpyS test, like this:

+  Zero= Mem (Destination, sizeof (Destination));
+ &nbs= p;Status =3D StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16)= , SOURCE_STRING);
+  UT_ASSERT_NOT_EFI_ERROR (Sta= tus);
+  UT_ASSERT_MEM_EQUAL (Destination, SOURCE= _STRING, sizeof (SOURCE_STRING));

Otherwise UT_ASSERT_MEM_EQUAL will miss the che= ck of StrCpyS doing anything =E2=80=94 Desti= nation could remain unchanged since case 1.

3. I will also make sense= to check the Destination string remains unchanged on failure for all the o= ther cases, like this:

+  CHAR16       &nb= sp; Destination[20];
+  CHAR16     &nbs= p;   AllZero[20];
+
+  = ZeroMem (AllZero, sizeof (AllZero));

+  //
+  // Negative test case = with DestMax too big
+  //
+  ZeroMem (Destination, sizeof (Destination));
+  Status =3D StrCpyS (Destination, MAX_UINTN, SOURCE_STRING)= ;
+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeo= f (AllZero));

=
This behaviour was very unintuitive to me whe= n 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 =D0=BC=D0=B0=D1=8F 2020 =D0=B3., =D0=B2 06:11, Michael= D Kinney <mich= ael.d.kinney@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(= =D0=B0):


Use the safe string function StrCpyS() in BaseLib= to test the
SAFE_STRING_CONSTRAINT_CHECK() macro.

Cc: Andrew Fish <afish@apple.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro= .org>
Cc: Bret Barkelew <bret.barkelew@microsoft.com>
Cc: Brian J. Johnson <brian.johnson@hpe.com>
Cc: Chasel Chiu <chasel.chiu@intel.com= >
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: L= aszlo Ersek <lersek@redh= at.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Liming = Gao <liming.gao@intel= .com>
Cc: Marvin H?user <mhaeuser@outlook.de>
Cc: Micha= el D Kinney <mi= chael.d.kinney@intel.com>
Cc: Vincent Zimmer <vincent.zimmer@intel.com<= /a>>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Jiewen Y= ao <jiewen.yao@intel.= com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Si= gned-off-by: Michael D Kinney <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/Libr= ary/BaseLib/Base64UnitTest.c
+++ b/MdePkg/Test/UnitTest/Libra= ry/BaseLib/Base64UnitTest.c
@@ -290,6 +290,77 @@ RfcDecodeTes= t(
  return UNIT_TEST_PASSED;
}

+#define SOURCE_STRING  L"Hello"
+
+STATIC
+UNIT_TEST_STATUS
+E= FIAPI
+SafeStringContraintCheckTest (
+  I= N UNIT_TEST_CONTEXT  Context
+  )
+{<= br class=3D"">+  RETURN_STATUS  Status;
+  CHA= R16         Destination[20];
+
+  //
+  // Positive test c= ase copy source unicode string to destination
+  //
+  Status =3D StrCpyS (Destination, sizeof (Destination) / s= izeof (CHAR16), SOURCE_STRING);
+  UT_ASSERT_NOT_EFI_ERR= OR (Status);
+  UT_ASSERT_MEM_EQUAL (Destination, SOURCE= _STRING, sizeof (SOURCE_STRING));
+
+  //<= br class=3D"">+  // Positive test case with DestMax the same as Source= size
+  //
+  Status =3D StrCpyS (De= stination, 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 =3D StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOURCE_ST= RING);
+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID= _PARAMETER);
+
+  //
+  = ;// Negative test case with Source NULL
+  //
+  Status =3D StrCpyS (Destination, sizeof (Destination) / sizeo= f (CHAR16), NULL);
+  UT_ASSERT_STATUS_EQUAL (Status, RE= TURN_INVALID_PARAMETER);
+
+  //
+  // Negative test case with DestMax too big
+ &n= bsp;//
+  Status =3D StrCpyS (Destination, MAX_UINTN, SO= URCE_STRING);
+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_= INVALID_PARAMETER);
+
+  //
= +  // Negative test case with DestMax 0
+  //
+  Status =3D StrCpyS (Destination, 0, SOURCE_STRING);
+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);<= br class=3D"">+
+  //
+  // Negative = test case with DestMax smaller than Source size
+  //+  Status =3D StrCpyS (Destination, 1, SOURCE_STRING);
+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL)= ;
+
+  //
+  // Negativ= e test case with DestMax smaller than Source size by one character
+  //
+  Status =3D StrCpyS (Destination, siz= eof (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING);
+ =  UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
+
+  //
+  // Negative test case w= ith DestMax smaller than Source size
+  //
+  Status =3D StrCpyS (Destination, sizeof (Destination) / sizeof (CH= AR16), Destination);
+  UT_ASSERT_STATUS_EQUAL (Status, = RETURN_ACCESS_DENIED);
+
+  return UNIT_TE= ST_PASSED;
+}
+
/**
  Initialze the unit test framework, suite, and unit tests for= the
  Base64 conversion APIs of BaseLib and run t= he unit tests.
@@ -309,6 +380,7 @@ UnitTestingEntry (
  UNIT_TEST_FRAMEWORK_HANDLE  Fw;
&nb= sp; UNIT_TEST_SUITE_HANDLE      b64EncodeTest= s;
  UNIT_TEST_SUITE_HANDLE    &nbs= p; b64DecodeTests;
+  UNIT_TEST_SUITE_HANDLE  =     SafeStringTests;

&nbs= p; Fw =3D NULL;

@@ -367,6 +439,19 @@ Unit= TestingEntry (
  AddTestCase (b64DecodeTests, "Inc= orrectly placed padding character", "Error4", RfcDecodeTest, NULL, CleanUpB= 64TestContext, &mBasicDecodeError4);
  AddTest= Case (b64DecodeTests, "Too small of output buffer", "Error5", RfcDecodeTest= , NULL, CleanUpB64TestContext, &mBasicDecodeError5);

+  //
+  // Populate the safe string Uni= t Test Suite.
+  //
+  Status =3D Cre= ateUnitTestSuite (&SafeStringTests, Fw, "Safe String", "BaseLib.SafeStr= ing", NULL, NULL);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestSui= te for SafeStringTests\n"));
+    Status =3D E= FI_OUT_OF_RESOURCES;
+    goto EXIT;
+  }
+
+  // --------------Suit= e-----------Description--------------Class Name----------Function--------Pr= e---Post-------------------Context-----------
+  AddTest= Case (SafeStringTests, "SAFE_STRING_CONSTRAINT_CHECK", "SafeStringContraint= CheckTest", SafeStringContraintCheckTest, NULL, NULL, NULL);
= +
  //
  // Execute the t= ests.
  //
--
2.21.0.w= indows.1


--Apple-Mail=_BD6849C7-6DA0-45A3-A096-C198A5C0D5B1-- -----------------------64c42bfb4029bea4bc2357990b900ec2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: ProtonMail wsBmBAEBCAAQBQJexMkuCRBPsoxt7Hy0xQAKCRBPsoxt7Hy0xUhCB/wLqRdG Sn+PM494svQvn/7jsFfkDJKKCjZL9Asm0YUqc66hOOpaoiIeVPoBZs6r4oU8 di3BYaTcKSbAsyEAzJ9uwQZhDNO7dsnbMg5LXb4M0eho+cWSVOFkcvI/zk29 M+KNC9ZJXKn4rIalL2d+3/+ORKvK4qQjVI6ydVE1bfka3tFcZefomJP2YnPj +aQliKG1NLzP5mC1PeVubY57vJHJGfN9PKXkeTKWDg5SAEofoS6dY4LM+7X3 q/yn/uqqsrc4RCVzdCKtqXZrsbLw1C8kuTWHlfpFH+hpyCafZl9S0P8z9wlQ nteRXq6ro0WxedNNFJilKSwn59MxYYLrb+5z =lqxG -----END PGP SIGNATURE----- -----------------------64c42bfb4029bea4bc2357990b900ec2--