On 2/27/2024 10:43 PM, Laszlo Ersek wrote:
On 2/27/24 17:04, Michael Kubacki wrote:
Hi Gerd,
There is a way to suppress results explained here:
https://github.com/tianocore/edk2/tree/master/BaseTools/Plugin/CodeQL#filter-patterns
A real-world example is here:
https://github.com/microsoft/mu_basecore/blob/release/202311/CodeQlFilters.yml
That can currently operate at the file and CodeQL rule level
granularity. In this case, the null pointer test rule
("cpp/missing-null-test" as shown in
https://github.com/tianocore/edk2/security/code-scanning/1277) could be
excluded in MpLib.c.
---
Taking a quick look at the code highlighted:
MpHandOffConfig = GetMpHandOffConfigHob ();
ASSERT (MpHandOffConfig != NULL);
DEBUG ((
DEBUG_INFO,
"FirstMpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *):
%04d\n",
MpHandOffConfig->WaitLoopExecutionMode,
sizeof (VOID *)
));
if (MpHandOffConfig->WaitLoopExecutionMode == sizeof (VOID *)) {
CodeQL flagged the two MpHandOffConfig dereferences. These is assigned
on the return value from GetMpHandOffConfigHob () defined as:
/**
Get pointer to MP_HAND_OFF_CONFIG GUIDed HOB body.
@return The pointer to MP_HAND_OFF_CONFIG structure.
**/
MP_HAND_OFF_CONFIG *
GetMpHandOffConfigHob (
VOID
)
{
EFI_HOB_GUID_TYPE *GuidHob;
GuidHob = GetFirstGuidHob (&mMpHandOffConfigGuid);
if (GuidHob == NULL) {
return NULL;
}
return (MP_HAND_OFF_CONFIG *)GET_GUID_HOB_DATA (GuidHob);
}
Can you please provide more context about why you believe a NULL return
value from the function is not a consideration? Generally, anything is
possible in the HOB list, for example, other code could overflow a HOB
boundary and mutate the this HOB's header preventing it from being found.
The GetMpHandOffConfigHob() function itself is right, when viewed in
isolation, to deal with the possibility of the HOB missing.
However, the GetMpHandOffConfigHob() *call site* is only reached if the
"FirstMpHandOff" variable is not NULL.
And if "FirstMpHandOff" is not NULL, then in the PEI phase, the
SaveCpuMpData() function [UefiCpuPkg/Library/MpInitLib/PeiMpLib.c] will
have prepared *both* at least one MpHandOff GUID HOB, *and* the sole
MpHandOffConfig GUID HOB.
In other words, the presence of at least one MpHandOff GUID HOB
guarantees that there is going to be exactly one MpHandOffConfig GUID HOB.
Therefore the ASSERT() is right -- it expresses an invariant.
Thanks. With those details, I agree ASSERT() is reasonable.
That said:
ASSERT() is useful for debug but it's control path is unpredictable in
core code based on platform policies that often have varying
perspectives of how to apply asserts and how they should be configured
in different scenarios. We introduced a "panic library" to use in rare
cases where we want more consistent behavior for fatal conditions.
I agree that making this more explicit wouldn't hurt.
The panic library is not upstream (yet? :))
I added Ken from my team as a reminder to prepare that for upstream soon.
, so I'll suggest a
CpuDeadLoop() under the patch itself:
[PATCH 1/1] UefiCpuPkg/MpInitLib: add struct MP_HAND_OFF_CONFIG
msgid <20240227114122.1140614-1-kra...@redhat.com>
https://edk2.groups.io/g/devel/message/116029
Thanks,
Laszlo
Thanks,
Michael
On 2/27/2024 6:39 AM, Gerd Hoffmann wrote:
Hi,
I am hoping we can work together to improve the overall quality of the
code and minimize the number of CodeQL alerts.
Seems CodeQL now runs as part of CI and flags issues it has found.
It complains about a possible NULL pointer dereference:
https://github.com/tianocore/edk2/runs/22021016348
This is not correct, but I doubt code analysis will ever be clever
enough to figure this automatically. So I've added an ASSERT()
explicitly saying so, which should help both human reviewers and
code analyzers.
Apparently that does not change anything for CodeQL though. I guess
the CodeQL config must be updated so it knows what ASSERT() means?
Maybe it is ignored simply because it is upper case (unlike the
standard C library version which is lower case)?
thanks & take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116078): https://edk2.groups.io/g/devel/message/116078
Mute This Topic: https://groups.io/mt/102444916/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
