From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4597F21F3041A for ; Thu, 28 Sep 2017 17:48:10 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 28 Sep 2017 17:51:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,451,1500966000"; d="scan'208";a="317494804" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga004.fm.intel.com with ESMTP; 28 Sep 2017 17:51:26 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 28 Sep 2017 17:51:25 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 28 Sep 2017 17:51:25 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.159]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.98]) with mapi id 14.03.0319.002; Fri, 29 Sep 2017 08:51:23 +0800 From: "Wu, Hao A" To: "Kinney, Michael D" , "edk2-devel@lists.01.org" CC: "Zeng, Star" , "Yao, Jiewen" Thread-Topic: [PATCH] MdeModulePkg/DxeCore: Add comments for the ASSERT to check NULL ptr Thread-Index: AQHTOHmsJFjOGPY9dk29yhLeUD4dsqLLCGYA Date: Fri, 29 Sep 2017 00:51:22 +0000 Message-ID: References: <20170928070039.632-1-hao.a.wu@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/DxeCore: Add comments for the ASSERT to check NULL ptr X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Sep 2017 00:48:11 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 >=20 > Hao Wu, >=20 > 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. >=20 > The real reason the ASSERT() is added is because of a false > positive report from static analysis. >=20 > 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. >=20 > Thanks, >=20 > Mike >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Thursday, September 28, 2017 12:01 AM > > To: edk2-devel@lists.01.org > > Cc: Wu, Hao A ; Zeng, Star > > ; Kinney, Michael D > > ; Yao, Jiewen > > > > 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 > > Cc: Michael D Kinney > > Cc: Jiewen Yao > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Hao Wu > > --- > > 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 =3D=3D 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 !=3D NULL); > > + // > > // EFI_ALREADY_STARTED is not an error for bus driver. > > // Return the corresponding protocol interface. > > // > > - ASSERT (Prot !=3D NULL); > > *Interface =3D Prot->Interface; > > } else if (Status =3D=3D EFI_UNSUPPORTED) { > > // > > -- > > 2.12.0.windows.1