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

["Liming Gao" <liming.gao@intel.com>]

Laszlo:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Saturday, July 13, 2019 3:31 AM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Marvin Häuser <mhaeuser@outlook.de>; Philippe Mathieu-Daudé <philmd@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode()
> 
> 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.

OK to me.

> 
> > 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.

Yes. Source is NULL, SourceSize is zero. Code does nothing. I am fine with it. 

I have no other comments for the code logic. Reviewed-by: Liming Gao <liming.gao@intel.com>

> 
> 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;
> >> +    }
> >> +
> >
> 
> 
>