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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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.wu@intel.com>; 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>
> 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.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>
> ---
>  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