* [PATCH 1/2] ShellPkg/Shell: clean up bogus member types in SPLIT_LIST
2017-04-25 13:57 [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing Laszlo Ersek
@ 2017-04-25 13:57 ` Laszlo Ersek
2017-04-25 13:57 ` [PATCH 2/2] ShellPkg/Shell: eliminate double-free in RunSplitCommand() Laszlo Ersek
2017-04-25 15:28 ` [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing Carsey, Jaben
2 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2017-04-25 13:57 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jaben Carsey, Marvin Häuser, Qiu Shumin, Ruiyu Ni
The "SPLIT_LIST.SplitStdOut" and "SPLIT_LIST.SplitStdIn" members currently
have type (SHELL_FILE_HANDLE *). This is wrong; SHELL_FILE_HANDLE is
already a pointer, there's no need to store a pointer to a pointer.
The error is obvious if we check where and how these members are used:
- In the RunSplitCommand() function, these members are used (populated)
extensively; this function has to be updated in sync.
ConvertEfiFileProtocolToShellHandle() already returns the temporary
memory file created with CreateFileInterfaceMem() as SHELL_FILE_HANDLE,
not as (SHELL_FILE_HANDLE *).
- In particular, the ConvertShellHandleToEfiFileProtocol() calls need to
be dropped as well in RunSplitCommand(), since
EFI_SHELL_PROTOCOL.SetFilePosition() and EFI_SHELL_PROTOCOL.CloseFile()
take SHELL_FILE_HANDLE parameters, not (EFI_FILE_PROTOCOL *).
Given that ConvertShellHandleToEfiFileProtocol() only performs a
type-cast (it does not adjust any pointer values), *and*
SHELL_FILE_HANDLE -- taken by EFI_SHELL_PROTOCOL member functions -- is
actually a typedef to (VOID *) -- see more on this later --, this
conversion error hasn't been caught by compilers.
- In the ProcessNewSplitCommandLine() function, RunSplitCommand() is
called either initially (passing in NULL / NULL; no update needed), or
recursively (passing in Split->SplitStdIn / Split->SplitStdOut; again no
update is necessary beyond the RunSplitCommand() modification above).
- In the UpdateStdInStdOutStdErr() and RestoreStdInStdOutStdErr()
functions, said structure members are compared and assigned to
"EFI_SHELL_PARAMETERS_PROTOCOL.StdIn" and
"EFI_SHELL_PARAMETERS_PROTOCOL.StdOut", both of which have type
SHELL_FILE_HANDLE, *not* (SHELL_FILE_HANDLE *).
The compiler hasn't caught this error because of the fatally flawed type
definition of SHELL_FILE_HANDLE, namely
typedef VOID *SHELL_FILE_HANDLE;
Pointer-to-void silently converts to and from most other pointer types;
among them, pointer-to-pointer-to-void. That is also why no update is
necessary for UpdateStdInStdOutStdErr() and RestoreStdInStdOutStdErr()
in this fix.
(
Generally speaking, using (VOID *) typedefs for opaque handles is a tragic
mistake in all of the UEFI-related specifications; this practice defeats
any type checking that compilers might help programmers with. The right
way to define an opaque handle is as follows:
//
// Introduce the incomplete structure type, and the derived pointer
// type, in both the specification and the public edk2 headers. Note
// that the derived pointer type itself is a complete type, and it can
// be used freely by client code.
//
typedef struct SHELL_FILE *SHELL_FILE_HANDLE;
//
// Complete the structure type in the edk2 internal C source files.
//
struct SHELL_FILE {
//
// list fields
//
};
This way the structure size and members remain hidden from client code,
but the C compiler can nonetheless catch any invalid conversions between
incompatible XXX_HANDLE types.
)
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
Cc: Qiu Shumin <shumin.qiu@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ShellPkg/Application/Shell/Shell.h | 4 ++--
ShellPkg/Application/Shell/Shell.c | 12 ++++++------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/ShellPkg/Application/Shell/Shell.h b/ShellPkg/Application/Shell/Shell.h
index 25ac114da71d..ff0849446817 100644
--- a/ShellPkg/Application/Shell/Shell.h
+++ b/ShellPkg/Application/Shell/Shell.h
@@ -63,8 +63,8 @@ extern CONST CHAR16 mNoNestingFalse[];
typedef struct {
LIST_ENTRY Link; ///< Standard linked list handler.
- SHELL_FILE_HANDLE *SplitStdOut; ///< ConsoleOut for use in the split.
- SHELL_FILE_HANDLE *SplitStdIn; ///< ConsoleIn for use in the split.
+ SHELL_FILE_HANDLE SplitStdOut; ///< ConsoleOut for use in the split.
+ SHELL_FILE_HANDLE SplitStdIn; ///< ConsoleIn for use in the split.
} SPLIT_LIST;
typedef struct {
diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
index 4383298aab72..fa958170a1db 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1714,8 +1714,8 @@ ShellConvertVariables (
EFI_STATUS
RunSplitCommand(
IN CONST CHAR16 *CmdLine,
- IN SHELL_FILE_HANDLE *StdIn,
- IN SHELL_FILE_HANDLE *StdOut
+ IN SHELL_FILE_HANDLE StdIn,
+ IN SHELL_FILE_HANDLE StdOut
)
{
EFI_STATUS Status;
@@ -1724,7 +1724,7 @@ RunSplitCommand(
UINTN Size1;
UINTN Size2;
SPLIT_LIST *Split;
- SHELL_FILE_HANDLE *TempFileHandle;
+ SHELL_FILE_HANDLE TempFileHandle;
BOOLEAN Unicode;
ASSERT(StdOut == NULL);
@@ -1790,7 +1790,7 @@ RunSplitCommand(
Split->SplitStdOut = Split->SplitStdIn;
}
Split->SplitStdIn = TempFileHandle;
- ShellInfoObject.NewEfiShellProtocol->SetFilePosition(ConvertShellHandleToEfiFileProtocol(Split->SplitStdIn), 0);
+ ShellInfoObject.NewEfiShellProtocol->SetFilePosition (Split->SplitStdIn, 0);
if (!EFI_ERROR(Status)) {
Status = RunCommand(NextCommandLine);
@@ -1806,10 +1806,10 @@ RunSplitCommand(
// Note that the original StdIn is now the StdOut...
//
if (Split->SplitStdOut != NULL) {
- ShellInfoObject.NewEfiShellProtocol->CloseFile(ConvertShellHandleToEfiFileProtocol(Split->SplitStdOut));
+ ShellInfoObject.NewEfiShellProtocol->CloseFile (Split->SplitStdOut);
}
if (Split->SplitStdIn != NULL) {
- ShellInfoObject.NewEfiShellProtocol->CloseFile(ConvertShellHandleToEfiFileProtocol(Split->SplitStdIn));
+ ShellInfoObject.NewEfiShellProtocol->CloseFile (Split->SplitStdIn);
FreePool (Split->SplitStdIn);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] ShellPkg/Shell: eliminate double-free in RunSplitCommand()
2017-04-25 13:57 [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing Laszlo Ersek
2017-04-25 13:57 ` [PATCH 1/2] ShellPkg/Shell: clean up bogus member types in SPLIT_LIST Laszlo Ersek
@ 2017-04-25 13:57 ` Laszlo Ersek
2017-04-25 15:28 ` [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing Carsey, Jaben
2 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2017-04-25 13:57 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jaben Carsey, Marvin Häuser, Qiu Shumin, Ruiyu Ni
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.carsey@intel.com>
Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
Cc: Qiu Shumin <shumin.qiu@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Fixes: bd3fc8133b2b17ad2e0427d1bf6b44b08cf2f3b2
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing
2017-04-25 13:57 [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing Laszlo Ersek
2017-04-25 13:57 ` [PATCH 1/2] ShellPkg/Shell: clean up bogus member types in SPLIT_LIST Laszlo Ersek
2017-04-25 13:57 ` [PATCH 2/2] ShellPkg/Shell: eliminate double-free in RunSplitCommand() Laszlo Ersek
@ 2017-04-25 15:28 ` Carsey, Jaben
2017-04-26 10:17 ` Laszlo Ersek
2 siblings, 1 reply; 5+ messages in thread
From: Carsey, Jaben @ 2017-04-25 15:28 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Marvin Häuser, Qiu Shumin, Ni, Ruiyu
I am good with the series. Good research and description!
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 25, 2017 6:57 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Marvin Häuser
> <Marvin.Haeuser@outlook.com>; Qiu Shumin <shumin.qiu@intel.com>; Ni,
> Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing
> Importance: High
>
> The first patch cleans up a bit of mess around the SPLIT_LIST structure.
> This is unrelated to the actual bug being fixed in the series, it just
> prepares the grounds.
>
> The second patch fixes the bug.
>
> Repo: https://github.com/lersek/edk2.git
> Branch: shell_split
>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Cc: Qiu Shumin <shumin.qiu@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (2):
> ShellPkg/Shell: clean up bogus member types in SPLIT_LIST
> ShellPkg/Shell: eliminate double-free in RunSplitCommand()
>
> ShellPkg/Application/Shell/Shell.h | 4 ++--
> ShellPkg/Application/Shell/Shell.c | 13 ++++++-------
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> --
> 2.9.3
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing
2017-04-25 15:28 ` [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing Carsey, Jaben
@ 2017-04-26 10:17 ` Laszlo Ersek
0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2017-04-26 10:17 UTC (permalink / raw)
To: Carsey, Jaben, Marvin Häuser; +Cc: edk2-devel-01, Ni, Ruiyu, Qiu Shumin
On 04/25/17 17:28, Carsey, Jaben wrote:
> I am good with the series. Good research and description!
>
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
Thank you Jaben and Marvin for the prompt reviews!
Series pushed as 6bbd4a8f5f2a..227fe49d5d4f.
Cheers,
Laszlo
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, April 25, 2017 6:57 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Marvin Häuser
>> <Marvin.Haeuser@outlook.com>; Qiu Shumin <shumin.qiu@intel.com>; Ni,
>> Ruiyu <ruiyu.ni@intel.com>
>> Subject: [PATCH 0/2] ShellPkg/Shell: fix double free in pipeline processing
>> Importance: High
>>
>> The first patch cleans up a bit of mess around the SPLIT_LIST structure.
>> This is unrelated to the actual bug being fixed in the series, it just
>> prepares the grounds.
>>
>> The second patch fixes the bug.
>>
>> Repo: https://github.com/lersek/edk2.git
>> Branch: shell_split
>>
>> Cc: Jaben Carsey <jaben.carsey@intel.com>
>> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
>> Cc: Qiu Shumin <shumin.qiu@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (2):
>> ShellPkg/Shell: clean up bogus member types in SPLIT_LIST
>> ShellPkg/Shell: eliminate double-free in RunSplitCommand()
>>
>> ShellPkg/Application/Shell/Shell.h | 4 ++--
>> ShellPkg/Application/Shell/Shell.c | 13 ++++++-------
>> 2 files changed, 8 insertions(+), 9 deletions(-)
>>
>> --
>> 2.9.3
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 5+ messages in thread