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....@intel.com>; devel@edk2.groups.io; Gao, Liming > <liming....@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: Marvin Häuser <mhaeu...@outlook.de>; Philippe Mathieu-Daudé > <phi...@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....@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....@intel.com>; Marvin Häuser > >> <mhaeu...@outlook.de>; Kinney, Michael D <michael.d.kin...@intel.com>; > >> Philippe Mathieu-Daudé <phi...@redhat.com>; Gao, Zhichao > >> <zhichao....@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....@intel.com> > >> Cc: Marvin Häuser <mhaeu...@outlook.de> > >> Cc: Michael D Kinney <michael.d.kin...@intel.com> > >> Cc: Philippe Mathieu-Daudé <phi...@redhat.com> > >> Cc: Zhichao Gao <zhichao....@intel.com> > >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1891 > >> Ref: http://mid.mail-archive.com/c495bd0b-ea4d-7206-8a4f- > >> a7149760d...@redhat.com > >> Signed-off-by: Laszlo Ersek <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
