From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EB1DF1A1DEB for ; Wed, 28 Sep 2016 19:48:24 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga102.fm.intel.com with ESMTP; 28 Sep 2016 19:48:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,413,1470726000"; d="scan'208,217";a="14456457" Received: from orsmsx107.amr.corp.intel.com ([10.22.240.5]) by orsmga005.jf.intel.com with ESMTP; 28 Sep 2016 19:48:24 -0700 Received: from orsmsx156.amr.corp.intel.com (10.22.240.22) by ORSMSX107.amr.corp.intel.com (10.22.240.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 28 Sep 2016 19:48:24 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.161]) by ORSMSX156.amr.corp.intel.com ([10.22.240.22]) with mapi id 14.03.0248.002; Wed, 28 Sep 2016 19:48:23 -0700 From: "Kinney, Michael D" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Carsey, Jaben" Thread-Topic: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk Thread-Index: AQHSGfUo6wo3bjCiE0S5dNNGBcQzzqCPuODwgAB9HID//42jsA== Date: Thu, 29 Sep 2016 02:48:23 +0000 Message-ID: References: <1475109500-13024-1-git-send-email-michael.d.kinney@intel.com> <734D49CCEBEEF84792F5B80ED585239D58E0CE24@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D58E0CFCF@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D58E0CFCF@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTdjYjEyNTMtY2ExZS00MWU1LTkxYmEtOGVhZjBiNjU3YTk0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InB2azVLNFdpNEllTEZcLzNzXC9ORkFtS0RmMUk0NmRaUElkS3VrckY0VXRGQT0ifQ== x-originating-ip: [10.22.254.138] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writing disk X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Sep 2016 02:48:25 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 ; edk2-devel@lists.01.org Cc: Carsey, Jaben 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 =3D FileTypeDiskBuffer; - DevicePath =3D gEfiShellProtocol->GetDevicePathFromMap(Devi= ceName); + DevicePath =3D (EFI_DEVICE_PATH_PROTOCOL *) gEfiShellProtoc= ol->GetDevicePathFromMap(DeviceName); if (DevicePath =3D=3D NULL) { // StatusBarSetStatusString (L"Cannot Find Device"); return EFI_INVALID_PARAMETER; } - DupDevicePath =3D DuplicateDevicePath(DevicePath); - // // get blkio interface // - Status =3D gBS->LocateDevicePath(&gEfiBlockIoProtocolGuid,&DupDevicePath= ,&Handle); - FreePool(DupDevicePath); + Status =3D gBS->LocateDevicePath(&gEfiBlockIoProtocolGuid,&DevicePath,&H= andle); 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 >; edk2-devel@l= ists.01.org; Kinney, Michael D > Cc: Carsey, Jaben > 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 >; edk2-devel@lists.01.org > Cc: Carsey, Jaben > > Subject: RE: [Patch] ShellPkg/Hexedit: Fix FreePool() ASSERT() when writi= ng 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 = >; Ni, Ruiyu > > >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: > > > >Cc: Jaben Carsey > > >Cc: Ruiyu Ni > > >Contributed-under: TianoCore Contribution Agreement 1.0 > >Signed-off-by: Michael Kinney > > >--- > > ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c | 4 +++= - > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > >diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskIma= ge.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 =3D DuplicateDevicePath(DevicePath); > >+ DupDevicePathForFree =3D DupDevicePath; > > > > // > > // get blkio interface > > // > > Status =3D gBS->LocateDevicePath(&gEfiBlockIoProtocolGuid,&DupDeviceP= ath,&Handle); > >- FreePool(DupDevicePath); > >+ FreePool(DupDevicePathForFree); > > if (EFI_ERROR (Status)) { > > // StatusBarSetStatusString (L"Read Disk Failed"); > > return Status; > >-- > >2.6.3.windows.1