[PATCH 0/3] MdePkg/BaseLib: Base64Decode: Make it follow its specification

[PATCH 0/3] MdePkg/BaseLib: Base64Decode: Make it follow its specification

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1891

Adjust the coding style.
Set DestinationSize before return.
Add addition decription for the RETURN_SUCCESS case.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Marvin Hauser <mhaeuser@outlook.de>
Cc: Laszlo Ersek <lersek@redhat.com>

Zhichao Gao (3):
  MdePkg/BaseLib: Adjust the coding style in Base64Decode
  MdePkg/BaseLib: Base64Decode: Make DestinationSize complied to spec
  MdePkg/BaseLib: Base64Decode: Add decription for RETURN_SUCCESS

 MdePkg/Library/BaseLib/String.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

-- 
2.21.0.windows.1

[PATCH 1/3] MdePkg/BaseLib: Adjust the coding style in Base64Decode

[Gao, Zhichao]

Adjust the code style for better view.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdePkg/Library/BaseLib/String.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
index 32e189791c..b86e7e9436 100644
--- a/MdePkg/Library/BaseLib/String.c
+++ b/MdePkg/Library/BaseLib/String.c
@@ -1993,8 +1993,7 @@ Base64Decode (
       if (BufferSize < -2) {
         return RETURN_INVALID_PARAMETER;
       }
-    }
-    else {
+    } else {
       Chr = Source[SourceIndex];
       if (BAD_V != DecodingTable[(UINT8) Chr]) {
 
@@ -2006,8 +2005,7 @@ Base64Decode (
           return RETURN_INVALID_PARAMETER;
         }
           ActualSourceLength++;
-      }
-        else {
+      } else {
 
         //
         // The reset of the decoder will ignore all invalid characters allowed here.
@@ -2029,8 +2027,8 @@ Base64Decode (
   }
 
   BufferSize += ActualSourceLength / 4 * 3;
-    if (BufferSize < 0) {
-      return RETURN_INVALID_PARAMETER;
+  if (BufferSize < 0) {
+    return RETURN_INVALID_PARAMETER;
   }
 
   //
@@ -2061,7 +2059,7 @@ Base64Decode (
     //
     for (Index = 0; Index < 4; Index++) {
       do {
-      Chr = DecodingTable[(UINT8) Source[SourceIndex++]];
+        Chr = DecodingTable[(UINT8) Source[SourceIndex++]];
       } while (Chr == BAD_V);
       Value <<= 6;
       Value |= (UINT32)Chr;
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 1/3] MdePkg/BaseLib: Adjust the coding style in Base64Decode

[Laszlo Ersek]

On 06/28/19 05:57, Gao, Zhichao wrote:
> Adjust the code style for better view.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdePkg/Library/BaseLib/String.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
> index 32e189791c..b86e7e9436 100644
> --- a/MdePkg/Library/BaseLib/String.c
> +++ b/MdePkg/Library/BaseLib/String.c
> @@ -1993,8 +1993,7 @@ Base64Decode (
>        if (BufferSize < -2) {
>          return RETURN_INVALID_PARAMETER;
>        }
> -    }
> -    else {
> +    } else {
>        Chr = Source[SourceIndex];
>        if (BAD_V != DecodingTable[(UINT8) Chr]) {
>  
> @@ -2006,8 +2005,7 @@ Base64Decode (
>            return RETURN_INVALID_PARAMETER;
>          }
>            ActualSourceLength++;
> -      }
> -        else {
> +      } else {
>  
>          //
>          // The reset of the decoder will ignore all invalid characters allowed here.
> @@ -2029,8 +2027,8 @@ Base64Decode (
>    }
>  
>    BufferSize += ActualSourceLength / 4 * 3;
> -    if (BufferSize < 0) {
> -      return RETURN_INVALID_PARAMETER;
> +  if (BufferSize < 0) {
> +    return RETURN_INVALID_PARAMETER;
>    }
>  
>    //
> @@ -2061,7 +2059,7 @@ Base64Decode (
>      //
>      for (Index = 0; Index < 4; Index++) {
>        do {
> -      Chr = DecodingTable[(UINT8) Source[SourceIndex++]];
> +        Chr = DecodingTable[(UINT8) Source[SourceIndex++]];
>        } while (Chr == BAD_V);
>        Value <<= 6;
>        Value |= (UINT32)Chr;
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH 1/3] MdePkg/BaseLib: Adjust the coding style in Base64Decode

[Laszlo Ersek]

I have to revise my previous R-b for this patch. The patch is not wrong,
but it's not complete:

On 06/28/19 05:57, Gao, Zhichao wrote:
> Adjust the code style for better view.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdePkg/Library/BaseLib/String.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
> index 32e189791c..b86e7e9436 100644
> --- a/MdePkg/Library/BaseLib/String.c
> +++ b/MdePkg/Library/BaseLib/String.c
> @@ -1993,8 +1993,7 @@ Base64Decode (
>        if (BufferSize < -2) {
>          return RETURN_INVALID_PARAMETER;
>        }
> -    }
> -    else {
> +    } else {
>        Chr = Source[SourceIndex];
>        if (BAD_V != DecodingTable[(UINT8) Chr]) {
>  
> @@ -2006,8 +2005,7 @@ Base64Decode (
>            return RETURN_INVALID_PARAMETER;
>          }
>            ActualSourceLength++;

The line above remains incorrectly indented.

Thanks
Laszlo

> -      }
> -        else {
> +      } else {
>  
>          //
>          // The reset of the decoder will ignore all invalid characters allowed here.
> @@ -2029,8 +2027,8 @@ Base64Decode (
>    }
>  
>    BufferSize += ActualSourceLength / 4 * 3;
> -    if (BufferSize < 0) {
> -      return RETURN_INVALID_PARAMETER;
> +  if (BufferSize < 0) {
> +    return RETURN_INVALID_PARAMETER;
>    }
>  
>    //
> @@ -2061,7 +2059,7 @@ Base64Decode (
>      //
>      for (Index = 0; Index < 4; Index++) {
>        do {
> -      Chr = DecodingTable[(UINT8) Source[SourceIndex++]];
> +        Chr = DecodingTable[(UINT8) Source[SourceIndex++]];
>        } while (Chr == BAD_V);
>        Value <<= 6;
>        Value |= (UINT32)Chr;
> 

Re: [edk2-devel] [PATCH 1/3] MdePkg/BaseLib: Adjust the coding style in Base64Decode

[Laszlo Ersek]

On 06/28/19 05:57, Gao, Zhichao wrote:
> Adjust the code style for better view.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdePkg/Library/BaseLib/String.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

Sorry about sending individual emails about coding style problems. This code is quite rich in surprises. I'm not quoting any part of the patch because the line I'm about to highlight is not part of the context.

We have this line

        if ((Chr != ' ') &&(Chr != '\t') &&(Chr != '\n') &&(Chr != '\r')) {

A space character is missing after each of the && operators.

Thanks
Laszlo

[PATCH 2/3] MdePkg/BaseLib: Base64Decode: Make DestinationSize complied to spec

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1891

DestinationSize is decripted as 'Set to bytes stored on return'.
Before return the status, set its converted bytes to be complied
to the decriptions.
DestinationIndex may be overflow if the *DestinationSize is bigger
than (MAX_ADDRESS - 1). Move its incrementation under condition
'DestinationIndex < *DestinationSize' to make sure it wouldn't be
overflow.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Marvin Hauser <mhaeuser@outlook.de>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdePkg/Library/BaseLib/String.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
index b86e7e9436..7ebc2ecddd 100644
--- a/MdePkg/Library/BaseLib/String.c
+++ b/MdePkg/Library/BaseLib/String.c
@@ -1964,6 +1964,7 @@ Base64Decode (
   // Check if SourceLength or  DestinationSize is valid
   //
   if ((SourceLength >= (MAX_ADDRESS - (UINTN)Source)) || (*DestinationSize >= (MAX_ADDRESS - (UINTN)Destination))){
+    *DestinationSize = 0;
     return RETURN_INVALID_PARAMETER;
   }
 
@@ -1991,6 +1992,7 @@ Base64Decode (
       // Only two '=' characters can be valid.
       //
       if (BufferSize < -2) {
+        *DestinationSize = 0;
         return RETURN_INVALID_PARAMETER;
       }
     } else {
@@ -2002,6 +2004,7 @@ Base64Decode (
         // valid character after an '=', will be flagged as an error.
         //
         if (BufferSize < 0) {
+          *DestinationSize = 0;
           return RETURN_INVALID_PARAMETER;
         }
           ActualSourceLength++;
@@ -2013,6 +2016,7 @@ Base64Decode (
         // ignore ' ', '\t', '\n', and '\r'.
         //
         if ((Chr != ' ') &&(Chr != '\t') &&(Chr != '\n') &&(Chr != '\r')) {
+          *DestinationSize = 0;
           return RETURN_INVALID_PARAMETER;
         }
       }
@@ -2023,11 +2027,13 @@ Base64Decode (
   // The Base64 character string must be a multiple of 4 character quantums.
   //
   if (ActualSourceLength % 4 != 0) {
+    *DestinationSize = 0;
     return RETURN_INVALID_PARAMETER;
   }
 
   BufferSize += ActualSourceLength / 4 * 3;
   if (BufferSize < 0) {
+    *DestinationSize = 0;
     return RETURN_INVALID_PARAMETER;
   }
 
@@ -2075,13 +2081,16 @@ Base64Decode (
     // Due to the '=' special cases for the two bytes at the end,
     // we have to check the length and not store the padding data
     //
-    if (DestinationIndex++ < *DestinationSize) {
+    if (DestinationIndex < *DestinationSize) {
+      DestinationIndex++;
       *Destination++ = (UINT8) (Value >>  8);
     }
-    if (DestinationIndex++ < *DestinationSize) {
+    if (DestinationIndex < *DestinationSize) {
+      DestinationIndex++;
       *Destination++ = (UINT8) Value;
     }
   }
+  *DestinationSize = DestinationIndex;
 
   return RETURN_SUCCESS;
 }
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: Base64Decode: Make DestinationSize complied to spec

[Laszlo Ersek]

On 06/28/19 05:57, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1891
> 
> DestinationSize is decripted as 'Set to bytes stored on return'.
> Before return the status, set its converted bytes to be complied
> to the decriptions.
> DestinationIndex may be overflow if the *DestinationSize is bigger
> than (MAX_ADDRESS - 1). Move its incrementation under condition
> 'DestinationIndex < *DestinationSize' to make sure it wouldn't be
> overflow.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Marvin Hauser <mhaeuser@outlook.de>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdePkg/Library/BaseLib/String.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

I'm going to skip this patch because, as I stated under the blurb, I
believe that the basic approach of the function implementation is wrong.

Thanks
Laszlo

[PATCH 3/3] MdePkg/BaseLib: Base64Decode: Add decription for RETURN_SUCCESS

[Gao, Zhichao]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1891

While the convertion Base64 ascii string is null string (the
string only contain white space would be regard as null string),
there would be no decodeable data. Set *DestinationSize to zero
and return RETURN_SUCCESS. But it is not mention in the comment
of the function. So add this decription.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdePkg/Library/BaseLib/String.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
index 7ebc2ecddd..8829d2cbbf 100644
--- a/MdePkg/Library/BaseLib/String.c
+++ b/MdePkg/Library/BaseLib/String.c
@@ -1929,7 +1929,8 @@ Base64Encode (
   @param DestinationSize   Caller is responsible for passing in buffer of at least DestinationSize.
                            Set 0 to get the size needed. Set to bytes stored on return.
 
-  @retval RETURN_SUCCESS             When binary buffer is filled in.
+  @retval RETURN_SUCCESS             When binary buffer is filled in. Or if the Base64 ascii string is empty, set
+                                     *DestinationSize to zero to indicate this case.
   @retval RETURN_INVALID_PARAMETER   If Source is NULL or DestinationSize is NULL.
   @retval RETURN_INVALID_PARAMETER   If SourceLength or DestinationSize is bigger than (MAX_ADDRESS -(UINTN)Destination ).
   @retval RETURN_INVALID_PARAMETER   If there is any invalid character in input stream.
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 3/3] MdePkg/BaseLib: Base64Decode: Add decription for RETURN_SUCCESS

[Laszlo Ersek]

On 06/28/19 05:57, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1891
> 
> While the convertion Base64 ascii string is null string (the
> string only contain white space would be regard as null string),
> there would be no decodeable data. Set *DestinationSize to zero
> and return RETURN_SUCCESS. But it is not mention in the comment
> of the function. So add this decription.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  MdePkg/Library/BaseLib/String.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
> index 7ebc2ecddd..8829d2cbbf 100644
> --- a/MdePkg/Library/BaseLib/String.c
> +++ b/MdePkg/Library/BaseLib/String.c
> @@ -1929,7 +1929,8 @@ Base64Encode (
>    @param DestinationSize   Caller is responsible for passing in buffer of at least DestinationSize.
>                             Set 0 to get the size needed. Set to bytes stored on return.
>  
> -  @retval RETURN_SUCCESS             When binary buffer is filled in.
> +  @retval RETURN_SUCCESS             When binary buffer is filled in. Or if the Base64 ascii string is empty, set
> +                                     *DestinationSize to zero to indicate this case.
>    @retval RETURN_INVALID_PARAMETER   If Source is NULL or DestinationSize is NULL.
>    @retval RETURN_INVALID_PARAMETER   If SourceLength or DestinationSize is bigger than (MAX_ADDRESS -(UINTN)Destination ).
>    @retval RETURN_INVALID_PARAMETER   If there is any invalid character in input stream.
> 

The new documentation points in the right direction, but it's not
entirely correct. The right language is, "if Source decodes to zero
output characters".

Thanks
Laszlo

Re: [edk2-devel] [PATCH 0/3] MdePkg/BaseLib: Base64Decode: Make it follow its specification

[Laszlo Ersek]

On 06/28/19 05:57, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1891
> 
> Adjust the coding style.
> Set DestinationSize before return.
> Add addition decription for the RETURN_SUCCESS case.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Marvin Hauser <mhaeuser@outlook.de>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Zhichao Gao (3):
>   MdePkg/BaseLib: Adjust the coding style in Base64Decode
>   MdePkg/BaseLib: Base64Decode: Make DestinationSize complied to spec
>   MdePkg/BaseLib: Base64Decode: Add decription for RETURN_SUCCESS
> 
>  MdePkg/Library/BaseLib/String.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 

I'll try to review patches #2 and #3 in this series later.

If you get sufficient MdePkg owner R-b's meanwhile, don't wait for me
with pushing the patches.

Thanks
Laszlo

Re: [edk2-devel] [PATCH 0/3] MdePkg/BaseLib: Base64Decode: Make it follow its specification

[Laszlo Ersek]

On 06/28/19 05:57, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1891
> 
> Adjust the coding style.
> Set DestinationSize before return.
> Add addition decription for the RETURN_SUCCESS case.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Marvin Hauser <mhaeuser@outlook.de>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Zhichao Gao (3):
>   MdePkg/BaseLib: Adjust the coding style in Base64Decode
>   MdePkg/BaseLib: Base64Decode: Make DestinationSize complied to spec
>   MdePkg/BaseLib: Base64Decode: Add decription for RETURN_SUCCESS
> 
>  MdePkg/Library/BaseLib/String.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 

Issues that have not been addressed by this patch set, but should be:

(1) the leading comment says, "Produce Null-terminated binary data in
the output buffer". That's bogus, the binary output is never
NUL-terminated (nor should it be).


(2) One of the RETURN_INVALID_PARAMETER cases is documented as:

"If SourceLength or DestinationSize is bigger than (MAX_ADDRESS
-(UINTN)Destination )."

There are two problems with this.


(2a) SourceLength has nothing to do with Destination. The comment should
be updated -- making sure that (Source + SourceLength) do not overflow
MAX_ADDRESS is worthwhile, but the comment is misleading.


(2b) The code that actually performs the check is off by one. What we
need is that the byte *one past* each buffer still be expressible as a
valid address, so mathematically we need

  (UINTN)Buffer + BufferLength <= MAX_ADDRESS

If you reorder this for a C expression that cannot overflow, you get

  BufferLength <= MAX_ADDRESS - (UINTN)Buffer

If you negate this, to express the failure condition, you get

  BufferLength > MAX_ADDRESS - (UINTN)Buffer

Therefore, the comment for RETURN_INVALID_PARAMETER that says

  DestinationSize is bigger than (MAX_ADDRESS -(UINTN)Destination)

is correct; *however*, the code is wrong:

  *DestinationSize >= (MAX_ADDRESS - (UINTN)Destination)

This failure condition is too lax (IOW, the success condition is too
strict), and shuld be restricted (IOW the success condition should be
relaxed).


(3) Maintaining a signed integer (INTN) BufferSize, and then doing
arithmetic on it with UINTN values, is really bad practice. In
particular, the following expression makes me nervous:

  BufferSize += ActualSourceLength / 4 * 3;

A separate UINTN variable called "EqualSigns" should be introduced,
BufferSize should be made an UINTN, and the logic should be reworked
using those.


(4) "DecodingTable" should be called "mDecodingTable".


(5) The decoding loop checks

  (SourceIndex < SourceLength) && (DestinationIndex < *DestinationSize)

and we have an inner loop

      do {
        Chr = DecodingTable[(UINT8) Source[SourceIndex++]];
      } while (Chr == BAD_V);

Now consider the following case. The caller passes in a valid
(*DestinationSize) that is larger than what is requried for the
decoding. In addition, assume that the input data (Source), which is
otherwise completely valid, is terminated with a space character (or
even with a NUL character, although NUL-termination is not required by
the function's specification).

In the above situation, the innermost loop, which scans for BAD_V, will
fall off the end of Source. The outermost loop condition will evaluate
to TRUE (we have some Source characters left -- namely, one space, or
NUL), and we have room in the Destination buffer too (the caller
specified / allocated a larger DestinationSize thatn what is required).
So we reach the innermost loop, and the space character (BAD_V) will
lead it right off the end of Source.

The outermost loop condition should be changed.

First, the SourceIndex subcondition should be dropped altogether.

Second, *DestinationSize should be set to the actual decoded data size
*before* starting the actual decoding. Then, if we still have output
bytes to produce, in the outermost loop, the Source scanning in the
innermost BAD_V loop is guaranteed to remain in-bounds.


... Honestly, at this point, I sort of wish we just rewrote this
function from zero. The current *approach* of the function is wrong. The
function currently forms a mental image of how the input data "should"
look, and tries to parse that -- it tries to shoehorn the input into the
"expected" format. If the input does not look like the expectation, we
run into gaps here and there.

Instead, the function should follow a state machine approach, where the
outermost loop scans input characters one by one, and makes *absolutely
no assumption* about the character that has just been found. Every UINT8
character in the input should be checked against the full possible UINT8
domain (valid BASE64 range, the equal sign, tolerated whitespace, and
the rest), and acted upon accordingly.

For example, valid BASE64 characters should be accumulated into a 24-bit
value, and flushed when the latter becomes full, and also at the end of
the scanning loop.

Counting vs. decoding can be implemented by making just the flushing
operation conditional (do not write to memory).

Thanks
Laszlo

Re: [edk2-devel] [PATCH 0/3] MdePkg/BaseLib: Base64Decode: Make it follow its specification

[Laszlo Ersek]

On 07/01/19 13:02, Laszlo Ersek wrote:
> On 06/28/19 05:57, Gao, Zhichao wrote:

> (2a) SourceLength has nothing to do with Destination. The comment should
> be updated -- making sure that (Source + SourceLength) do not overflow
> MAX_ADDRESS is worthwhile, but the comment is misleading.

Let me re-state that.

Usually, when you expect the caller to provide an array of bytes,
identified by base address and size, the burden to provide an *actual
array* is on the caller. If the caller does not conform to the function
specification, the behavior is undefined, and it's on the caller.
Therefore, checking MAX_ADDRESS overflows is *generally* dubious, in my
opinion, because a valid array can never overflow MAX_ADDRESS.

If we want to be paranoid about this, I guess we can keep implement
MAX_ADDRESS checks, but then we should both document and implement them
correctly.

Second, it is usually good to specify whether overlap between input and
output is permitted. If we want to be paranoid, we can check that
explicitly again. I don't necessarily suggest that we implement an
overlap check, but we should likely specify in the leading comment that
overlap is not permitted. (This is similar to the "restrict" keyword
from C99.)

Thanks
Laszlo

Re: [edk2-devel] [PATCH 0/3] MdePkg/BaseLib: Base64Decode: Make it follow its specification

[Laszlo Ersek]

On 07/01/19 13:02, Laszlo Ersek wrote:

> ... Honestly, at this point, I sort of wish we just rewrote this
> function from zero. The current *approach* of the function is wrong. The
> function currently forms a mental image of how the input data "should"
> look, and tries to parse that -- it tries to shoehorn the input into the
> "expected" format. If the input does not look like the expectation, we
> run into gaps here and there.
> 
> Instead, the function should follow a state machine approach, where the
> outermost loop scans input characters one by one, and makes *absolutely
> no assumption* about the character that has just been found. Every UINT8
> character in the input should be checked against the full possible UINT8
> domain (valid BASE64 range, the equal sign, tolerated whitespace, and
> the rest), and acted upon accordingly.
> 
> For example, valid BASE64 characters should be accumulated into a 24-bit
> value, and flushed when the latter becomes full, and also at the end of
> the scanning loop.
> 
> Counting vs. decoding can be implemented by making just the flushing
> operation conditional (do not write to memory).

If time allows, I'd like to attempt contributing a version like this.
Please give me a bit of time to work on that.

Thanks
Laszlo