1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v1 00/12] Enable New CodeQL Queries
  4. View patch

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

Michael Kubacki posted 12 patches 3 years, 1 month ago
There is a newer version of this series
[edk2-devel] [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
Posted by Michael Kubacki 3 years, 1 month ago 
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>
 #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;
+
   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 (#96152): https://edk2.groups.io/g/devel/message/96152
Mute This Topic: https://groups.io/mt/94918094/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
Posted by Michael D Kinney 3 years ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
Posted by Michael Kubacki 3 years ago 
Thanks. Responses inline.

On 11/23/2022 8:53 PM, Kinney, Michael D wrote:
> 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.
> 

I'll double check it.

>>   #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.
> 

I agree the suggested style is easier to read.

I will also add a note about the change in return value behavior.

>>     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 (#96599): https://edk2.groups.io/g/devel/message/96599
Mute This Topic: https://groups.io/mt/94918094/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.