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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Mike,
How about remove the DuplicateDevicePath and FreePool?
------
.../Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c    | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
index a50b52f..bfdbb40 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c
@@ -340,9 +340,7 @@ HDiskImageSave (
   IN UINTN  Size
   )
{
-
-  CONST EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
-  EFI_DEVICE_PATH_PROTOCOL        *DupDevicePath;
+  EFI_DEVICE_PATH_PROTOCOL        *DevicePath;
   EFI_BLOCK_IO_PROTOCOL           *BlkIo;
   EFI_STATUS                      Status;
   EFI_HANDLE                      Handle;
@@ -358,18 +356,15 @@ HDiskImageSave (
   HBufferImage.BufferType = FileTypeDiskBuffer;
-  DevicePath              = gEfiShellProtocol->GetDevicePathFromMap(DeviceName);
+  DevicePath              = (EFI_DEVICE_PATH_PROTOCOL *) gEfiShellProtocol->GetDevicePathFromMap(DeviceName);
   if (DevicePath == NULL) {
//    StatusBarSetStatusString (L"Cannot Find Device");
     return EFI_INVALID_PARAMETER;
   }
-  DupDevicePath = DuplicateDevicePath(DevicePath);
-
   //
   // get blkio interface
   //
-  Status = gBS->LocateDevicePath(&gEfiBlockIoProtocolGuid,&DupDevicePath,&Handle);
-  FreePool(DupDevicePath);
+  Status = gBS->LocateDevicePath(&gEfiBlockIoProtocolGuid,&DevicePath,&Handle);
   if (EFI_ERROR (Status)) {
//    StatusBarSetStatusString (L"Read Disk Failed");
     return Status;

Regards,
Ray

From: Kinney, Michael D
Sent: Thursday, September 29, 2016 10:12 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Carsey, Jaben <jaben.carsey@intel.com>
Subject: RE: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk

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<mailto:michael.d.kinney@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Carsey, Jaben <jaben.carsey@intel.com<mailto: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<mailto:edk2-devel@lists.01.org>
> >Cc: Carsey, Jaben <jaben.carsey@intel.com<mailto:jaben.carsey@intel.com>>; Ni, Ruiyu <ruiyu.ni@intel.com<mailto: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<mailto:jaben.carsey@intel.com>>
> >Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> >Contributed-under: TianoCore Contribution Agreement 1.0
> >Signed-off-by: Michael Kinney <michael.d.kinney@intel.com<mailto: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