Re: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report

["Liming Gao" <liming.gao@intel.com>]

Hao:
   The change is good to me. Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Wu, Hao A
>Sent: Friday, April 26, 2019 1:39 PM
>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D
><michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang,
>Jian J <jian.j.wang@intel.com>
>Cc: edk2-devel-groups-io <devel@edk2.groups.io>
>Subject: RE: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for
>false report
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Wednesday, April 24, 2019 3:07 PM
>> To: Wu, Hao A
>> Cc: edk2-devel-groups-io; Kinney, Michael D; Gao, Liming; Wang, Jian J
>> Subject: Re: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for
>> false report
>>
>> On Wed, 24 Apr 2019 at 07:05, Hao Wu <hao.a.wu@intel.com> wrote:
>> >
>> > 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.
>> >
>> > This commit will re-write the return status check for CoreHandleProtocol()
>> > to add explicit NULL pointer check for protocol instance pointer.
>> >
>> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> > Cc: Liming Gao <liming.gao@intel.com>
>> > Cc: Jian J Wang <jian.j.wang@intel.com>
>> > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>>
>> Again, I think it is rather unfortunate that we need code changes such
>> as this one just to remove warnings from a flawed static analyzer. But
>> the change looks correct to me, so
>>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
>Thanks Ard,
>
>Hello Mike, Liming and Jian,
>
>Do you have additional feedbacks on this patch?
>If not, I plan to push it with the 'Ack' tag from Ard.
>
>Best Regards,
>Hao Wu
>
>>
>> > ---
>> >  MdeModulePkg/Core/Dxe/Image/Image.c | 23 ++++++++++++--------
>> >  1 file changed, 14 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
>> b/MdeModulePkg/Core/Dxe/Image/Image.c
>> > index 08306a73fd..de5b8bed27 100644
>> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
>> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
>> > @@ -134,12 +134,14 @@ PeCoffEmuProtocolNotify (
>> >    IN  VOID            *Context
>> >    )
>> >  {
>> > -  EFI_STATUS          Status;
>> > -  UINTN               BufferSize;
>> > -  EFI_HANDLE          EmuHandle;
>> > -  EMULATOR_ENTRY      *Entry;
>> > +  EFI_STATUS                            Status;
>> > +  UINTN                                 BufferSize;
>> > +  EFI_HANDLE                            EmuHandle;
>> > +  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL  *Emulator;
>> > +  EMULATOR_ENTRY                        *Entry;
>> >
>> >    EmuHandle = NULL;
>> > +  Emulator  = NULL;
>> >
>> >    while (TRUE) {
>> >      BufferSize = sizeof (EmuHandle);
>> > @@ -157,16 +159,19 @@ PeCoffEmuProtocolNotify (
>> >        return;
>> >      }
>> >
>> > -    Entry = AllocateZeroPool (sizeof (*Entry));
>> > -    ASSERT (Entry != NULL);
>> > -
>> >      Status = CoreHandleProtocol (
>> >                 EmuHandle,
>> >                 &gEdkiiPeCoffImageEmulatorProtocolGuid,
>> > -               (VOID **)&Entry->Emulator
>> > +               (VOID **)&Emulator
>> >                 );
>> > -    ASSERT_EFI_ERROR (Status);
>> > +    if (EFI_ERROR (Status) || Emulator == NULL) {
>> > +      continue;
>> > +    }
>> > +
>> > +    Entry = AllocateZeroPool (sizeof (*Entry));
>> > +    ASSERT (Entry != NULL);
>> >
>> > +    Entry->Emulator    = Emulator;
>> >      Entry->MachineType = Entry->Emulator->MachineType;
>> >
>> >      InsertTailList (&mAvailableEmulators, &Entry->Link);
>> > --
>> > 2.12.0.windows.1
>> >