Hi Michael, Comments below.
Mike > -----Original Message----- > From: mikub...@linux.microsoft.com <mikub...@linux.microsoft.com> > Sent: Wednesday, November 9, 2022 9:33 AM > To: devel@edk2.groups.io > Cc: Erich McMillan <emcmil...@microsoft.com>; Gao, Liming > <gaolim...@byosoft.com.cn>; Kinney, Michael D > <michael.d.kin...@intel.com>; Michael Kubacki <mikub...@linux.microsoft.com>; > Liu, Zhiguang <zhiguang....@intel.com> > Subject: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables > > From: Michael Kubacki <michael.kuba...@microsoft.com> > > Fixes CodeQL alerts for CWE-457: > https://cwe.mitre.org/data/definitions/457.html > > Cc: Erich McMillan <emcmil...@microsoft.com> > Cc: Liming Gao <gaolim...@byosoft.com.cn> > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Cc: Michael Kubacki <mikub...@linux.microsoft.com> > Cc: Zhiguang Liu <zhiguang....@intel.com> > Co-authored-by: Erich McMillan <emcmil...@microsoft.com> > Signed-off-by: Michael Kubacki <michael.kuba...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
