From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: michael.d.kinney@intel.com)
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
 by groups.io with SMTP; Mon, 22 Apr 2019 07:41:00 -0700
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga001.jf.intel.com ([10.7.209.18])
  by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Apr 2019 07:41:00 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.60,382,1549958400"; 
   d="scan'208";a="225640472"
Received: from orsmsx108.amr.corp.intel.com ([10.22.240.6])
  by orsmga001.jf.intel.com with ESMTP; 22 Apr 2019 07:40:59 -0700
Received: from orsmsx113.amr.corp.intel.com ([169.254.9.24]) by
 ORSMSX108.amr.corp.intel.com ([169.254.2.238]) with mapi id 14.03.0415.000;
 Mon, 22 Apr 2019 07:40:59 -0700
From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: "Wu, Hao A" <hao.a.wu@intel.com>, "devel@edk2.groups.io"
	<devel@edk2.groups.io>, "Kinney, Michael D" <michael.d.kinney@intel.com>
CC: Ard Biesheuvel <ard.biesheuvel@linaro.org>, "Gao, Liming"
	<liming.gao@intel.com>, "Wang, Jian J" <jian.j.wang@intel.com>
Subject: Re: [PATCH v1] MdeModulePkg/DxeCore: Please static checker for false report
Thread-Topic: [PATCH v1] MdeModulePkg/DxeCore: Please static checker for
 false report
Thread-Index: AQHU+NyCFLJSQ9UzLUCX3X75IX3GN6ZIP1zA
Date: Mon, 22 Apr 2019 14:40:59 +0000
Message-ID: <E92EE9817A31E24EB0585FDF735412F5B9C9C2DC@ORSMSX113.amr.corp.intel.com>
References: <20190422072447.10548-1-hao.a.wu@intel.com>
In-Reply-To: <20190422072447.10548-1-hao.a.wu@intel.com>
Accept-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
dlp-product: dlpe-windows
dlp-version: 11.0.600.7
dlp-reaction: no-action
x-originating-ip: [10.22.254.138]
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

Hi Hao,

I think a cleaner fix to this issues is replace both
ASSERT() statements with the following:

      if (EFI_ERROR (Status) || Entry->Emulator =3D=3D NULL) {
        FreePool (Entry);
        continue;
      }

We do not expect the emulator protocol to disappear between
finding the handle and looking up the protocol instance,=20
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=20
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
>=20
> After commit 57df17fe26, some static check reports
> suspicous NULL pointer
> deference at line:
>=20
>   Entry->MachineType =3D Entry->Emulator->MachineType;
>                        ^^^^^^^^^^^^^^^
>=20
> within function PeCoffEmuProtocolNotify().
>=20
> However, 'Entry->Emulator' is guaranteed to have a non-
> NULL value when
> previous call to the CoreHandleProtocol() returns
> EFI_SUCCESS.
>=20
> Thus, in order to please the static checker, this
> commit will add an
> ASSERT right before the false-positive NULL pointer
> dereference report.
>=20
> 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(+)
>=20
> 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 !=3D NULL)
>=20
>      Entry->MachineType =3D Entry->Emulator->MachineType;
>=20
> --
> 2.12.0.windows.1