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 62332740040 for ; Thu, 31 Aug 2023 21:20:20 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=UYZ/xCm5u412HgY1SOaC0IPPEeae76MIn025yE1k/n4=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1693516818; v=1; b=QNvx41bjrqU1FveHXaSu9g2WVWGSLA+xU9plJaCb10ehxMF+M7wP1ifhiVwuiCJC+m3podbC OLxnY5nXz9TQO9+23KdBNmmNNC2F96sZIdt01WCS0xvmXsnso++J7Z7ZC+FnFAfDQsQe+RdE6+v LwbxXvFJhPY5bN6iE83Xek/M= X-Received: by 127.0.0.2 with SMTP id mf2RYY7687511xvF1gQUjwJV; Thu, 31 Aug 2023 14:20:18 -0700 X-Received: from mail-yb1-f176.google.com (mail-yb1-f176.google.com [209.85.219.176]) by mx.groups.io with SMTP id smtpd.web10.6473.1693516818202366195 for ; Thu, 31 Aug 2023 14:20:18 -0700 X-Received: by mail-yb1-f176.google.com with SMTP id 3f1490d57ef6-d7ba4c5f581so1018865276.0 for ; Thu, 31 Aug 2023 14:20:17 -0700 (PDT) X-Gm-Message-State: HEd3KbQ8TuEow4qvnKEDHmmIx7686176AA= X-Google-Smtp-Source: AGHT+IEa0z0Q3Yu80Iwa0x0ncjEdLKqkim9orWtSfnTDZfFHlP6uJ7r7fet2oTnpOS61lStg4T9yyn45+fATlX31EQ0= X-Received: by 2002:a25:404e:0:b0:d0d:2d17:3f11 with SMTP id n75-20020a25404e000000b00d0d2d173f11mr860511yba.17.1693516817013; Thu, 31 Aug 2023 14:20:17 -0700 (PDT) MIME-Version: 1.0 References: <20230830164641.588-1-nathaniel.l.desimone@intel.com> In-Reply-To: From: "Mike Maslenkin" Date: Fri, 1 Sep 2023 00:19:40 +0300 Message-ID: Subject: Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fix memory leak in LocateHandleBuffer() To: devel@edk2.groups.io, nathaniel.l.desimone@intel.com Cc: "Kinney, Michael D" , "Gao, Liming" , "Wang, Jian J" , "Bi, Dandan" 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,mike.maslenkin@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" 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=QNvx41bj; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.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 A gentle ping :) https://edk2.groups.io/g/devel/message/92452 As MdeModulePkg/Core was modified, might it be possible to add more? Should I resend those patches? MdeModulePkg/Core maintainers didn't reply. Regards, MIke. On Thu, Aug 31, 2023 at 11:02=E2=80=AFPM Nate DeSimone wrote: > > Hi Mike, > > I agree that it should be extremely rare for the 1st call to succeed AND = the 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= out 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 > Sent: Thursday, August 31, 2023 10:34 AM > To: Desimone, Nathaniel L ; devel@edk2.gr= oups.io > Cc: Gao, Liming ; Wang, Jian J ; Bi, Dandan ; Kinney, Michael D > Subject: RE: [PATCH v1] MdeModulePkg: Fix memory leak in LocateHandleBuff= er() > > 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 h= andle 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= code 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 > > ; Kinney, Michael D > > ; Bi, Dandan > > Subject: [PATCH v1] MdeModulePkg: Fix memory leak in > > LocateHandleBuffer() > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4543 > > REF: > > https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi- > > boot-services-locatehandlebuffer > > > > CoreLocateHandleBuffer() can in certain cases, can return an error and > > not free an allocated buffer. This scenario occurs if the first call > > to InternalCoreLocateHandle() returns success and the second call > > returns an error. > > > > On a successful return, LocateHandleBuffer() passes ownership of the > > buffer to the caller. However, the UEFI specification is not explicit > > about what the expected ownership of this buffer is in the case of an > > error. > > However, it is heavily implied by the code example given in section > > 7.3.15 of v2.10 of the UEFI specificaton that if LocateHandleBuffer() > > returns a non-successful status code then the ownership of the buffer > > does NOT transfer to the caller. This code example explicitly refrains > > from calling FreePool() if LocateHandleBuffer() returns an error. > > > > From a practical standpoint, it is logical to assume that a > > non-successful status code indicates that no buffer of handles was > > ever allocated. Indeed, in most error cases, > > LocateHandleBuffer() does not go far enough to get to the point where > > a buffer is allocated. Therefore, all existing users of this API must > > already be coded to support the case of a non-successful status code > > resulting in an invalid handle buffer being returned. Therefore, this > > change will not cause any backwards compatibility issues with existing > > code. > > > > In conclusion, this boils down to a fix for a memory leak that also > > brings the behavior of our LocateHandleBuffer() implementation into > > alignment with the original intentions of the UEFI specification > > authors. > > > > 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(-) > > > > 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 > > > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights > > reserved.
> > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > > +reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -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; > > + } > > } > > > > CoreReleaseProtocolLock (); > > -- > > 2.34.1 > > > >=20 > > -=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 (#108209): https://edk2.groups.io/g/devel/message/108209 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-