[edk2-devel] [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables

[Michael D Kinney]

Hi Michael, 

Comments below.

Mike

> -----Original Message-----
> From: mikuback at linux.microsoft.com <mikuback at linux.microsoft.com>
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel at edk2.groups.io
> Cc: Erich McMillan <emcmillan at microsoft.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney at intel.com>; Michael Kubacki <mikuback at linux.microsoft.com>; Liu, Zhiguang <zhiguang.liu at intel.com>
> Subject: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
> 
> From: Michael Kubacki <michael.kubacki at microsoft.com>
> 
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
> 
> Cc: Erich McMillan <emcmillan at microsoft.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Michael Kubacki <mikuback at linux.microsoft.com>
> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
> Co-authored-by: Erich McMillan <emcmillan at microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki at 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96598): https://edk2.groups.io/g/devel/message/96598
Mute This Topic: https://groups.io/mt/94918094/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-