MdeModulePkg/Core/Dxe/Image/Image.c | 7 +++++++ 1 file changed, 7 insertions(+)
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39362): https://edk2.groups.io/g/devel/message/39362
Mute This Topic: https://groups.io/mt/31271609/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39369): https://edk2.groups.io/g/devel/message/39369
Mute This Topic: https://groups.io/mt/31271609/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39372): https://edk2.groups.io/g/devel/message/39372
Mute This Topic: https://groups.io/mt/31271609/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39373): https://edk2.groups.io/g/devel/message/39373
Mute This Topic: https://groups.io/mt/31271609/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39374): https://edk2.groups.io/g/devel/message/39374 Mute This Topic: https://groups.io/mt/31271609/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> 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 > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39379): https://edk2.groups.io/g/devel/message/39379 Mute This Topic: https://groups.io/mt/31271609/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.