Re: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Michael, 

Comments below.

Mike

> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel@edk2.groups.io
> Cc: Erich McMillan <emcmillan@microsoft.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
> 
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  MdePkg/Library/BaseLib/String.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
> index 98e6d31463e0..0ff0454b9d98 100644
> --- a/MdePkg/Library/BaseLib/String.c
> +++ b/MdePkg/Library/BaseLib/String.c
> @@ -6,6 +6,7 @@
> 
>  **/
> 
> +#include <Uefi/UefiBaseType.h>

Why is this change needed?

I think this should be <Base.h> for a library of type BASE
and BaseLibInternals.h includes <Base.h>.  I see the use 
of EFI_ERROR() in changes below.  The BASE lib macro to use
that does not require UEFI types is the RETURN_ERROR() macro.

>  #include "BaseLibInternals.h"
> 
>  /**
> @@ -408,7 +409,8 @@ StrDecimalToUintn (
>  {
>    UINTN  Result;
> 
> -  StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result);
> +  Result = !EFI_ERROR (StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINTN;> +

I think RETURN_ERROR() should be used instead of EFI_ERROR()), and putting
this on a single line makes it hard to understand.  Perhaps the following
style:


  if (RETURN_ERROR (StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result))) {
    return MAX_UINTN;
  }
  return Result;

I would also add more details to the commit message.  The current form would
return an undefined Result value from the stack if StrDecimalToUintnS()
returned an error.  This change now consistently returns MAX_UINTN.  
This may impact the caller of this API.

These comments apply to the similar changes below.

>    return Result;
>  }
> 
> @@ -454,7 +456,8 @@ StrDecimalToUint64 (
>  {
>    UINT64  Result;
> 
> -  StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result);
> +  Result = !EFI_ERROR (StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINT64;
> +
>    return Result;
>  }
> 
> @@ -501,7 +504,8 @@ StrHexToUintn (
>  {
>    UINTN  Result;
> 
> -  StrHexToUintnS (String, (CHAR16 **)NULL, &Result);
> +  Result = !EFI_ERROR (StrHexToUintnS (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINTN;
> +
>    return Result;
>  }
> 
> @@ -548,7 +552,7 @@ StrHexToUint64 (
>  {
>    UINT64  Result;
> 
> -  StrHexToUint64S (String, (CHAR16 **)NULL, &Result);
> +  Result = !EFI_ERROR (StrHexToUint64S (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINT64;
>    return Result;
>  }
> 
> @@ -989,7 +993,7 @@ AsciiStrDecimalToUintn (
>  {
>    UINTN  Result;
> 
> -  AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result);
> +  Result = !EFI_ERROR (AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINTN;
>    return Result;
>  }
> 
> @@ -1031,7 +1035,7 @@ AsciiStrDecimalToUint64 (
>  {
>    UINT64  Result;
> 
> -  AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result);
> +  Result = !EFI_ERROR (AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINT64;
>    return Result;
>  }
> 
> @@ -1077,7 +1081,7 @@ AsciiStrHexToUintn (
>  {
>    UINTN  Result;
> 
> -  AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result);
> +  Result = !EFI_ERROR (AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINTN;
>    return Result;
>  }
> 
> @@ -1123,7 +1127,7 @@ AsciiStrHexToUint64 (
>  {
>    UINT64  Result;
> 
> -  AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result);
> +  Result = !EFI_ERROR (AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINT64;
>    return Result;
>  }
> 
> --
> 2.28.0.windows.1