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

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

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/615356ba32d3147957d215debd844e7709f06849 . It is tested in Emulator environment. If it is OK, I think you can take my Tested-by for this patch. If there 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 OK to be NULL, and return DestinationSize to be zero with success status to indicate no decode occurred . I don't know if it is meaningful to report the result as that. In my opinion, NULL Source is an invalid input. Just 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=0, 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.

> 
> Some other minor comment about the block scope variable blow.
> 
>> -----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 <devel@edk2.groups.io>
>> Cc: Gao, Liming <liming.gao@intel.com>; Marvin Häuser
>> <mhaeuser@outlook.de>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> Philippe Mathieu-Daudé <philmd@redhat.com>; Gao, Zhichao
>> <zhichao.gao@intel.com>
>> Subject: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode()
>>
>> 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.
>>
>> 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.
>> +    //
>> +    return RETURN_INVALID_PARAMETER;
>> +  }
>> +
>> +  //
>> +  // Check Destination array validity.
>> +  //
>> +  if (Destination == 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 != NULL && Destination != NULL) {
>> +    //
>> +    // Both arrays have been provided, and we know from earlier that each
>> array
>> +    // is valid in itself.
>> +    //
>> +    if ((UINTN)Source + SourceSize <= (UINTN)Destination) {
>> +      //
>> +      // Source array precedes Destination array, OK.
>> +      //
>> +    } else if ((UINTN)Destination + *DestinationSize <= (UINTN)Source) {
>> +      //
>> +      // Destination array precedes Source array, OK.
>> +      //
>> +    } else {
>> +      //
>> +      // Overlap.
>> +      //
>> +      return RETURN_INVALID_PARAMETER;
>> +    }
>> +  }
>> +
>> +  //
>> +  // Decoding loop setup.
>> +  //
>> +  PaddingMode             = FALSE;
>> +  SixBitGroupsConsumed    = 0;
>> +  Accumulator             = 0;
>> +  OriginalDestinationSize = *DestinationSize;
>> +  *DestinationSize        = 0;
>> +
>> +  //
>> +  // Decoding loop.
>> +  //
>> +  for (SourceIndex = 0; SourceIndex < SourceSize; SourceIndex++) {
>> +    CHAR8  SourceChar;
>> +    UINT32 Base64Value;
>> +    UINT8  DestinationOctet;
> 
> 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 = Source[SourceIndex];
>> +
>> +    //
>> +    // Whitespace is ignored at all positions (regardless of padding mode).
>> +    //
>> +    if (SourceChar == '\t' || SourceChar == '\n' || SourceChar == '\v' ||
>> +        SourceChar == '\f' || SourceChar == '\r' || SourceChar == ' ') {
>> +      continue;
>> +    }
>> +
>