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