[edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode()
Liming Gao
liming.gao at intel.com
Mon Jul 15 15:22:19 UTC 2019
Laszlo:
> -----Original Message-----
> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Saturday, July 13, 2019 3:31 AM
> To: Gao, Zhichao <zhichao.gao at intel.com>; devel at edk2.groups.io; Gao, Liming <liming.gao at intel.com>; Kinney, Michael D
> <michael.d.kinney at intel.com>
> Cc: Marvin Häuser <mhaeuser at outlook.de>; Philippe Mathieu-Daudé <philmd at 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 at 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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Tuesday, July 2, 2019 6:29 PM
> >> To: edk2-devel-groups-io <devel at edk2.groups.io>
> >> Cc: Gao, Liming <liming.gao at intel.com>; Marvin Häuser
> >> <mhaeuser at outlook.de>; Kinney, Michael D <michael.d.kinney at intel.com>;
> >> Philippe Mathieu-Daudé <philmd at redhat.com>; Gao, Zhichao
> >> <zhichao.gao at 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 at intel.com>
> >> Cc: Marvin Häuser <mhaeuser at outlook.de>
> >> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> >> Cc: Philippe Mathieu-Daudé <philmd at redhat.com>
> >> Cc: Zhichao Gao <zhichao.gao at intel.com>
> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1891
> >> Ref: http://mid.mail-archive.com/c495bd0b-ea4d-7206-8a4f-
> >> a7149760d19a at redhat.com
> >> Signed-off-by: Laszlo Ersek <lersek at 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;
> >> + }
> >> +
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43727): https://edk2.groups.io/g/devel/message/43727
Mute This Topic: https://groups.io/mt/32284614/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list