Good day, Please excuse my late reply and thank you very much for your reimplementation effort.
I feel like my rushed message mentioning 'MAX_ADDRESS' was misleading a little - the point with that was a potential index overflow (I may actually have meant 'MAX_UINTN', I am not sure about the details anymore) in the original code, I personally see a lot of sense in *not* checking whether the buffer wraps around (similarly the overlap condition). For one consistency with similar code, where no such checks exist, and then a sanity-trust in the caller (which, as this is a library function, is interior as opposed to an external protocol caller which should naturally be trusted less). Generally, I am rather confused about the edk2 trust model for static calls. A bunch of libraries verify input parameter sanity via ASSERTs, another bunch by runtime checks and appropriate return statuses. Is there any kind of policy I am unaware of? Regards, Marvin > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > Ersek > Sent: Tuesday, July 2, 2019 12:29 PM > To: edk2-devel-groups-io <devel@edk2.groups.io> > Cc: Liming Gao <liming....@intel.com>; Marvin Häuser > <mhaeu...@outlook.de>; Michael D Kinney <michael.d.kin...@intel.com>; > Philippe Mathieu-Daudé <phi...@redhat.com>; Zhichao Gao > <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; > + > + 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; > + } > + > + // > + // If we're in padding mode, accept another padding character, as long as > + // that padding character completes the quantum. This completes case (2) > + // from RFC4648, Chapter 4. "Base 64 Encoding": > + // > + // (2) The final quantum of encoding input is exactly 8 bits; here, the > + // final unit of encoded output will be two characters followed by > two > + // "=" padding characters. > + // > + if (PaddingMode) { > + if (SourceChar == '=' && SixBitGroupsConsumed == 3) { > + SixBitGroupsConsumed = 0; > + continue; > + } > + return RETURN_INVALID_PARAMETER; > + } > + > + // > + // When not in padding mode, decode Base64Value based on RFC4648, > "Table 1: > + // The Base 64 Alphabet". > + // > + if ('A' <= SourceChar && SourceChar <= 'Z') { > + Base64Value = SourceChar - 'A'; > + } else if ('a' <= SourceChar && SourceChar <= 'z') { > + Base64Value = 26 + (SourceChar - 'a'); > + } else if ('0' <= SourceChar && SourceChar <= '9') { > + Base64Value = 52 + (SourceChar - '0'); > + } else if (SourceChar == '+') { > + Base64Value = 62; > + } else if (SourceChar == '/') { > + Base64Value = 63; > + } else if (SourceChar == '=') { > + // > + // Enter padding mode. > + // > + PaddingMode = TRUE; > + > + if (SixBitGroupsConsumed == 2) { > + // > + // If we have consumed two 6-bit groups from the current quantum > before > + // encountering the first padding character, then this is case (2) > from > + // RFC4648, Chapter 4. "Base 64 Encoding". Bump > SixBitGroupsConsumed, > + // and we'll enforce another padding character. > + // > + SixBitGroupsConsumed = 3; > + } else if (SixBitGroupsConsumed == 3) { > + // > + // If we have consumed three 6-bit groups from the current quantum > + // before encountering the first padding character, then this is case > + // (3) from RFC4648, Chapter 4. "Base 64 Encoding". The quantum is > now > + // complete. > + // > + SixBitGroupsConsumed = 0; > + } else { > + // > + // Padding characters are not allowed at the first two positions of a > + // quantum. > + // > + return RETURN_INVALID_PARAMETER; > + } > + > + // > + // Wherever in a quantum we enter padding mode, we enforce the > padding > + // bits pending in the accumulator -- from the last 6-bit group just > + // preceding the padding character -- to be zero. Refer to RFC4648, > + // Chapter 3.5. "Canonical Encoding". > + // > + if (Accumulator != 0) { > + return RETURN_INVALID_PARAMETER; > + } > + > + // > + // Advance to the next source character. > + // > + continue; > + } else { > + // > + // Other characters outside of the encoding alphabet are rejected. > + // > + return RETURN_INVALID_PARAMETER; > + } > + > + // > + // Feed the bits of the current 6-bit group of the quantum to the > + // accumulator. > + // > + Accumulator = (Accumulator << 6) | Base64Value; > + SixBitGroupsConsumed++; > + switch (SixBitGroupsConsumed) { > + case 1: > + // > + // No octet to spill after consuming the first 6-bit group of the > + // quantum; advance to the next source character. > + // > + continue; > + case 2: > + // > + // 12 bits accumulated (6 pending + 6 new); prepare for spilling an > + // octet. 4 bits remain pending. > + // > + DestinationOctet = (UINT8)(Accumulator >> 4); > + Accumulator &= 0xF; > + break; > + case 3: > + // > + // 10 bits accumulated (4 pending + 6 new); prepare for spilling an > + // octet. 2 bits remain pending. > + // > + DestinationOctet = (UINT8)(Accumulator >> 2); > + Accumulator &= 0x3; > + break; > + default: > + ASSERT (SixBitGroupsConsumed == 4); > + // > + // 8 bits accumulated (2 pending + 6 new); prepare for spilling an > octet. > + // The quantum is complete, 0 bits remain pending. > + // > + DestinationOctet = (UINT8)Accumulator; > + Accumulator = 0; > + SixBitGroupsConsumed = 0; > + break; > + } > + > + // > + // Store the decoded octet if there's room left. Increment > + // (*DestinationSize) unconditionally. > + // > + if (*DestinationSize < OriginalDestinationSize) { > + ASSERT (Destination != NULL); > + Destination[*DestinationSize] = DestinationOctet; > + } > + (*DestinationSize)++; > + > + // > + // Advance to the next source character. > + // > + } > + > + // > + // If Source terminates mid-quantum, then Source is invalid. > + // > + if (SixBitGroupsConsumed != 0) { > + return RETURN_INVALID_PARAMETER; > + } > + > + // > + // Done. > + // > + if (*DestinationSize <= OriginalDestinationSize) { > + return RETURN_SUCCESS; > + } > + return RETURN_BUFFER_TOO_SMALL; > } > > /** > -- > 2.19.1.3.g30247aa5d201 > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43738): https://edk2.groups.io/g/devel/message/43738 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] -=-=-=-=-=-=-=-=-=-=-=-
