From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id AEB17941D63 for ; Thu, 31 Aug 2023 20:02:54 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=A+mWinO69CFWHiTxJVhlRdzG6PpyVePLNXgOOogAZpA=; c=relaxed/simple; d=groups.io; h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:From:To:CC:Subject:Thread-Topic:Thread-Index:Date:Message-ID:References:In-Reply-To:Accept-Language:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1693512173; v=1; b=Cr6/NFBaKwVv/Lh7uXhTs62SXs8m9pLD8J6+q+TmdsMCx2AGpePZUndJ23HFR+GfUtvqO+69 0opjh1X8GV/fAYJHTNHqXfuE6XZRL7z+dOYs9uvj1ypP+sEmHsPjEPY1+i8txPT03zvpjkZAJi+ RsCRKR4W0QYxifyzRUCfA4sc= X-Received: by 127.0.0.2 with SMTP id rRgSYY7687511xJ1XhafPbqv; Thu, 31 Aug 2023 13:02:53 -0700 X-Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web10.4779.1693512172249466066 for ; Thu, 31 Aug 2023 13:02:52 -0700 X-IronPort-AV: E=McAfee;i="6600,9927,10819"; a="366279303" X-IronPort-AV: E=Sophos;i="6.02,217,1688454000"; d="scan'208";a="366279303" X-Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Aug 2023 13:02:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10819"; a="768929259" X-IronPort-AV: E=Sophos;i="6.02,217,1688454000"; d="scan'208";a="768929259" X-Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by orsmga008.jf.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 31 Aug 2023 13:02:34 -0700 X-Received: from fmsmsx610.amr.corp.intel.com (10.18.126.90) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Thu, 31 Aug 2023 13:02:33 -0700 X-Received: from fmsedg602.ED.cps.intel.com (10.1.192.136) by fmsmsx610.amr.corp.intel.com (10.18.126.90) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27 via Frontend Transport; Thu, 31 Aug 2023 13:02:33 -0700 X-Received: from NAM11-CO1-obe.outbound.protection.outlook.com (104.47.56.169) by edgegateway.intel.com (192.55.55.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.27; Thu, 31 Aug 2023 13:02:32 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZIW1c+8fh1gmoBiU/wCLN9GtkopcoQx8SaW9yREA06mxdazwdjN5jLGayUDQGKR046Ys9+24YAreKP/3g87o4nAGEln7TUSQKfDCdZ1+h29F0/VWRi6bFJI9ox5y96DGdt7bRsDthCTfCGfmUlmUZAKY7XSDRQ6sJ5bYW7Yzufk3BIftAsyvqvYMUUjCIhRlaHmCZRoIn3l6WTf4RoUukMNEEh8b6CDju2znLh/tg/wJNzHIYz1N+BjDbN2zRhr+EwP0Z5DfgZFDI+M9FsSKSQiTaUb3BugNEDnmSkHp8uaW3pDFqyhkmiC+xTv3zifL94HETlRQvjK9fdg1VXTnbg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=9+aokd1/B1rk+5Mn9GXrIRYGWtxMpTTpt11jZZJOByM=; b=BocET1BE0A9Zu7t22PKvzcoPo1AdClLs+FO99vpfFoOo44t4qPiyUUx9+rI73Izi91HOnCRP8jA35Ir94zDzNDcvgDQnnCoENt1+5MOfgnztWRyH2d+LLPiTfEGHnd4rYZp5izlzbIcxNc85dSxxIfLJXBGilUM6ppmFnBw0C2CnxhT0429HEwebJuHynE+0w0DcHJl70qUY1i9aTkOf0DF0YMCy2KePSHeLwNXxEpDOc+iSc7QWVVZL/j2MQ7Xt0eI45vPuSmi9O8YOT1Euzr5hZ/6//AUfzNCK9sWRpyt3GQQb87RZCQUvn6gnPnHCNrEgH26zVUZQtIi53LHvQw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none X-Received: from MW4PR11MB5821.namprd11.prod.outlook.com (2603:10b6:303:184::5) by IA1PR11MB6099.namprd11.prod.outlook.com (2603:10b6:208:3d5::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6745.22; Thu, 31 Aug 2023 20:02:29 +0000 X-Received: from MW4PR11MB5821.namprd11.prod.outlook.com ([fe80::89db:ecf0:29f5:9f3c]) by MW4PR11MB5821.namprd11.prod.outlook.com ([fe80::89db:ecf0:29f5:9f3c%4]) with mapi id 15.20.6699.035; Thu, 31 Aug 2023 20:02:29 +0000 From: "Nate DeSimone" To: "Kinney, Michael D" , "devel@edk2.groups.io" CC: "Gao, Liming" , "Wang, Jian J" , "Bi, Dandan" Subject: Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fix memory leak in LocateHandleBuffer() Thread-Topic: [PATCH v1] MdeModulePkg: Fix memory leak in LocateHandleBuffer() Thread-Index: AQHZ22GrMTTqwXSP+0ay5ZaisevlALAEqhjwgAASvjA= Date: Thu, 31 Aug 2023 20:02:29 +0000 Message-ID: References: <20230830164641.588-1-nathaniel.l.desimone@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MW4PR11MB5821:EE_|IA1PR11MB6099:EE_ x-ms-office365-filtering-correlation-id: ebf8f13d-116f-42e3-50b8-08dbaa5d3493 x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: bMWU6yJPrmTQfPd4VXXEq3HGh2E4ZksPvQhanZQDLLqdmRmqHSTYnBv/wSefqIXHk5rAH7goa57tmq6EgJ13sE7Q9Mmo/dp1AuEYgywYQm2xnwcpvnXYgpxtiXhdO/iY7EGJlIVIwDyg2Kh9kO6Ip5loxX1ye33Tjjxg3p34pOUHQmE5sG2w/4FP0gYsVT7NrB/i7HnKQ8O/Mq3k+xld5/4dmSe7IWqQx/bHFVhPW6GN0P5KKRR47Wmm5rrddSY9pwvKf2cXz4w/vNFPh1jrtvUQnh1gungCbW5VGt0UlFqP+DErMVpn5DdF/oIlY241K7jBZ4RsK1VzjH8qtClpjkw5Qe93Q8h1GDVqLRC+TxZH8sYnjSQFtFRKsK8UoD3RtjRBQQB6eh6sugknzAQkbMZQahL+EgaK+atJC0+Tfw8e5FUHOVSHXkPqY7JRaljjxsqA16UAjcmbJJaTf1WKeHed9yVwZOEcuyu8rbwwOns3QehpZ88lNlJac08KMFS+hwMAQdk3wKNLu+gKXeFNe9xTf8BApl0z/jwXuC2PrqsLn3k0RQoiddabi1qrp2eIfu31iEkUGNzVPpfwHNYnDhDnIndXQueZh/z21Sung9B1cYt5yTVgTF6lMPAGo+ciG4gzZkEypNdjtEAlu/jkaQ== x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?NRoAxbvNf//PeVxsipWEevd2ntOGysYvt52g39tp7PrlbAskyaRLAPNiO/GE?= =?us-ascii?Q?mJDTHJqYH39NinAIdYeyjrjDyO8Xy7yqvukK8QaFZa1Jktq8DZ1hHZoGqWtS?= =?us-ascii?Q?/DDSsrMmW5jPIWW9q7qz8tEZlAB/My0dT+EzpiFoReXgq312zMg3l2byXM/6?= =?us-ascii?Q?9kEYyaXHhECK1T6A0yo06uZ28Etjddgan7h+3Pm3NhDX3a3oqRAa4JdCL1gH?= =?us-ascii?Q?jMDTldXZm+5wpXVLAZXYfIa5zVIaXrJrOd9B5h0T6P1h2u04hVMW/ytxfID8?= =?us-ascii?Q?T035gEMzSmyrjiIyYZ5dYljpIByNG2tLd1xVnyi0820g0zW8f30wup0p+yIK?= =?us-ascii?Q?D89l/MAM2ja1y0F7yt5rSdauWVr7UTcEdcxVOmjZCvZy0EIl+bDA/juCBqIp?= =?us-ascii?Q?Eegk9NIg1awSpHbaTIpoob+5m7mNXaN2DepkyDrW5xhKxrOh/kE1cMHEd/bg?= =?us-ascii?Q?5WrC4Zmuf8lnZOFCLzlICjrd1TGRTmsAwluBXXp4wlv1maxnP0BESyKYUiLo?= =?us-ascii?Q?SMVDbufrtTyYoE4yNg6jkEpJHv5dkVk2iE+E8ucN+N04okbPAoM3U7rBJQ4F?= =?us-ascii?Q?kQlZee7gp1/ywmA1+QkUU4gOZ8hdMre8yshFJTEokVf+ehoHBB9CHFX8CA3T?= =?us-ascii?Q?sBTJBmpWIg6WEtTgH4AA/y3zW/7nP8DXX4RHnuh2PBlcudVqbYuHmnDb4h0E?= =?us-ascii?Q?Gn7yz7uAiUzpegakbVyHrKGR+vp1fjh+0Fe79DzFbnYjUYCZZQUzZe7BbGRu?= =?us-ascii?Q?bhTeXrw3xwNboWgpotbFh53vgMjTnLQMZavTXE82M34OcbPuvotXQpVzXuc1?= =?us-ascii?Q?GUOGmzoNIb8gKnPbSM12IGFYo/FNvqFQHw3mSh4V0lsDp4UW68GLzvPt6Lpo?= =?us-ascii?Q?tpHoIsAhrNpp06y8E2HitNP1J1pGk3VuQ7NlNHxLtLIrGwRi1UwUQesCA45a?= =?us-ascii?Q?67rIc8uBa9DgzaVLQp4129Vabck/2GIIN3fmzfOdjRGpocWhTO457vOE8XHx?= =?us-ascii?Q?G1d96BpposEUXV+xIRBbLZp12GCD2KvK+JtvGn4sGpGyhdL9rweEZtNBLJNa?= =?us-ascii?Q?J9mo5zFu3MPXJ/yOf/LasNskqeyIHIxQfUEWcCk822nrNUPF7uFsgM3K+W3f?= =?us-ascii?Q?Dy+J9T5gCwxxZBDbH/crNmcWMEY5o8F7n4NOlxkRZwP3LjV74yjCq+FHSsd/?= =?us-ascii?Q?svb0ZsNjhDyk+w89a98T5M2RTRGhhmhC31Y4z/itQ2bmWhvK80OpbVJXWueL?= =?us-ascii?Q?C0vNQ+ehgcWfk2sKpBW4357zfj0Ha/i7fqcLQDijfm9rlosnCZJmiyHxc5JE?= =?us-ascii?Q?gQYkja/P+kdRZoFfhyuIQS8iRFFq3nMMRq6/a7h1g4xgOE2SPQeGvK/7HiP3?= =?us-ascii?Q?nJPxlBlFvsFyBl1bdZ6ZfkyLQY1Nq4Qea5rS/HyzzL0NlT3SnIwue8hZkRTS?= =?us-ascii?Q?3lh8BBlEoIby419rnAozmXZ7maQ+83zG+u/xPKhTTD2R39G8enMzUuabxw8Y?= =?us-ascii?Q?pUaT0tsXrlI4zNH8CEavZKPPlCa/1DEWOU9X3kp6HHKvR7ehqNIlxSOJOEws?= =?us-ascii?Q?oAdSyzGOJJWdPpcOP0CRz9M82cbSknzWYouzfxipxyq/I5QyTMCg1RD9HFi3?= =?us-ascii?Q?Ww=3D=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MW4PR11MB5821.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: ebf8f13d-116f-42e3-50b8-08dbaa5d3493 X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Aug 2023 20:02:29.3303 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: XSmd6z3lWsr/52vYz4IzJTlA743ofWZLmGmRrMybrnS7UrpHc2l6os4/Fo0VtnC6NUiV5KXoJfuk8Kyg2tU2ZXAg88y0xJxiW5SHnZFqQP8= X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6099 X-OriginatorOrg: intel.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,nathaniel.l.desimone@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: L0DvFMFNE1OqjnCUWC3w1t8vx7686176AA= Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="Cr6/NFBa"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=intel.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hi Mike, I agree that it should be extremely rare for the 1st call to succeed AND th= e 2nd call to fail. The only case I can think of where that could happen is= if the call to AllocatePool() in CoreFindProtocolEntry() fails due to an o= ut of memory scenario. As always thank you for your careful review. Pushed: https://github.com/tianocore/edk2/commit/beafabd Thanks, Nate -----Original Message----- From: Kinney, Michael D =20 Sent: Thursday, August 31, 2023 10:34 AM To: Desimone, Nathaniel L ; devel@edk2.grou= ps.io Cc: Gao, Liming ; Wang, Jian J ; Bi, Dandan ; Kinney, Michael D Subject: RE: [PATCH v1] MdeModulePkg: Fix memory leak in LocateHandleBuffer= () Hi Nate, I do not disagree with the logic of the patch. Reviewed-by: Michael D Kinney However, I do not understand how the 1st call to InternalCoreLocateHandle() can succeed and the 2nd call can fail if the han= dle database protocol lock is in the acquired state. Is there a real scenario where this can happen? How would the state of the handle database change between the 2 calls? If the answer is that this scenario can not happen,then the impact to any c= ode calling this API for this change is zero. Mike > -----Original Message----- > From: Desimone, Nathaniel L > Sent: Wednesday, August 30, 2023 9:47 AM > To: devel@edk2.groups.io > Cc: Gao, Liming ; Wang, Jian J=20 > ; Kinney, Michael D=20 > ; Bi, Dandan > Subject: [PATCH v1] MdeModulePkg: Fix memory leak in=20 > LocateHandleBuffer() >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4543 > REF:=20 > https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi- > boot-services-locatehandlebuffer >=20 > CoreLocateHandleBuffer() can in certain cases, can return an error and=20 > not free an allocated buffer. This scenario occurs if the first call=20 > to InternalCoreLocateHandle() returns success and the second call=20 > returns an error. >=20 > On a successful return, LocateHandleBuffer() passes ownership of the=20 > buffer to the caller. However, the UEFI specification is not explicit=20 > about what the expected ownership of this buffer is in the case of an=20 > error. > However, it is heavily implied by the code example given in section=20 > 7.3.15 of v2.10 of the UEFI specificaton that if LocateHandleBuffer()=20 > returns a non-successful status code then the ownership of the buffer=20 > does NOT transfer to the caller. This code example explicitly refrains=20 > from calling FreePool() if LocateHandleBuffer() returns an error. >=20 > From a practical standpoint, it is logical to assume that a=20 > non-successful status code indicates that no buffer of handles was=20 > ever allocated. Indeed, in most error cases, > LocateHandleBuffer() does not go far enough to get to the point where=20 > a buffer is allocated. Therefore, all existing users of this API must=20 > already be coded to support the case of a non-successful status code=20 > resulting in an invalid handle buffer being returned. Therefore, this=20 > change will not cause any backwards compatibility issues with existing=20 > code. >=20 > In conclusion, this boils down to a fix for a memory leak that also=20 > brings the behavior of our LocateHandleBuffer() implementation into=20 > alignment with the original intentions of the UEFI specification=20 > authors. >=20 > Cc: Liming Gao > Cc: Jian J Wang > Cc: Michael D Kinney > Cc: Dandan Bi > Signed-off-by: Nate DeSimone > --- > MdeModulePkg/Core/Dxe/Hand/Locate.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) >=20 > diff --git a/MdeModulePkg/Core/Dxe/Hand/Locate.c > b/MdeModulePkg/Core/Dxe/Hand/Locate.c > index a29010a545..8f20c6332d 100644 > --- a/MdeModulePkg/Core/Dxe/Hand/Locate.c > +++ b/MdeModulePkg/Core/Dxe/Hand/Locate.c > @@ -1,7 +1,7 @@ > /** @file > Locate handle functions >=20 > -Copyright (c) 2006 - 2018, Intel Corporation. All rights=20 > reserved.
> +Copyright (c) 2006 - 2023, Intel Corporation. All rights=20 > +reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > **/ > @@ -730,6 +730,10 @@ CoreLocateHandleBuffer ( > *NumberHandles =3D BufferSize / sizeof (EFI_HANDLE); > if (EFI_ERROR (Status)) { > *NumberHandles =3D 0; > + if (*Buffer !=3D NULL) { > + CoreFreePool (*Buffer); > + *Buffer =3D NULL; > + } > } >=20 > CoreReleaseProtocolLock (); > -- > 2.34.1 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108208): https://edk2.groups.io/g/devel/message/108208 Mute This Topic: https://groups.io/mt/101056724/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-