On 07/16/19 12:05, Philippe Mathieu-Daudé wrote: > Hi Laszlo, > > On 7/2/19 12:28 PM, Laszlo Ersek wrote: >> Rewrite Base64Decode() from scratch, due to reasons listed in the second >> reference below. >> >> Implement Base64Decode() according to the specification added in the >> previous patch. The decoder scans the input buffer once, it has no inner >> loop(s), and it spills each output byte as soon as the output byte is >> complete. > > 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. > >> Cc: Liming Gao <[email protected]> >> Cc: Marvin Häuser <[email protected]> >> Cc: Michael D Kinney <[email protected]> >> Cc: Philippe Mathieu-Daudé <[email protected]> >> Cc: Zhichao Gao <[email protected]> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1891 >> Ref: >> [email protected]">http://mid.mail-archive.com/[email protected] >> Signed-off-by: Laszlo Ersek <[email protected]> >> --- >> 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 == NULL) { >> + return RETURN_INVALID_PARAMETER; >> + } >> + >> + // >> + // Check Source array validity. >> + // >> + if (Source == 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. > > You might want to add a comment in the commit description from your > reply to Marvin regarding keeping MAX_ADDRESS, mostly "The original code > 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. > > No more comments :) > Reviewed-by: Philippe Mathieu-Daude <[email protected]> Thank you! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43790): https://edk2.groups.io/g/devel/message/43790 Mute This Topic: https://groups.io/mt/32284614/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
