Re: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Ray,

I agree that the contents of the device path buffer is not changed.

&DupDevicePath is passed into LocateDevicePath() and the
value pointed to by &DupDevicePath is modified.

Then the call to FreePool(DupDevicePath) uses a different 
DupDevicePath value than was returned by 
DuplicateDevicePath(DevicePath).  That generated CR macro
ASSERT().

Mike


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, September 28, 2016 7:00 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>
> Subject: RE: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk
> 
> Mike,
> BS.LocateDevicePath() doesn't change the device path content.
> So I think the Duplication/Free of device path is not needed.
> 
> Regards,
> Ray
> 
> >-----Original Message-----
> >From: Kinney, Michael D
> >Sent: Thursday, September 29, 2016 8:38 AM
> >To: edk2-devel@lists.01.org
> >Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> >Subject: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk
> >
> >The HDiskImageSave() function copies a device path using
> >DuplicateDevicePath() and passes that device path to
> >gBS->LocateDevicePath() that changes the value of the
> >device path pointer.  When FreePool() is called with the
> >modified device path pointer, the FreePool() service
> >generates an ASSERT() because the signature for the pool
> >head can not be found.
> >
> >The function HDiskImageRead() immediately above
> >HDiskImageSave() has the correct algorithm that uses an
> >additional local variable called DupDevicePathForFree to
> >preserve the pointer to the allocated buffer so it can
> >be used in the call to FreePool().
> >
> >Bug: <https://bugzilla.tianocore.org/show_bug.cgi?id=131>
> >
> >Cc: Jaben Carsey <jaben.carsey@intel.com>
> >Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >Contributed-under: TianoCore Contribution Agreement 1.0
> >Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
> >---
> > ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> >diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
> >b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
> >index a50b52f..bc74a4f 100644
> >--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
> >+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
> >@@ -343,6 +343,7 @@ HDiskImageSave (
> >
> >   CONST EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
> >   EFI_DEVICE_PATH_PROTOCOL        *DupDevicePath;
> >+  EFI_DEVICE_PATH_PROTOCOL        *DupDevicePathForFree;
> >   EFI_BLOCK_IO_PROTOCOL           *BlkIo;
> >   EFI_STATUS                      Status;
> >   EFI_HANDLE                      Handle;
> >@@ -364,12 +365,13 @@ HDiskImageSave (
> >     return EFI_INVALID_PARAMETER;
> >   }
> >   DupDevicePath = DuplicateDevicePath(DevicePath);
> >+  DupDevicePathForFree = DupDevicePath;
> >
> >   //
> >   // get blkio interface
> >   //
> >   Status = gBS->LocateDevicePath(&gEfiBlockIoProtocolGuid,&DupDevicePath,&Handle);
> >-  FreePool(DupDevicePath);
> >+  FreePool(DupDevicePathForFree);
> >   if (EFI_ERROR (Status)) {
> > //    StatusBarSetStatusString (L"Read Disk Failed");
> >     return Status;
> >--
> >2.6.3.windows.1