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

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

[Wu, Hao A]

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

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

[Michael D Kinney]

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

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

[Ard Biesheuvel]

On Mon, 22 Apr 2019 at 16:41, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> 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.
>

Is there any way we can #define the OUT modifier to something the
static analyzer understands? (Which static analyzer is this btw?)

Surely, we are not the only project dealing with pointers that are
initialized by reference. Adding code to please the tools should
really be the last resort imo.

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

[Michael D Kinney]

Ard,

This seems to be a common limitation seen in some
static analyzers.  We have not found a workaround
that does not involve code changes to quiet the 
false positives.

For this specific case, I think the code change I
suggest is correct.  

Best regards,

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, April 22, 2019 2:26 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>;
> devel@edk2.groups.io; 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
> 
> On Mon, 22 Apr 2019 at 16:41, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > 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.
> >
> 
> Is there any way we can #define the OUT modifier to
> something the
> static analyzer understands? (Which static analyzer is
> this btw?)
> 
> Surely, we are not the only project dealing with
> pointers that are
> initialized by reference. Adding code to please the
> tools should
> really be the last resort imo.

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

[Ard Biesheuvel]

On Mon, 22 Apr 2019 at 23:53, Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> Ard,
>
> This seems to be a common limitation seen in some
> static analyzers.  We have not found a workaround
> that does not involve code changes to quiet the
> false positives.
>
> For this specific case, I think the code change I
> suggest is correct.
>

I agree that the change is correct, and isn't that intrusive in this
particular case, so I don't have any objections to it.

I was just thinking aloud whether the IN vs OUT modifiers could be put
to use here. There are some examples in Linux of the patten

#ifdef __CHECKER__
#define ...
#else
#define ...
#endif

where __CHECKER__ is only set by the 'sparse' tool, which is basically
a combination of a static checker with a more pedantic coding style
checker.

I guess in our case, we'dl have to cater for multiple build
environments and more than one static checker, so this is probably not
as easy to achieve, unfortunately.

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

[Andrew Fish]

> On Apr 22, 2019, at 3:02 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On Mon, 22 Apr 2019 at 23:53, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
>> 
>> Ard,
>> 
>> This seems to be a common limitation seen in some
>> static analyzers.  We have not found a workaround
>> that does not involve code changes to quiet the
>> false positives.
>> 
>> For this specific case, I think the code change I
>> suggest is correct.
>> 
> 
> I agree that the change is correct, and isn't that intrusive in this
> particular case, so I don't have any objections to it.
> 
> I was just thinking aloud whether the IN vs OUT modifiers could be put
> to use here. There are some examples in Linux of the patten
> 
> #ifdef __CHECKER__
> #define ...
> #else
> #define ...
> #endif
> 
> where __CHECKER__ is only set by the 'sparse' tool, which is basically
> a combination of a static checker with a more pedantic coding style
> checker.
> 
> I guess in our case, we'dl have to cater for multiple build
> environments and more than one static checker, so this is probably not
> as easy to achieve, unfortunately.
> 

Ard,

This would be a really good item to put on our longer term list of enhancements. 

Thanks,

Andrew Fish

> 
>