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; Tue, 16 Jul 2019 07:17:12 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2F4FB2BE93; Tue, 16 Jul 2019 14:17:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-187.ams2.redhat.com [10.36.117.187]) by smtp.corp.redhat.com (Postfix) with ESMTP id 443575E7A5; Tue, 16 Jul 2019 14:17:08 +0000 (UTC) Subject: Re: [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode() To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , edk2-devel-groups-io Cc: Liming Gao , =?UTF-8?Q?Marvin_H=c3=a4user?= , Michael D Kinney , Zhichao Gao References: <20190702102836.27589-1-lersek@redhat.com> <20190702102836.27589-3-lersek@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 16 Jul 2019 16:17:07 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 16 Jul 2019 14:17:12 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 07/16/19 12:05, Philippe Mathieu-Daud=C3=A9 wrote: > Hi Laszlo, >=20 > On 7/2/19 12:28 PM, Laszlo Ersek wrote: >> 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. >=20 > Sorry it took me so long, I was reluctant to review this at first, > because reimplementing a piece of code to fix a bug often introduce new > bugs. However your implementation is very clean to follow (well > described) and certainly safer. >=20 >> 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. >=20 > You might want to add a comment in the commit description from your > reply to Marvin regarding keeping MAX_ADDRESS, mostly "The original cod= e > included similar MAX_ADDRESS checks". Good point -- I'll say that the intent is to only strengthen the sanity checks, and hence e.g. the MAX_ADDRESS checks are preserved. >=20 > No more comments :) > Reviewed-by: Philippe Mathieu-Daude Thank you! Laszlo