Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fix memory leak in LocateHandleBuffer()

["Michael D Kinney" <michael.d.kinney@intel.com>]

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.desimone@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Wang, Jian J <jian.j.wang@intel.com>; Bi,
> Dandan <dandan.bi@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.desimone@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.kinney@intel.com>
> > Sent: Thursday, August 31, 2023 10:34 AM
> > To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
> devel@edk2.groups.io
> > Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Wang, Jian J
> <jian.j.wang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Kinney,
> Michael D <michael.d.kinney@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.kinney@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.desimone@intel.com>
> > > Sent: Wednesday, August 30, 2023 9:47 AM
> > > To: devel@edk2.groups.io
> > > Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Wang, Jian J
> > > <jian.j.wang@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@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 <gaoliming@byosoft.com.cn>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Dandan Bi <dandan.bi@intel.com>
> > > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-