From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.13860.1669255156024691993 for ; Wed, 23 Nov 2022 17:59:16 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=kATvKt4W; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id EE59E20B83C2; Wed, 23 Nov 2022 17:59:14 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com EE59E20B83C2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1669255155; bh=CxXWrBmCUdhQ3vw2pboueGNMHsUkY7sATNjiq0grM/E=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=kATvKt4WpiVVq3l4xm1rNQtLxPtpJo6lObCYM1S+JgA3Or6MYvhldd6btIBu1/B++ QslxUJz2QBHj7NOZ2KRkrlzTKhErk/77LlhTZEj5Imt68QT6U3cCU5LJ64Ry0FdrIR 2qYh2dVpehnzKuJqDumhp0hwkqSq65wLgnYXLR40= Message-ID: <9b4a11c3-5227-9cd8-a79b-2328d96dcb93@linux.microsoft.com> Date: Wed, 23 Nov 2022 20:59:13 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables To: "Kinney, Michael D" , "devel@edk2.groups.io" Cc: Erich McMillan , "Gao, Liming" , "Liu, Zhiguang" References: <20221109173246.174-1-mikuback@linux.microsoft.com> <20221109173246.174-7-mikuback@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 >> Sent: Wednesday, November 9, 2022 9:33 AM >> To: devel@edk2.groups.io >> Cc: Erich McMillan ; Gao, Liming ; Kinney, Michael D >> ; Michael Kubacki ; Liu, Zhiguang >> Subject: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables >> >> From: Michael Kubacki >> >> Fixes CodeQL alerts for CWE-457: >> https://cwe.mitre.org/data/definitions/457.html >> >> Cc: Erich McMillan >> Cc: Liming Gao >> Cc: Michael D Kinney >> Cc: Michael Kubacki >> Cc: Zhiguang Liu >> Co-authored-by: Erich McMillan >> Signed-off-by: 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 > > Why is this change needed? > > I think this should be for a library of type BASE > and BaseLibInternals.h includes . 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 >