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

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

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