From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=michael.d.kinney@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 3BCF021F2AF94 for ; Thu, 28 Sep 2017 09:45:39 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Sep 2017 09:48:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,450,1500966000"; d="scan'208";a="904814029" Received: from orsmsx102.amr.corp.intel.com ([10.22.225.129]) by FMSMGA003.fm.intel.com with ESMTP; 28 Sep 2017 09:48:53 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.135]) by ORSMSX102.amr.corp.intel.com ([169.254.3.17]) with mapi id 14.03.0319.002; Thu, 28 Sep 2017 09:48:53 -0700 From: "Kinney, Michael D" To: "Wu, Hao A" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Zeng, Star" , "Yao, Jiewen" Thread-Topic: [PATCH] MdeModulePkg/DxeCore: Add comments for the ASSERT to check NULL ptr Thread-Index: AQHTOCeQWC4oTO2g2EqxpnLGN8wpMKLKgMgA Date: Thu, 28 Sep 2017 16:48:51 +0000 Message-ID: References: <20170928070039.632-1-hao.a.wu@intel.com> In-Reply-To: <20170928070039.632-1-hao.a.wu@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.22.254.139] 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: Thu, 28 Sep 2017 16:45:39 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 ; Zeng, Star > ; Kinney, Michael D > ; Yao, Jiewen > > Subject: [PATCH] MdeModulePkg/DxeCore: Add comments for the > ASSERT to check NULL ptr >=20 > Commit 8932679df5be046feba30fae80776c5815232a08 adds an ASSERT > for > checking NULL pointer dereference. >=20 > This commit adds comments to clarify the reason for using > ASSERT as the > check. >=20 > 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(-) >=20 > 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