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

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

[Nate DeSimone]

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

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

[Michael D Kinney]

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

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

[Nate DeSimone]

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

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

[Mike Maslenkin]

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 (#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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael D Kinney]

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]
-=-=-=-=-=-=-=-=-=-=-=-