[edk2-devel] [PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Hi Igor,
Thanks for catching this issue, I have a comment below inline.

> -----Original Message-----
> From: Igor Kulchytskyy <igork at ami.com>
> Sent: Saturday, February 11, 2023 5:43 AM
> To: devel at edk2.groups.io
> Cc: Chang, Abner <Abner.Chang at amd.com>; Nickle Wang
> <nicklew at nvidia.com>
> Subject: [PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> gRedfishDiscoveredToken buffer is allocated as one piece during protocol
> installed process, but deleted by parts during driver unload process.
> 
> Cc: Abner Chang <abner.chang at amd.com>
> Cc: Nickle Wang <nicklew at nvidia.com>
> Cc: Igor Kulchytskyy <igork at ami.com>
> Signed-off-by: Igor Kulchytskyy <igork at ami.com>
> ---
>  RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> b/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> index 96ac70f418..64b7fb7841 100644
> --- a/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> +++ b/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> @@ -481,6 +481,7 @@ RedfishDiscoverProtocolInstalled (
>  ErrorReturn:
>    if (gRedfishDiscoveredToken != NULL) {
>      FreePool (gRedfishDiscoveredToken);
> +    gRedfishDiscoveredToken = NULL;
>    }
>  }
> 
> @@ -511,10 +512,10 @@ RedfishConfigHandlerDriverUnload (
>          gBS->CloseEvent (ThisRedfishDiscoveredToken->Event);
>        }
> 
> -      FreePool (ThisRedfishDiscoveredToken);
>        ThisRedfishDiscoveredToken++;
>      }
> 
> +    FreePool (gRedfishDiscoveredToken);
I found here is a potential issue of gRedfishDiscoveredToken:
Due to RedfishDiscoverProtocolInstalled() may be called more than once in the case of multiple NIC installed on the system, means gRedfishDiscoveredToken will be overwritten by AllocateZeroPool() and result in memory leakage when unload the RedfishConfigHandler driver.
Could you please help to create an linked list (e.g. mRedfishDiscoveredTokenList) to record the newly allocated memory for gRedfishDiscoveredToken (I think we can rename it to just RedfishDiscoveredToken)?
So we can go through the link list to free RedfishDiscoveredToken allocated for each NIC when unload the driver.
Does this make sense?
Thanks
Abner

>      gRedfishDiscoveredToken = NULL;
>    }
> 
> --
> 2.37.1.windows.1
> -The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by
> telephone at 770-246-8600, and then delete or destroy all copies of the
> transmission.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100157): https://edk2.groups.io/g/devel/message/100157
Mute This Topic: https://groups.io/mt/96886534/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-