From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 12 Jul 2019 12:31:18 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 139DC30832E6; Fri, 12 Jul 2019 19:31:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-65.ams2.redhat.com [10.36.116.65]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1C0CF1001281; Fri, 12 Jul 2019 19:31:13 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode() To: "Gao, Zhichao" , "devel@edk2.groups.io" , "Gao, Liming" , "Kinney, Michael D" Cc: =?UTF-8?Q?Marvin_H=c3=a4user?= , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20190702102836.27589-1-lersek@redhat.com> <20190702102836.27589-3-lersek@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B80750F@SHSMSX101.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <35840ec1-6208-1a6f-b7a2-e63f929118fe@redhat.com> Date: Fri, 12 Jul 2019 21:31:12 +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: <3CE959C139B4C44DBEA1810E3AA6F9000B80750F@SHSMSX101.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Fri, 12 Jul 2019 19:31:18 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 07/12/19 04:31, Gao, Zhichao wrote: > Sorry for late respond. > The whole code is OK for me. And I write a tiny test for it without the= memory address check. See https://github.com/ZhichaoGao/edk2/commit/6153= 56ba32d3147957d215debd844e7709f06849 . It is tested in Emulator environme= nt. If it is OK, I think you can take my Tested-by for this patch. If the= re are some missing, please let me know. Thanks for writing that test app. It seems to have pretty good coverage. I like that it covers the exit points systematically. Mike, Liming: I intend to pick up Zhichao's T-b, from above. If you are aware of another test suite (perhaps used in conjunction with the originally contributed Base64Decode() impl), please let me know. > Base64Decode parameter Source is indicate as OPTIONAL. Although it is O= K to be NULL, and return DestinationSize to be zero with success status t= o indicate no decode occurred . I don't know if it is meaningful to repor= t the result as that. In my opinion, NULL Source is an invalid input. Jus= t my opinion, if the maintainer is OK with that, I am OK too. I think that it is helpful. It is similar to the standard C function free() accepting NULL (it is a no-op). Edk2's FreePool() doesn't accept NULL, and that is inconvenient sometimes. Consider the following use case. There is some code that optionally receives data and then decodes it from base64. By default that data is represented as a NULL pointer, with size 0. If Base64Decode() does not accept NULL input with size 0, then the code in question has to implement a separate check for that. But it keeps the call site simpler if Base64Decode() explicitly specifies "do nothing", for those parameter values. Consider CopyMem() and CompareMem() too. In practice, if you pass Length=3D0, they will work just fine, even in case the buffers are NULL. These functions will exhibit the expected / intuitive behavior -- they will not dereference NULL. However, because their interface contracts don't spell out this case as valid (i.e. OPTIONAL), *technically* such calls are not permitted, and so every CopyMem() and CompareMem() call site has to be surrounded with if's, if NULL buffers with size 0 are otherwise possible there. That's a waste -- it duplicates checks and it makes the call site inconvenient. NULL input with nonzero size is caught and rejected (as described in the function's specification). "OPTIONAL" does not mean that the buffer can be NULL under *any* circumstances -- it means that it can be NULL under *some* (specified) circumstances. >=20 > Some other minor comment about the block scope variable blow. >=20 >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> Laszlo Ersek >> Sent: Tuesday, July 2, 2019 6:29 PM >> To: edk2-devel-groups-io >> Cc: Gao, Liming ; Marvin H=C3=A4user >> ; Kinney, Michael D ; >> Philippe Mathieu-Daud=C3=A9 ; Gao, Zhichao >> >> Subject: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode= () >> >> Rewrite Base64Decode() from scratch, due to reasons listed in the seco= nd >> reference below. >> >> Implement Base64Decode() according to the specification added in the >> previous patch. The decoder scans the input buffer once, it has no inn= er >> loop(s), and it spills each output byte as soon as the output byte is = complete. >> >> Cc: Liming Gao >> Cc: Marvin H=C3=A4user >> Cc: Michael D Kinney >> Cc: Philippe Mathieu-Daud=C3=A9 >> Cc: Zhichao Gao >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1891 >> Ref: http://mid.mail-archive.com/c495bd0b-ea4d-7206-8a4f- >> a7149760d19a@redhat.com >> Signed-off-by: Laszlo Ersek >> --- >> MdePkg/Library/BaseLib/String.c | 249 +++++++++++++++++++- >> 1 file changed, 247 insertions(+), 2 deletions(-) >> >> diff --git a/MdePkg/Library/BaseLib/String.c >> b/MdePkg/Library/BaseLib/String.c index f8397035c32a..6198ccbc9672 >> 100644 >> --- a/MdePkg/Library/BaseLib/String.c >> +++ b/MdePkg/Library/BaseLib/String.c >> @@ -1973,8 +1973,253 @@ Base64Decode ( >> IN OUT UINTN *DestinationSize >> ) >> { >> - ASSERT (FALSE); >> - return RETURN_INVALID_PARAMETER; >> + BOOLEAN PaddingMode; >> + UINTN SixBitGroupsConsumed; >> + UINT32 Accumulator; >> + UINTN OriginalDestinationSize; >> + UINTN SourceIndex; >> + >> + if (DestinationSize =3D=3D NULL) { >> + return RETURN_INVALID_PARAMETER; >> + } >> + >> + // >> + // Check Source array validity. >> + // >> + if (Source =3D=3D NULL) { >> + if (SourceSize > 0) { >> + // >> + // At least one CHAR8 element at NULL Source. >> + // >> + return RETURN_INVALID_PARAMETER; >> + } >> + } else if (SourceSize > MAX_ADDRESS - (UINTN)Source) { >> + // >> + // Non-NULL Source, but it wraps around. >> + // >> + return RETURN_INVALID_PARAMETER; >> + } >> + >> + // >> + // Check Destination array validity. >> + // >> + if (Destination =3D=3D NULL) { >> + if (*DestinationSize > 0) { >> + // >> + // At least one UINT8 element at NULL Destination. >> + // >> + return RETURN_INVALID_PARAMETER; >> + } >> + } else if (*DestinationSize > MAX_ADDRESS - (UINTN)Destination) { >> + // >> + // Non-NULL Destination, but it wraps around. >> + // >> + return RETURN_INVALID_PARAMETER; >> + } >> + >> + // >> + // Check for overlap. >> + // >> + if (Source !=3D NULL && Destination !=3D NULL) { >> + // >> + // Both arrays have been provided, and we know from earlier that = each >> array >> + // is valid in itself. >> + // >> + if ((UINTN)Source + SourceSize <=3D (UINTN)Destination) { >> + // >> + // Source array precedes Destination array, OK. >> + // >> + } else if ((UINTN)Destination + *DestinationSize <=3D (UINTN)Sour= ce) { >> + // >> + // Destination array precedes Source array, OK. >> + // >> + } else { >> + // >> + // Overlap. >> + // >> + return RETURN_INVALID_PARAMETER; >> + } >> + } >> + >> + // >> + // Decoding loop setup. >> + // >> + PaddingMode =3D FALSE; >> + SixBitGroupsConsumed =3D 0; >> + Accumulator =3D 0; >> + OriginalDestinationSize =3D *DestinationSize; >> + *DestinationSize =3D 0; >> + >> + // >> + // Decoding loop. >> + // >> + for (SourceIndex =3D 0; SourceIndex < SourceSize; SourceIndex++) { >> + CHAR8 SourceChar; >> + UINT32 Base64Value; >> + UINT8 DestinationOctet; >=20 > The local variable define in a block scope. For CSS_2_1 Section 5.4.1.1= "Block (local) Scope". It is strongly discouraged. Maybe we should move = them to the beginning of the function. Yes, I can do that. Thanks for pointing out the exact location in the CCS. Before I post v2 though, I'd like to hear back from the MdePkg maintainers as well. Thank you! Laszlo >> + >> + SourceChar =3D Source[SourceIndex]; >> + >> + // >> + // Whitespace is ignored at all positions (regardless of padding = mode). >> + // >> + if (SourceChar =3D=3D '\t' || SourceChar =3D=3D '\n' || SourceCha= r =3D=3D '\v' || >> + SourceChar =3D=3D '\f' || SourceChar =3D=3D '\r' || SourceCha= r =3D=3D ' ') { >> + continue; >> + } >> + >=20