* [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk
@ 2016-09-29 0:38 Michael Kinney
[not found] ` <734D49CCEBEEF84792F5B80ED585239D58E0CE24@SHSMSX104.ccr.corp.intel.com>
2016-09-29 2:51 ` Ni, Ruiyu
0 siblings, 2 replies; 6+ messages in thread
From: Michael Kinney @ 2016-09-29 0:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Jaben Carsey, Ruiyu Ni
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk
[not found] ` <734D49CCEBEEF84792F5B80ED585239D58E0CE24@SHSMSX104.ccr.corp.intel.com>
@ 2016-09-29 2:11 ` Kinney, Michael D
2016-09-29 2:36 ` Ni, Ruiyu
0 siblings, 1 reply; 6+ messages in thread
From: Kinney, Michael D @ 2016-09-29 2:11 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org, Kinney, Michael D; +Cc: Carsey, Jaben
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk
2016-09-29 2:11 ` Kinney, Michael D
@ 2016-09-29 2:36 ` Ni, Ruiyu
2016-09-29 2:48 ` Kinney, Michael D
0 siblings, 1 reply; 6+ messages in thread
From: Ni, Ruiyu @ 2016-09-29 2:36 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Carsey, Jaben
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk
2016-09-29 2:36 ` Ni, Ruiyu
@ 2016-09-29 2:48 ` Kinney, Michael D
0 siblings, 0 replies; 6+ messages in thread
From: Kinney, Michael D @ 2016-09-29 2:48 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org, Kinney, Michael D; +Cc: Carsey, Jaben
Ray,
That solution sounds good too, but you should update the other functions in the same file the duplicate the device path when that is not needed.
The patch I provided just followed the example in the function above.
Mike
From: Ni, Ruiyu
Sent: Wednesday, September 28, 2016 7:36 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,
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<mailto:ruiyu.ni@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: Carsey, Jaben <jaben.carsey@intel.com<mailto: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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk
2016-09-29 0:38 [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk Michael Kinney
[not found] ` <734D49CCEBEEF84792F5B80ED585239D58E0CE24@SHSMSX104.ccr.corp.intel.com>
@ 2016-09-29 2:51 ` Ni, Ruiyu
[not found] ` <E92EE9817A31E24EB0585FDF735412F56481EC72@ORSMSX113.amr.corp.intel.com>
1 sibling, 1 reply; 6+ messages in thread
From: Ni, Ruiyu @ 2016-09-29 2:51 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Carsey, Jaben
I agree to use the same style of code as what the other functions do.
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>-----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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk
[not found] ` <E92EE9817A31E24EB0585FDF735412F56481EC72@ORSMSX113.amr.corp.intel.com>
@ 2016-10-03 18:24 ` Carsey, Jaben
0 siblings, 0 replies; 6+ messages in thread
From: Carsey, Jaben @ 2016-10-03 18:24 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel@lists.01.org
Reviewed-by: Jaben Carsey <Jaben.carsey@intel.com>
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, September 28, 2016 7:52 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
>
> I agree to use the same style of code as what the other functions do.
>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> -----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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-03 18:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-29 0:38 [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk Michael Kinney
[not found] ` <734D49CCEBEEF84792F5B80ED585239D58E0CE24@SHSMSX104.ccr.corp.intel.com>
2016-09-29 2:11 ` Kinney, Michael D
2016-09-29 2:36 ` Ni, Ruiyu
2016-09-29 2:48 ` Kinney, Michael D
2016-09-29 2:51 ` Ni, Ruiyu
[not found] ` <E92EE9817A31E24EB0585FDF735412F56481EC72@ORSMSX113.amr.corp.intel.com>
2016-10-03 18:24 ` Carsey, Jaben
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox