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

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

[Hao Wu]

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

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

[Udit Kumar]

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

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

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

[Wu, Hao A]

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

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

[Kinney, Michael D]

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

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

[Wu, Hao A]

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