Please excuse the mistake, I suppose I was running a search for "FreePool", which of course did not find "SHELL_FREE_NON_NULL". Thanks for fixing it up!
Reviewed-by: Marvin Häuser <marvin.haeu...@outlook.com> -----Ursprüngliche Nachricht----- Von: Laszlo Ersek [mailto:ler...@redhat.com] Gesendet: Dienstag, 25. April 2017 15:57 An: edk2-devel-01 <edk2-devel@lists.01.org> Cc: Jaben Carsey <jaben.car...@intel.com>; Marvin Häuser <marvin.haeu...@outlook.com>; Qiu Shumin <shumin....@intel.com>; Ruiyu Ni <ruiyu...@intel.com> Betreff: [PATCH 2/2] ShellPkg/Shell: eliminate double-free in RunSplitCommand() Commit bd3fc8133b2b ("ShellPkg/App: Fix memory leak and save resources.", 2016-05-20) added a FreePool() call for Split->SplitStdIn, near end of the RunSplitCommand(), right after the same shell file was closed with CloseFile(). The argument was: > 1) RunSplitCommand() allocates the initial SplitStdOut via > CreateFileInterfaceMem(). Free SplitStdIn after the swap to fix > the memory leak. There is no memory leak actually, and the FreePool() call in question constitutes a double-free: (a) This is how the handle is established: ConvertEfiFileProtocolToShellHandle ( CreateFileInterfaceMem (Unicode), NULL ); CreateFileInterfaceMem() allocates an EFI_FILE_PROTOCOL_MEM object and populates it fully. ConvertEfiFileProtocolToShellHandle() allocates some administrative structures and links the EFI_FILE_PROTOCOL_MEM object into "mFileHandleList". (b) EFI_SHELL_PROTOCOL.CloseFile() is required to close the SHELL_FILE_HANDLE and to release all associated data. Accordingly, near the end of RunSplitCommand(), we have: EfiShellClose() ShellFileHandleRemove() // // undoes the effects of ConvertEfiFileProtocolToShellHandle() // ConvertShellHandleToEfiFileProtocol() // // note that this does not adjust the pointer value; it's a pure // type cast // FileHandleClose() FileInterfaceMemClose() // // tears down EFI_FILE_PROTOCOL_MEM completely, undoing the // effects of CreateFileInterfaceMem () // The FreePool() call added by bd3fc8133b2b conflicts with SHELL_FREE_NON_NULL(This); in FileInterfaceMemClose(), so remove it. This error can be reproduced for example with: > Shell> map | more > 'more' is not recognized as an internal or external command, operable > program, or script file. which triggers: > ASSERT MdeModulePkg/Core/Dxe/Mem/Pool.c(624): CR has Bad Signature with the following stack dump: > #0 0x000000007f6dc094 in CpuDeadLoop () at > MdePkg/Library/BaseLib/CpuDeadLoop.c:37 > #1 0x000000007f6dd1b4 in DebugAssert (FileName=0x7f6ed9f0 > "MdeModulePkg/Core/Dxe/Mem/Pool.c", LineNumber=624, > Description=0x7f6ed9d8 "CR has Bad Signature") at > OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c:153 > #2 0x000000007f6d075d in CoreFreePoolI (Buffer=0x7e232c98, > PoolType=0x7f6bc1c4) at MdeModulePkg/Core/Dxe/Mem/Pool.c:624 > #3 0x000000007f6d060e in CoreInternalFreePool (Buffer=0x7e232c98, > PoolType=0x7f6bc1c4) at MdeModulePkg/Core/Dxe/Mem/Pool.c:529 > #4 0x000000007f6d0648 in CoreFreePool (Buffer=0x7e232c98) at > MdeModulePkg/Core/Dxe/Mem/Pool.c:552 > #5 0x000000007d49fbf8 in FreePool (Buffer=0x7e232c98) at > MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c:818 > #6 0x000000007d4875c3 in RunSplitCommand (CmdLine=0x7d898398, > StdIn=0x0, StdOut=0x0) at ShellPkg/Application/Shell/Shell.c:1813 > #7 0x000000007d487d59 in ProcessNewSplitCommandLine > (CmdLine=0x7d898398) at ShellPkg/Application/Shell/Shell.c:2121 > #8 0x000000007d488937 in RunShellCommand (CmdLine=0x7e233018, > CommandStatus=0x0) at ShellPkg/Application/Shell/Shell.c:2670 > #9 0x000000007d488b0b in RunCommand (CmdLine=0x7e233018) at > ShellPkg/Application/Shell/Shell.c:2732 > #10 0x000000007d4867c8 in DoShellPrompt () at > ShellPkg/Application/Shell/Shell.c:1349 > #11 0x000000007d48524d in UefiMain (ImageHandle=0x7e24c898, > SystemTable=0x7f5b6018) at ShellPkg/Application/Shell/Shell.c:631 Cc: Jaben Carsey <jaben.car...@intel.com> Cc: Marvin Häuser <marvin.haeu...@outlook.com> Cc: Qiu Shumin <shumin....@intel.com> Cc: Ruiyu Ni <ruiyu...@intel.com> Fixes: bd3fc8133b2b17ad2e0427d1bf6b44b08cf2f3b2 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <ler...@redhat.com> --- ShellPkg/Application/Shell/Shell.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c index fa958170a1db..fcf47b5c733d 100644 --- a/ShellPkg/Application/Shell/Shell.c +++ b/ShellPkg/Application/Shell/Shell.c @@ -1810,7 +1810,6 @@ RunSplitCommand( } if (Split->SplitStdIn != NULL) { ShellInfoObject.NewEfiShellProtocol->CloseFile (Split->SplitStdIn); - FreePool (Split->SplitStdIn); } FreePool(Split); -- 2.9.3 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel