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

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

Mike,

Thanks for the feedbacks. I will refine the commit message and code
comments according to your suggestions.


Best Regards,
Hao Wu

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Friday, September 29, 2017 12:49 AM
> To: Wu, Hao A; edk2-devel@lists.01.org; Kinney, Michael D
> Cc: Zeng, Star; Yao, Jiewen
> Subject: RE: [PATCH] MdeModulePkg/DxeCore: Add comments for the ASSERT
> to check NULL ptr
> 
> Hao Wu,
> 
> The comment block clearly describes that the condition is not
> possible, so we would never expect this ASSERT() condition to
> ever be triggered.  Looking at the comment in this patch and
> the ASSERT() statement, a developer in the future may be tempted
> to remove this comment and ASSERT() thinking there is no impact.
> 
> The real reason the ASSERT() is added is because of a false
> positive report from static analysis.
> 
> Please add to the commit message and the comment block that
> this ASSERT() is added to address a false positive from
> static analysis, so it is clear that this ASSERT() should
> not be removed.
> 
> Thanks,
> 
> Mike
> 
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Thursday, September 28, 2017 12:01 AM
> > To: edk2-devel@lists.01.org
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Zeng, Star
> > <star.zeng@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>
> > Subject: [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);
> > +      //
> >        // 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