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

["Wang, Jian J" <jian.j.wang@intel.com>]

Leif,

Thanks for the comments. I'll push the patch after removing the change log
from the commit log.

Regards,
Jian


> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Wednesday, November 07, 2018 3:53 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [PATCH v3 1/2] MdeModulePkg/Core: fill logic hole in
> MemoryProtectionCpuArchProtocolNotify
> 
> 
> 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
> >