Hi Hao, I think a cleaner fix to this issues is replace both ASSERT() statements with the following:
if (EFI_ERROR (Status) || Entry->Emulator == NULL) { FreePool (Entry); continue; } We do not expect the emulator protocol to disappear between finding the handle and looking up the protocol instance, but if it does, the handle can be skipped without ASSERT(). There are several examples of this style in DriverSupport.c. If we want to avoid the extra Allocate/Free in this error condition, then a local variable can be added to get the emulator protocol instance and only allocate an EMULATOR_ENTRY if the emulator instance is successfully found. Thanks, Mike > -----Original Message----- > From: Wu, Hao A > Sent: Monday, April 22, 2019 12:25 AM > To: devel@edk2.groups.io > Cc: Wu, Hao A <hao.a...@intel.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; Kinney, Michael D > <michael.d.kin...@intel.com>; Gao, Liming > <liming....@intel.com>; Wang, Jian J > <jian.j.w...@intel.com> > Subject: [PATCH v1] MdeModulePkg/DxeCore: Please static > checker for false report > > After commit 57df17fe26, some static check reports > suspicous NULL pointer > deference at line: > > Entry->MachineType = Entry->Emulator->MachineType; > ^^^^^^^^^^^^^^^ > > within function PeCoffEmuProtocolNotify(). > > However, 'Entry->Emulator' is guaranteed to have a non- > NULL value when > previous call to the CoreHandleProtocol() returns > EFI_SUCCESS. > > Thus, in order to please the static checker, this > commit will add an > ASSERT right before the false-positive NULL pointer > dereference report. > > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Cc: Liming Gao <liming....@intel.com> > Cc: Jian J Wang <jian.j.w...@intel.com> > Signed-off-by: Hao Wu <hao.a...@intel.com> > --- > MdeModulePkg/Core/Dxe/Image/Image.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c > b/MdeModulePkg/Core/Dxe/Image/Image.c > index 08306a73fd..546fa96eee 100644 > --- a/MdeModulePkg/Core/Dxe/Image/Image.c > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c > @@ -166,6 +166,13 @@ PeCoffEmuProtocolNotify ( > (VOID **)&Entry->Emulator > ); > ASSERT_EFI_ERROR (Status); > + // > + // When the above CoreHandleProtocol() call > returns with EFI_SUCCESS, > + // 'Entry->Emulator' is guaranteed to have a non- > NULL value. > + // The below ASSERT is for addressing a false > positive NULL pointer > + // dereference issue raised from static analysis. > + // > + ASSERT (Entry->Emulator != NULL) > > Entry->MachineType = Entry->Emulator->MachineType; > > -- > 2.12.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39369): https://edk2.groups.io/g/devel/message/39369 Mute This Topic: https://groups.io/mt/31271609/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-