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 (#108203): https://edk2.groups.io/g/devel/message/108203 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] -=-=-=-=-=-=-=-=-=-=-=-
