Can you please resend the patch series. Thanks,
Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Mike > Maslenkin > Sent: Thursday, August 31, 2023 2:20 PM > To: devel@edk2.groups.io; Desimone, Nathaniel L > <nathaniel.l.desim...@intel.com> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming > <gaolim...@byosoft.com.cn>; Wang, Jian J <jian.j.w...@intel.com>; Bi, > Dandan <dandan...@intel.com> > Subject: Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fix memory leak in > LocateHandleBuffer() > > 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 (#108219): https://edk2.groups.io/g/devel/message/108219 Mute This Topic: https://groups.io/mt/101056724/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
