Thanks. Responses inline.

On 11/23/2022 8:53 PM, Kinney, Michael D wrote:

Hi Michael,
Comments below.
Mike

-----Original Message-----
From: mikuback@... <mikuback@...>
Sent: Wednesday, November 9, 2022 9:33 AM
To: devel@edk2.groups.io
Cc: Erich McMillan <emcmillan@...>; Gao, Liming <gaoliming@...>; Kinney, Michael D
<michael.d.kinney@...>; Michael Kubacki <mikuback@...>; Liu, Zhiguang <zhiguang.liu@...>
Subject: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables

From: Michael Kubacki <michael.kubacki@...>

Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html

Cc: Erich McMillan <emcmillan@...>
Cc: Liming Gao <gaoliming@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Co-authored-by: Erich McMillan <emcmillan@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>
---
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

Hi Michael,

Comments below.

Mike

From: Michael Kubacki <michael.kubacki@...>

Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html

Cc: Erich McMillan <emcmillan@...>
Cc: Liming Gao <gaoliming@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Co-authored-by: Erich McMillan <emcmillan@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>
---
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/Str=
ing.c
index 98e6d31463e0..0ff0454b9d98 100644
--- a/MdePkg/Library/BaseLib/String.c
+++ b/MdePkg/Library/BaseLib/String.c
@@ -6,6 +6,7 @@
=20
**/
=20
+#include <Uefi/UefiBaseType.h>
#include "BaseLibInternals.h"
=20
/**
@@ -408,7 +409,8 @@ StrDecimalToUintn (
{
UINTN Result;
=20
- StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result);
+ Result =3D !EFI_ERROR (StrDecimalToUintnS (String, (CHAR16 **)NULL, &R=
esult)) ? Result : MAX_UINTN;
+
return Result;
}
=20
@@ -454,7 +456,8 @@ StrDecimalToUint64 (
{
UINT64 Result;
=20
- StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result);
+ Result =3D !EFI_ERROR (StrDecimalToUint64S (String, (CHAR16 **)NULL, &=
Result)) ? Result : MAX_UINT64;
+
return Result;
}
=20
@@ -501,7 +504,8 @@ StrHexToUintn (
{
UINTN Result;
=20
- StrHexToUintnS (String, (CHAR16 **)NULL, &Result);
+ Result =3D !EFI_ERROR (StrHexToUintnS (String, (CHAR16 **)NULL, &Resul=
t)) ? Result : MAX_UINTN;
+
return Result;
}
=20
@@ -548,7 +552,7 @@ StrHexToUint64 (
{
UINT64 Result;
=20
- StrHexToUint64S (String, (CHAR16 **)NULL, &Result);
+ Result =3D !EFI_ERROR (StrHexToUint64S (String, (CHAR16 **)NULL, &Resu=
lt)) ? Result : MAX_UINT64;
return Result;
}
=20
@@ -989,7 +993,7 @@ AsciiStrDecimalToUintn (
{
UINTN Result;
=20
- AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result);
+ Result =3D !EFI_ERROR (AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL=
, &Result)) ? Result : MAX_UINTN;
return Result;
}
=20
@@ -1031,7 +1035,7 @@ AsciiStrDecimalToUint64 (
{
UINT64 Result;
=20
- AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result);
+ Result =3D !EFI_ERROR (AsciiStrDecimalToUint64S (String, (CHAR8 **)NUL=
L, &Result)) ? Result : MAX_UINT64;
return Result;
}
=20
@@ -1077,7 +1081,7 @@ AsciiStrHexToUintn (
{
UINTN Result;
=20
- AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result);
+ Result =3D !EFI_ERROR (AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &R=
esult)) ? Result : MAX_UINTN;
return Result;
}
=20
@@ -1123,7 +1127,7 @@ AsciiStrHexToUint64 (
{
UINT64 Result;
=20
- AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result);
+ Result =3D !EFI_ERROR (AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &=
Result)) ? Result : MAX_UINT64;
return Result;
}
=20
--=20
2.28.0.windows.1