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.14088.1669256044080141555 for ; Wed, 23 Nov 2022 18:14:04 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=JmkGGHOt; 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 C9B6120B83C2; Wed, 23 Nov 2022 18:14:02 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com C9B6120B83C2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1669256043; bh=DkS1DJEvQe3ESetCHO+XWISabSVCnY7dPo9ltiv+Zv8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=JmkGGHOtTjVs8ain+gkMQCFLtFi0noN8bC8v4BI3HBvE1Qk8Ea+2l6c8UTU4Vxjdt OJPuCmkMiaXkr+OwJm4hugYsLywxaL5QF3ya6BIcNKGwpj3rMOR6epcIn+gN1f2rfR 2C7/+PzpSmOBAmDPTd9GytKqqAcwRGOW5cZwpwms= Message-ID: <3af32683-0162-dc9c-3cb7-e21751fe6e1f@linux.microsoft.com> Date: Wed, 23 Nov 2022 21:14:01 -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: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: "Dong, Eric" , Erich McMillan , "Kumar, Rahul R" , "Ni, Ray" References: <20221109173246.174-1-mikuback@linux.microsoft.com> <20221109173246.174-11-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 The ASSERT() was added to aid debugging by bringing attention to the point where the fallback assignment occurs in the error status check block. Are you suggesting the ASSERT() be removed because of a known case where it might trigger but the case is expected to return an error? Or, that is is not necessary in general? Thanks, Michael On 11/23/2022 9:04 PM, Michael D Kinney wrote: > Hi Michael, > > comments below. > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Michael Kubacki >> Sent: Wednesday, November 9, 2022 9:33 AM >> To: devel@edk2.groups.io >> Cc: Dong, Eric ; Erich McMillan ; Kinney, Michael D >> ; Michael Kubacki ; Kumar, Rahul R ; >> Ni, Ray >> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables >> >> From: Michael Kubacki >> >> Fixes CodeQL alerts for CWE-457: >> https://cwe.mitre.org/data/definitions/457.html >> >> Cc: Eric Dong >> Cc: Erich McMillan >> Cc: Michael D Kinney >> Cc: Michael Kubacki >> Cc: Rahul Kumar >> Cc: Ray Ni >> Co-authored-by: Erich McMillan >> Signed-off-by: Michael Kubacki >> --- >> UefiCpuPkg/CpuMpPei/CpuBist.c | 8 +++++++- >> UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++++++- >> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++- >> 3 files changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c >> index 7dc93cd784d4..122808139b87 100644 >> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c >> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c >> @@ -175,7 +175,13 @@ CollectBistDataFromPpi ( >> EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2; >> EFI_SEC_PLATFORM_INFORMATION_CPU *CpuInstanceInHob; >> >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors); >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors); >> + ASSERT_EFI_ERROR (Status); > > I think this ASSERT() should be removed. The added error status check looks correct. > >> + >> + if (EFI_ERROR (Status)) { >> + NumberOfProcessors = 1u; >> + NumberOfEnabledProcessors = 1u; >> + } >> >> BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) + >> sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors; >> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c >> index e7f1fe9f426c..a84304273168 100644 >> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c >> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c >> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers ( >> return; >> } >> >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); >> + ASSERT_EFI_ERROR (Status); > > I think this ASSERT() should be removed. The added error status check looks correct. > >> + >> + if (EFI_ERROR (Status)) { >> + NumberOfProcessors = 1u; >> + } >> + >> SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT))); >> ASSERT (SwitchStackData != NULL); >> ZeroMem (SwitchStackData, NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)); >> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c >> index 135422225340..1322fcb77f28 100644 >> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c >> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c >> @@ -538,6 +538,7 @@ SetupStackGuardPage ( >> UINTN NumberOfProcessors; >> UINTN Bsp; >> UINTN Index; >> + EFI_STATUS Status; >> >> // >> // One extra page at the bottom of the stack is needed for Guard page. >> @@ -547,7 +548,13 @@ SetupStackGuardPage ( >> ASSERT (FALSE); >> } >> >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); >> + ASSERT_EFI_ERROR (Status); > > I think this ASSERT() should be removed. The added error status check looks correct. > >> + >> + if (EFI_ERROR (Status)) { >> + NumberOfProcessors = 1u; >> + } >> + >> MpInitLibWhoAmI (&Bsp); >> for (Index = 0; Index < NumberOfProcessors; ++Index) { >> StackBase = 0; >> -- >> 2.28.0.windows.1 >> >> >> >> -=-=-=-=-=-= >> Groups.io Links: You receive all messages sent to this group. >> View/Reply Online (#96156): https://edk2.groups.io/g/devel/message/96156 >> Mute This Topic: https://groups.io/mt/94918104/1643496 >> Group Owner: devel+owner@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com] >> -=-=-=-=-=-= >> > > > > > >