public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Gao, Zhichao" <zhichao.gao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"lersek@redhat.com" <lersek@redhat.com>
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>
Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode()
Date: Fri, 12 Jul 2019 02:31:31 +0000	[thread overview]
Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B80750F@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <20190702102836.27589-3-lersek@redhat.com>

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.

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.

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.

Thanks,
Zhichao

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


  reply	other threads:[~2019-07-12  2:31 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-02 10:28 [PATCH 0/3] MdePkg, OvmfPkg: rewrite Base64Decode(), clean up call site Laszlo Ersek
2019-07-02 10:28 ` [PATCH 1/3] MdePkg/BaseLib: re-specify Base64Decode(), and add temporary stub impl Laszlo Ersek
2019-07-16  8:38   ` Philippe Mathieu-Daudé
2019-07-16  9:41     ` Philippe Mathieu-Daudé
2019-07-16 14:14       ` Laszlo Ersek
2019-07-16 14:59         ` Philippe Mathieu-Daudé
2019-07-16 18:53           ` [edk2-devel] " Laszlo Ersek
2019-07-16 10:49   ` Laszlo Ersek
2019-07-16 14:56     ` Liming Gao
2019-07-16 17:15       ` Laszlo Ersek
2019-07-02 10:28 ` [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode() Laszlo Ersek
2019-07-12  2:31   ` Gao, Zhichao [this message]
2019-07-12 19:31     ` [edk2-devel] " Laszlo Ersek
2019-07-15 15:22       ` Liming Gao
2019-07-15 21:56         ` Laszlo Ersek
2019-07-16  1:18           ` Gao, Zhichao
2019-07-16 10:48             ` Laszlo Ersek
2019-07-15 18:44   ` mhaeuser
2019-07-16  0:45     ` Laszlo Ersek
2019-07-16 10:05   ` Philippe Mathieu-Daudé
2019-07-16 14:17     ` Laszlo Ersek
2019-07-02 10:28 ` [PATCH 3/3] OvmfPkg/EnrollDefaultKeys: clean up Base64Decode() retval handling Laszlo Ersek
2019-07-15 21:58   ` [edk2-devel] " Laszlo Ersek
2019-07-16  8:36     ` Philippe Mathieu-Daudé
2019-07-10  9:20 ` [edk2-devel] [PATCH 0/3] MdePkg, OvmfPkg: rewrite Base64Decode(), clean up call site Laszlo Ersek
2019-07-16 22:02 ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3CE959C139B4C44DBEA1810E3AA6F9000B80750F@SHSMSX101.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox