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 PM Nate DeSimone <nathaniel.l.desim...@intel.com> 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 <michael.d.kin...@intel.com> > Sent: Thursday, August 31, 2023 10:34 AM > To: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; > devel@edk2.groups.io > Cc: Gao, Liming <gaolim...@byosoft.com.cn>; Wang, Jian J > <jian.j.w...@intel.com>; Bi, Dandan <dandan...@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > 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 <michael.d.kin...@intel.com> > > However, I do not understand how the 1st call to > InternalCoreLocateHandle() can succeed and the 2nd call can fail if the > handle 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 <nathaniel.l.desim...@intel.com> > > Sent: Wednesday, August 30, 2023 9:47 AM > > To: devel@edk2.groups.io > > Cc: Gao, Liming <gaolim...@byosoft.com.cn>; Wang, Jian J > > <jian.j.w...@intel.com>; Kinney, Michael D > > <michael.d.kin...@intel.com>; Bi, Dandan <dandan...@intel.com> > > Subject: [PATCH v1] MdeModulePkg: Fix memory leak in > > LocateHandleBuffer() > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4543 > > 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 <gaolim...@byosoft.com.cn> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Dandan Bi <dandan...@intel.com> > > Signed-off-by: Nate DeSimone <nathaniel.l.desim...@intel.com> > > --- > > 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.<BR> > > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > > +reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -730,6 +730,10 @@ CoreLocateHandleBuffer ( > > *NumberHandles = BufferSize / sizeof (EFI_HANDLE); > > if (EFI_ERROR (Status)) { > > *NumberHandles = 0; > > + if (*Buffer != NULL) { > > + CoreFreePool (*Buffer); > > + *Buffer = NULL; > > + } > > } > > > > CoreReleaseProtocolLock (); > > -- > > 2.34.1 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
