MdeModulePkg/Core/Dxe/Hand/Handle.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
> -----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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
> -----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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.