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

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report
@ 2019-04-24  5:05 Wu, Hao A
  2019-04-24  7:07 ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: Wu, Hao A @ 2019-04-24  5:05 UTC (permalink / raw)
  To: devel; +Cc: Hao Wu, Ard Biesheuvel, Michael D Kinney, Liming Gao, Jian J Wang

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>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report
  2019-04-24  5:05 [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report Wu, Hao A
@ 2019-04-24  7:07 ` Ard Biesheuvel
  2019-04-26  5:38   ` Wu, Hao A
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2019-04-24  7:07 UTC (permalink / raw)
  To: Hao Wu; +Cc: edk2-devel-groups-io, Michael D Kinney, Liming Gao, Jian J Wang

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>

> ---
>  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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report
  2019-04-24  7:07 ` Ard Biesheuvel
@ 2019-04-26  5:38   ` Wu, Hao A
  2019-04-26  7:32     ` Liming Gao
  2019-04-26 14:50     ` Michael D Kinney
  0 siblings, 2 replies; 6+ messages in thread
From: Wu, Hao A @ 2019-04-26  5:38 UTC (permalink / raw)
  To: Ard Biesheuvel, Kinney, Michael D, Gao, Liming, Wang, Jian J
  Cc: edk2-devel-groups-io

> -----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
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report
  2019-04-26  5:38   ` Wu, Hao A
@ 2019-04-26  7:32     ` Liming Gao
  2019-04-26 14:50     ` Michael D Kinney
  1 sibling, 0 replies; 6+ messages in thread
From: Liming Gao @ 2019-04-26  7:32 UTC (permalink / raw)
  To: Wu, Hao A, Ard Biesheuvel, Kinney, Michael D, Wang, Jian J
  Cc: edk2-devel-groups-io

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
>> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report
  2019-04-26  5:38   ` Wu, Hao A
  2019-04-26  7:32     ` Liming Gao
@ 2019-04-26 14:50     ` Michael D Kinney
  2019-04-28  0:33       ` Wu, Hao A
  1 sibling, 1 reply; 6+ messages in thread
From: Michael D Kinney @ 2019-04-26 14:50 UTC (permalink / raw)
  To: Wu, Hao A, Ard Biesheuvel, Gao, Liming, Wang, Jian J,
	Kinney, Michael D
  Cc: edk2-devel-groups-io

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, April 25, 2019 10: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
> > >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report
  2019-04-26 14:50     ` Michael D Kinney
@ 2019-04-28  0:33       ` Wu, Hao A
  0 siblings, 0 replies; 6+ messages in thread
From: Wu, Hao A @ 2019-04-28  0:33 UTC (permalink / raw)
  To: Kinney, Michael D, Ard Biesheuvel, Gao, Liming, Wang, Jian J
  Cc: edk2-devel-groups-io

Thanks all.
Patch pushed via commit dfaa565559.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Friday, April 26, 2019 10:51 PM
> To: Wu, Hao A; Ard Biesheuvel; Gao, Liming; Wang, Jian J; Kinney, Michael D
> Cc: edk2-devel-groups-io
> Subject: RE: [PATCH v2] MdeModulePkg/DxeCore: Please static checker for
> false report
> 
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> 
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Thursday, April 25, 2019 10: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
> > > >

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-04-28  0:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-24  5:05 [PATCH v2] MdeModulePkg/DxeCore: Please static checker for false report Wu, Hao A
2019-04-24  7:07 ` Ard Biesheuvel
2019-04-26  5:38   ` Wu, Hao A
2019-04-26  7:32     ` Liming Gao
2019-04-26 14:50     ` Michael D Kinney
2019-04-28  0:33       ` Wu, Hao A

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox