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

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

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