Re: [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode()

["Laszlo Ersek" <lersek@redhat.com>]

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 <liming.gao@intel.com>
>> Cc: Marvin Häuser <mhaeuser@outlook.de>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Zhichao Gao <zhichao.gao@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1891
>> Ref: http://mid.mail-archive.com/c495bd0b-ea4d-7206-8a4f-a7149760d19a@redhat.com
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  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 <philmd@redhat.com>

Thank you!
Laszlo