Re: [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in MemoryProtectionCpuArchProtocolNotify

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Nov 07, 2018 at 03:12:47PM +0800, Jian J Wang wrote:
> > v3: fixed one more memory leak in the same function and updated
> >     commit message accordingly.

No objection to the content, but comments not intended to be committed
should go in the cover letter or below the ---.

A changelog between revisions is best kept in the cover letter,
including all changes in reverse chronological order, like:

Changes since v2:
- ...

Changes since v1:
- ...

I expect whoever pushes the patches will edit the history out of the
commit message, but putting it in the cover-letter means less work for
them.

(I won't give a reviewed-by since I have only been commenting on
style, not functionality.)

/
    Leif

> At the end of of MemoryProtectionCpuArchProtocolNotify there's cleanup
> code to free resource. But at line 978, 994, 1005 the function returns
> directly. This patch use "goto" to replace "return" to make sure the
> resource is freed before exit.
> 
> 1029:  CoreCloseEvent (Event);
> 1030:  return;
> 
> There's another memory leak after calling gBS->LocateHandleBuffer() in
> the same function:
> 
>   Status = gBS->LocateHandleBuffer (
>                   ByProtocol,
>                   &gEfiLoadedImageProtocolGuid,
>                   NULL,
>                   &NoHandles,
>                   &HandleBuffer
>                   );
> 
> HandleBuffer is allocated in above call but never freed. This patch
> will also add code to free it.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 6298b67db1..8a93c5362a 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -975,7 +975,7 @@ MemoryProtectionCpuArchProtocolNotify (
>    DEBUG ((DEBUG_INFO, "MemoryProtectionCpuArchProtocolNotify:\n"));
>    Status = CoreLocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&gCpu);
>    if (EFI_ERROR (Status)) {
> -    return;
> +    goto Done;
>    }
>  
>    //
> @@ -991,7 +991,7 @@ MemoryProtectionCpuArchProtocolNotify (
>    HeapGuardCpuArchProtocolNotify ();
>  
>    if (mImageProtectionPolicy == 0) {
> -    return;
> +    goto Done;
>    }
>  
>    Status = gBS->LocateHandleBuffer (
> @@ -1002,7 +1002,7 @@ MemoryProtectionCpuArchProtocolNotify (
>                    &HandleBuffer
>                    );
>    if (EFI_ERROR (Status) && (NoHandles == 0)) {
> -    return ;
> +    goto Done;
>    }
>  
>    for (Index = 0; Index < NoHandles; Index++) {
> @@ -1025,9 +1025,10 @@ MemoryProtectionCpuArchProtocolNotify (
>  
>      ProtectUefiImage (LoadedImage, LoadedImageDevicePath);
>    }
> +  FreePool (HandleBuffer);
>  
> +Done:
>    CoreCloseEvent (Event);
> -  return;
>  }
>  
>  /**
> -- 
> 2.16.2.windows.1
>