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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

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