From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web12.1828.1589231075776483486 for ; Mon, 11 May 2020 14:04:36 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=M9xbo2N6; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589231073; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h+aL9c1xBkIsGYLGrQ4B2a3OP2e3c7HhuRkcoQlPcqU=; b=M9xbo2N6LqeqlLlFAYMO9k4oFlIDHVsNSDU6xc9/tw69hRdgx8CX2kZnepSll1VmefCWDj Sd0XV2p+wZIWAeq3lBougpY7FraF3ixi3EN3RRj/2kcFprMDbM5Gx2gxEAr4Q8Vj3AjXFD AOjXyyOyep1/kqxnq20PXkRGd669yfg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-27-FBCduGg2MKy-5vMlPEN_rg-1; Mon, 11 May 2020 17:04:31 -0400 X-MC-Unique: FBCduGg2MKy-5vMlPEN_rg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id F08968005B7; Mon, 11 May 2020 21:04:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-11.ams2.redhat.com [10.36.113.11]) by smtp.corp.redhat.com (Postfix) with ESMTP id 08E4C6C760; Mon, 11 May 2020 21:04:29 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V4 27/27] MdePkg: Use assertion on constraint violation bit in SafeString To: devel@edk2.groups.io, cheptsov@ispras.ru References: <20200511154121.3878-1-cheptsov@ispras.ru> <20200511154121.3878-28-cheptsov@ispras.ru> From: "Laszlo Ersek" Message-ID: <5c129e3c-4ea6-d7eb-f26c-7d524ffab1df@redhat.com> Date: Mon, 11 May 2020 23:04:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200511154121.3878-28-cheptsov@ispras.ru> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 05/11/20 17:41, Vitaly Cheptsov wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 > > This change allows using SafeString interfaces for untrusted data > checking when constraint violation assertions are disabled. > > Signed-off-by: Vitaly Cheptsov > --- > MdePkg/Include/Library/BaseLib.h | 120 ++++++++++---------- > MdePkg/Library/BaseLib/SafeString.c | 2 +- > 2 files changed, 61 insertions(+), 61 deletions(-) > > diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h > index b0bbe8cef8..9c9f9fe25f 100644 > --- a/MdePkg/Include/Library/BaseLib.h > +++ b/MdePkg/Include/Library/BaseLib.h > @@ -216,7 +216,7 @@ StrnSizeS ( > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -252,7 +252,7 @@ StrCpyS ( > > If Length > 0 and Destination is not aligned on a 16-bit boundary, then ASSERT(). > If Length > 0 and Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -290,7 +290,7 @@ StrnCpyS ( > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -330,7 +330,7 @@ StrCatS ( > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -377,12 +377,12 @@ StrnCatS ( > be ignored. Then, the function stops at the first character that is a not a > valid decimal character or a Null-terminator, whichever one comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Data is NULL, then ASSERT_CONSTRAINT(). > If String is not aligned in a 16-bit boundary, then ASSERT(). > If PcdMaximumUnicodeStringLength is not zero, and String contains more than > PcdMaximumUnicodeStringLength Unicode characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -433,12 +433,12 @@ StrDecimalToUintnS ( > be ignored. Then, the function stops at the first character that is a not a > valid decimal character or a Null-terminator, whichever one comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Data is NULL, then ASSERT_CONSTRAINT(). > If String is not aligned in a 16-bit boundary, then ASSERT(). > If PcdMaximumUnicodeStringLength is not zero, and String contains more than > PcdMaximumUnicodeStringLength Unicode characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -494,12 +494,12 @@ StrDecimalToUint64S ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Data is NULL, then ASSERT_CONSTRAINT(). > If String is not aligned in a 16-bit boundary, then ASSERT(). > If PcdMaximumUnicodeStringLength is not zero, and String contains more than > PcdMaximumUnicodeStringLength Unicode characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -555,12 +555,12 @@ StrHexToUintnS ( > the first character that is a not a valid hexadecimal character or NULL, > whichever one comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Data is NULL, then ASSERT_CONSTRAINT(). > If String is not aligned in a 16-bit boundary, then ASSERT(). > If PcdMaximumUnicodeStringLength is not zero, and String contains more than > PcdMaximumUnicodeStringLength Unicode characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -649,7 +649,7 @@ AsciiStrnSizeS ( > > This function is similar as strcpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -683,7 +683,7 @@ AsciiStrCpyS ( > > This function is similar as strncpy_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -719,7 +719,7 @@ AsciiStrnCpyS ( > > This function is similar as strcat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -757,7 +757,7 @@ AsciiStrCatS ( > > This function is similar as strncat_s defined in C11. > > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -804,11 +804,11 @@ AsciiStrnCatS ( > be ignored. Then, the function stops at the first character that is a not a > valid decimal character or a Null-terminator, whichever one comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Data is NULL, then ASSERT_CONSTRAINT(). > If PcdMaximumAsciiStringLength is not zero, and String contains more than > PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -859,11 +859,11 @@ AsciiStrDecimalToUintnS ( > be ignored. Then, the function stops at the first character that is a not a > valid decimal character or a Null-terminator, whichever one comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Data is NULL, then ASSERT_CONSTRAINT(). > If PcdMaximumAsciiStringLength is not zero, and String contains more than > PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If String has no valid decimal digits in the above format, then 0 is stored > at the location pointed to by Data. > @@ -918,11 +918,11 @@ AsciiStrDecimalToUint64S ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Data is NULL, then ASSERT_CONSTRAINT(). > If PcdMaximumAsciiStringLength is not zero, and String contains more than > PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -977,11 +977,11 @@ AsciiStrHexToUintnS ( > character that is a not a valid hexadecimal character or Null-terminator, > whichever on comes first. > > - If String is NULL, then ASSERT(). > - If Data is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Data is NULL, then ASSERT_CONSTRAINT(). > If PcdMaximumAsciiStringLength is not zero, and String contains more than > PcdMaximumAsciiStringLength Ascii characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If String has no valid hexadecimal digits in the above format, then 0 is > stored at the location pointed to by Data. > @@ -1533,15 +1533,15 @@ StrHexToUint64 ( > "::" can be used to compress one or more groups of X when X contains only 0. > The "::" can only appear once in the String. > > - If String is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > > - If Address is NULL, then ASSERT(). > + If Address is NULL, then ASSERT_CONSTRAINT(). > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > If PcdMaximumUnicodeStringLength is not zero, and String contains more than > PcdMaximumUnicodeStringLength Unicode characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > @@ -1594,15 +1594,15 @@ StrToIpv6Address ( > When /P is in the String, the function stops at the first character that is not > a valid decimal digit character after P is converted. > > - If String is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > > - If Address is NULL, then ASSERT(). > + If Address is NULL, then ASSERT_CONSTRAINT(). > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > If PcdMaximumUnicodeStringLength is not zero, and String contains more than > PcdMaximumUnicodeStringLength Unicode characters, not including the > - Null-terminator, then ASSERT(). > + Null-terminator, then ASSERT_CONSTRAINT(). > > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > @@ -1667,8 +1667,8 @@ StrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - If String is NULL, then ASSERT(). > - If Guid is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Guid is NULL, then ASSERT_CONSTRAINT(). > If String is not aligned in a 16-bit boundary, then ASSERT(). > > @param String Pointer to a Null-terminated Unicode string. > @@ -1703,16 +1703,16 @@ StrToGuid ( > > If String is not aligned in a 16-bit boundary, then ASSERT(). > > - If String is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > > - If Buffer is NULL, then ASSERT(). > + If Buffer is NULL, then ASSERT_CONSTRAINT(). > > - If Length is not multiple of 2, then ASSERT(). > + If Length is not multiple of 2, then ASSERT_CONSTRAINT(). > > If PcdMaximumUnicodeStringLength is not zero and Length is greater than > - PcdMaximumUnicodeStringLength, then ASSERT(). > + PcdMaximumUnicodeStringLength, then ASSERT_CONSTRAINT(). > > - If MaxBufferSize is less than (Length / 2), then ASSERT(). > + If MaxBufferSize is less than (Length / 2), then ASSERT_CONSTRAINT(). > > @param String Pointer to a Null-terminated Unicode string. > @param Length The number of Unicode characters to decode. > @@ -1804,7 +1804,7 @@ UnicodeStrToAsciiStr ( > the upper 8 bits, then ASSERT(). > > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -1851,7 +1851,7 @@ UnicodeStrToAsciiStrS ( > If any Unicode characters in Source contain non-zero value in the upper 8 > bits, then ASSERT(). > If Source is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2415,9 +2415,9 @@ AsciiStrHexToUint64 ( > "::" can be used to compress one or more groups of X when X contains only 0. > The "::" can only appear once in the String. > > - If String is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > > - If Address is NULL, then ASSERT(). > + If Address is NULL, then ASSERT_CONSTRAINT(). > > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > @@ -2470,9 +2470,9 @@ AsciiStrToIpv6Address ( > When /P is in the String, the function stops at the first character that is not > a valid decimal digit character after P is converted. > > - If String is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > > - If Address is NULL, then ASSERT(). > + If Address is NULL, then ASSERT_CONSTRAINT(). > > If EndPointer is not NULL and Address is translated from String, a pointer > to the character that stopped the scan is stored at the location pointed to > @@ -2535,8 +2535,8 @@ AsciiStrToIpv4Address ( > oo Data4[48:55] > pp Data4[56:63] > > - If String is NULL, then ASSERT(). > - If Guid is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > + If Guid is NULL, then ASSERT_CONSTRAINT(). > > @param String Pointer to a Null-terminated ASCII string. > @param Guid Pointer to the converted GUID. > @@ -2568,16 +2568,16 @@ AsciiStrToGuid ( > decoding stops after Length of characters and outputs Buffer containing > (Length / 2) bytes. > > - If String is NULL, then ASSERT(). > + If String is NULL, then ASSERT_CONSTRAINT(). > > - If Buffer is NULL, then ASSERT(). > + If Buffer is NULL, then ASSERT_CONSTRAINT(). > > - If Length is not multiple of 2, then ASSERT(). > + If Length is not multiple of 2, then ASSERT_CONSTRAINT(). > > If PcdMaximumAsciiStringLength is not zero and Length is greater than > - PcdMaximumAsciiStringLength, then ASSERT(). > + PcdMaximumAsciiStringLength, then ASSERT_CONSTRAINT(). > > - If MaxBufferSize is less than (Length / 2), then ASSERT(). > + If MaxBufferSize is less than (Length / 2), then ASSERT_CONSTRAINT(). > > @param String Pointer to a Null-terminated ASCII string. > @param Length The number of ASCII characters to decode. > @@ -2659,7 +2659,7 @@ AsciiStrToUnicodeStr ( > equal or greater than ((AsciiStrLen (Source) + 1) * sizeof (CHAR16)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then the Destination is unmodified. > > @@ -2705,7 +2705,7 @@ AsciiStrToUnicodeStrS ( > ((MIN(AsciiStrLen(Source), Length) + 1) * sizeof (CHAR8)) in bytes. > > If Destination is not aligned on a 16-bit boundary, then ASSERT(). > - If an error would be returned, then the function will also ASSERT(). > + If an error would be returned, then the function will also ASSERT_CONSTRAINT(). > > If an error is returned, then Destination and DestinationLength are > unmodified. > diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c > index 7dc03d2caa..f6cdd76c82 100644 > --- a/MdePkg/Library/BaseLib/SafeString.c > +++ b/MdePkg/Library/BaseLib/SafeString.c > @@ -14,7 +14,7 @@ > > #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status) \ > do { \ > - ASSERT (Expression); \ > + ASSERT_CONSTRAINT (Expression); \ > if (!(Expression)) { \ > return Status; \ > } \ > The amount of function-level comments that need an update is dizzying, so I'm not going to review them individually. The final macro update looks OK. Acked-by: Laszlo Ersek Note: unfortunately, the edk2-platforms tree contains a huge number of DebugLib class resolutions (124 resolutions in 48 files, at commit bfa32ac70a75), and I think the final patch in the present series will break those platforms unless you post patches for them too. IMO this is another good example why edk2-platforms should *not* exist as a separate repository. Thanks, Laszlo