Re: [PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]

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

> -----Original Message-----
> From: Igor Kulchytskyy <igork@ami.com>
> Sent: Saturday, February 11, 2023 5:43 AM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; Nickle Wang
> <nicklew@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@amd.com>
> Cc: Nickle Wang <nicklew@nvidia.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Signed-off-by: Igor Kulchytskyy <igork@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.