From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.8844.1589963876348533251 for ; Wed, 20 May 2020 01:37:56 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=evpE5T1k; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589963875; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2+2bYYKQ5zFeJmOPIAc6IlhVNgG5KJnhzfoCw4+zucc=; b=evpE5T1k2gCa1ox3jsvyi0itZ7gys4DaBHQBJVe9rmFtxqqGwTvgV8f0xml5swVZiqQZZe VsISl9dd/4cYgXmvMI0TOAKdE1B1cibn1srfeVmXfIybvHtMOCze/npwfTEZc2BZBuR7/J 7CZ8RHSU96F4E/SIcuEOtFiWR2bwT1c= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-282-1D2GJFSVPSqAVeO5DBtItQ-1; Wed, 20 May 2020 04:37:50 -0400 X-MC-Unique: 1D2GJFSVPSqAVeO5DBtItQ-1 Received: by mail-wr1-f70.google.com with SMTP id 30so1088146wrq.15 for ; Wed, 20 May 2020 01:37:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=2+2bYYKQ5zFeJmOPIAc6IlhVNgG5KJnhzfoCw4+zucc=; b=eNCK3x8gy2eUyor+iVTPLt9I5I2faMcwTYnR02FFkrQSAVzuc19w/ZoDXyBiq7yx3L CMufq7iBjSFFdpMrHXNnvAufrdLe+uoYIolm0D89wOILVvIIHZurIZK9xRUe6Abf4PTm 2ZPU7BfSavS4gKMGrNfjaDN7Sbp3Qv+M/EVmYSIYO7QPvKrhkZw8h14JPZB3LUidosFN H9vErcb654K3M/hoUGvo+oAejBiubVnmwqEat84rduI4a2sluv8L3lgeEObB9O+5L/AP jQPhyez0umyjfC7qYJiquVDubtzCRQfp3UgGYvQQ1kVDZe3TpRkBhpJSw9dTF4m8nrsI uNpQ== X-Gm-Message-State: AOAM531lfxDOLPEcnO5ue21OPKARPGbiWgUkszIpb0U6uMLMc9qUt6tI EG16RTVu3OE/qlOWAfe90d9BALDBcbwbs69gk3qbVt+EoMHNl3kMmJFRXlVEghPRcxz9NMCItQi 8WAvUJG6s5Gn60A== X-Received: by 2002:a5d:6283:: with SMTP id k3mr3106433wru.62.1589963868825; Wed, 20 May 2020 01:37:48 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw2vpdrPvAVpqXfLQ2vw0UjDWRkATFsHo4BlubVtSa+r6IP5pCDAm2NWisGX2jGLail8fxN6w== X-Received: by 2002:a5d:6283:: with SMTP id k3mr3106401wru.62.1589963868570; Wed, 20 May 2020 01:37:48 -0700 (PDT) Return-Path: Received: from [192.168.1.38] (17.red-88-21-202.staticip.rima-tde.net. [88.21.202.17]) by smtp.gmail.com with ESMTPSA id p7sm2259186wmg.38.2020.05.20.01.37.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 20 May 2020 01:37:48 -0700 (PDT) Subject: Re: [edk2-devel] [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test To: devel@edk2.groups.io, vit9696@protonmail.com, Michael D Kinney Cc: 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 References: <20200520031108.9644-1-michael.d.kinney@intel.com> <20200520031108.9644-3-michael.d.kinney@intel.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: Date: Wed, 20 May 2020 10:37:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > 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 > > написал(а): >> >> >> Use the safe string function StrCpyS() in BaseLib to test the >> SAFE_STRING_CONSTRAINT_CHECK() macro. >> >> 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(+) >> >> 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 >> > >