Re: [PATCH] MdeModulePkg/DxeCore: Add comments for the ASSERT to check NULL ptr

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Udit
> Kumar
> Sent: Thursday, September 28, 2017 5:28 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Kinney, Michael D; Yao, Jiewen; Zeng, Star
> Subject: Re: [edk2] [PATCH] MdeModulePkg/DxeCore: Add comments for the
> ASSERT to check NULL ptr
> 
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Hao
> > Wu
> > Sent: Thursday, September 28, 2017 12:31 PM
> > To: edk2-devel@lists.01.org
> > Cc: Hao Wu <hao.a.wu@intel.com>; Michael D Kinney
> > <michael.d.kinney@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Star
> Zeng
> > <star.zeng@intel.com>
> > Subject: [edk2] [PATCH] MdeModulePkg/DxeCore: Add comments for the
> > ASSERT to check NULL ptr
> >
> > Commit 8932679df5be046feba30fae80776c5815232a08 adds an ASSERT for
> > checking NULL pointer dereference.
> >
> > This commit adds comments to clarify the reason for using ASSERT as the
> check.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> > ---
> >  MdeModulePkg/Core/Dxe/Hand/Handle.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > index 2db441725c..344ff1fe02 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > @@ -1175,10 +1175,15 @@ Done:
> >      //
> >      if (!EFI_ERROR (Status) || Status == EFI_ALREADY_STARTED) {
> >        //
> > +      // According to above logic, if 'Prot' is NULL, then the 'Status' must be
> > +      // EFI_UNSUPPORTED. Here the 'Status' is not EFI_UNSUPPORTED, so
> 'Prot'
> > +      // must be not NULL.
> > +      //
> > +      ASSERT (Prot != NULL);
> > +      //
> 
> I think , we should take care of no debug environment here
> If MDEPKG_NDEBUG is not defined and Prot is NULL then
> shouldn't we return error ?

Hi Udit Kumar,

As mentioned in another feedback for this patch from Mike, the ASSERT here
is added duet to a false positive report from static analysis.

The code logic actually ensures that 'Prot' will not be NULL within the
'if' statement:

"if (!EFI_ERROR (Status) || Status == EFI_ALREADY_STARTED) {"

I will refine the patch to add more comments for the ASSERT used here so
that later if there's improvement for the static analysis, we can locate
and remove this ASSERT by searching keywords in the comments.


Best Regards,
Hao Wu

> 
> >        // EFI_ALREADY_STARTED is not an error for bus driver.
> >        // Return the corresponding protocol interface.
> >        //
> > -      ASSERT (Prot != NULL);
> >        *Interface = Prot->Interface;
> >      } else if (Status == EFI_UNSUPPORTED) {
> >        //
> > --
> > 2.12.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel