[PATCH 1/1] ShellPkg/CommandLib: avoid NULL derefence and memory leak

[PATCH 1/1] ShellPkg/CommandLib: avoid NULL derefence and memory leak

[Heinrich Schuchardt]

Since TianoCore EDK2 commit d65f2cea36d1 ("ShellPkg/CommandLib: Locate
proper UnicodeCollation instance") in edk2 the UEFI Shell crashes if EFI
variable PlatformLang is not defined due to dereferencing gUnicodeCollation
gUnicodeCollation (= NULL) in ShellCommandRegisterCommandName().

Furthermore CommandInit() is leaking PlatformLang if
gUnicodeCollation != NULL.

Close the memory leak and use the first UnicodeCollation instance if
PlatfomLang is not defined.

Fixes: d65f2cea36d1 ("ShellPkg/CommandLib: Locate proper UnicodeCollation
instance")
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 .../UefiShellCommandLib/UefiShellCommandLib.c | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
index ddc4bb1567..e60279e5ac 100644
--- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
+++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
@@ -80,12 +80,10 @@ CommandInit(
   EFI_STATUS                      Status;
   CHAR8                           *PlatformLang;

-  GetEfiGlobalVariable2 (EFI_PLATFORM_LANG_VARIABLE_NAME, (VOID**)&PlatformLang, NULL);
-  if (PlatformLang == NULL) {
-    return EFI_UNSUPPORTED;
-  }
-
   if (gUnicodeCollation == NULL) {
+
+    GetEfiGlobalVariable2 (EFI_PLATFORM_LANG_VARIABLE_NAME, (VOID**)&PlatformLang, NULL);
+
     Status = gBS->LocateHandleBuffer (
                     ByProtocol,
                     &gEfiUnicodeCollation2ProtocolGuid,
@@ -113,6 +111,14 @@ CommandInit(
         continue;
       }

+      //
+      // Without clue provided use the first Unicode Collation2 protocol.
+      //
+      if (PlatformLang == NULL) {
+        gUnicodeCollation = Uc;
+        break;
+      }
+
       //
       // Find the best matching matching language from the supported languages
       // of Unicode Collation2 protocol.
@@ -132,7 +138,9 @@ CommandInit(
     if (Handles != NULL) {
       FreePool (Handles);
     }
-    FreePool (PlatformLang);
+    if (PlatformLang != NULL) {
+      FreePool (PlatformLang);
+    }
   }

   return (gUnicodeCollation == NULL) ? EFI_UNSUPPORTED : EFI_SUCCESS;
--
2.20.1

Re: [edk2-devel] [PATCH 1/1] ShellPkg/CommandLib: avoid NULL derefence and memory leak

[Gao, Zhichao]

The patch code looks good to me.
But while I apply this patch, it will shows "error: corrupt patch at line 24" and " error: could not build fake ancestor". Maybe you direct change the patch file and remove some blank lines.
For Uefi BdsDxe driver, the variable "PlatfomLang" would be initialize all the time. But maybe other manufacturers may have their own Bds solution and do not set this variable. This patch makes the Shell app more compatible.

Without the patch warning.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Heinrich Schuchardt
> Sent: Friday, April 5, 2019 9:29 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Cc: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Heinrich Schuchardt
> <xypron.glpk@gmx.de>
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg/CommandLib: avoid NULL
> derefence and memory leak
> 
> Since TianoCore EDK2 commit d65f2cea36d1 ("ShellPkg/CommandLib: Locate
> proper UnicodeCollation instance") in edk2 the UEFI Shell crashes if EFI
> variable PlatformLang is not defined due to dereferencing gUnicodeCollation
> gUnicodeCollation (= NULL) in ShellCommandRegisterCommandName().
> 
> Furthermore CommandInit() is leaking PlatformLang if gUnicodeCollation !=
> NULL.
> 
> Close the memory leak and use the first UnicodeCollation instance if
> PlatfomLang is not defined.
> 
> Fixes: d65f2cea36d1 ("ShellPkg/CommandLib: Locate proper
> UnicodeCollation
> instance")
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  .../UefiShellCommandLib/UefiShellCommandLib.c | 20 +++++++++++++-----
> -
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
> b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
> index ddc4bb1567..e60279e5ac 100644
> --- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
> +++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
> @@ -80,12 +80,10 @@ CommandInit(
>    EFI_STATUS                      Status;
>    CHAR8                           *PlatformLang;
> 
> -  GetEfiGlobalVariable2 (EFI_PLATFORM_LANG_VARIABLE_NAME,
> (VOID**)&PlatformLang, NULL);
> -  if (PlatformLang == NULL) {
> -    return EFI_UNSUPPORTED;
> -  }
> -
>    if (gUnicodeCollation == NULL) {
> +
> +    GetEfiGlobalVariable2 (EFI_PLATFORM_LANG_VARIABLE_NAME,
> + (VOID**)&PlatformLang, NULL);
> +
>      Status = gBS->LocateHandleBuffer (
>                      ByProtocol,
>                      &gEfiUnicodeCollation2ProtocolGuid,
> @@ -113,6 +111,14 @@ CommandInit(
>          continue;
>        }
> 
> +      //
> +      // Without clue provided use the first Unicode Collation2 protocol.
> +      //
> +      if (PlatformLang == NULL) {
> +        gUnicodeCollation = Uc;
> +        break;
> +      }
> +
>        //
>        // Find the best matching matching language from the supported
> languages
>        // of Unicode Collation2 protocol.
> @@ -132,7 +138,9 @@ CommandInit(
>      if (Handles != NULL) {
>        FreePool (Handles);
>      }
> -    FreePool (PlatformLang);
> +    if (PlatformLang != NULL) {
> +      FreePool (PlatformLang);
> +    }
>    }
> 
>    return (gUnicodeCollation == NULL) ? EFI_UNSUPPORTED : EFI_SUCCESS;
> --
> 2.20.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#38597): https://edk2.groups.io/g/devel/message/38597
> Mute This Topic: https://groups.io/mt/30920004/1768756
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [zhichao.gao@intel.com] -=-=-=-=-=-=

Re: [edk2-devel] [PATCH 1/1] ShellPkg/CommandLib: avoid NULL derefence and memory leak

[Heinrich Schuchardt]

On 4/9/19 7:33 AM, Gao, Zhichao wrote:
> The patch code looks good to me.
> But while I apply this patch, it will shows "error: corrupt patch at line 24" and " error: could not build fake ancestor". Maybe you direct change the patch file and remove some blank lines.
> For Uefi BdsDxe driver, the variable "PlatfomLang" would be initialize all the time. But maybe other manufacturers may have their own Bds solution and do not set this variable. This patch makes the Shell app more compatible.
>
> Without the patch warning.
> Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks for reviewing.

The firmware that gave me the problem was U-Boot. In v2019.07 of U-Boot
PlatformLang will be set.

I am using Linux. The checked out EDK code has Windows line endings. Git
seems not to work correctly with it.

I will try to recreate the patch on a Windows system and resend it.

Best regards

Heinrich

>
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Heinrich Schuchardt
>> Sent: Friday, April 5, 2019 9:29 AM
>> To: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
>> Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> Cc: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Heinrich Schuchardt
>> <xypron.glpk@gmx.de>
>> Subject: [edk2-devel] [PATCH 1/1] ShellPkg/CommandLib: avoid NULL
>> derefence and memory leak
>>
>> Since TianoCore EDK2 commit d65f2cea36d1 ("ShellPkg/CommandLib: Locate
>> proper UnicodeCollation instance") in edk2 the UEFI Shell crashes if EFI
>> variable PlatformLang is not defined due to dereferencing gUnicodeCollation
>> gUnicodeCollation (= NULL) in ShellCommandRegisterCommandName().
>>
>> Furthermore CommandInit() is leaking PlatformLang if gUnicodeCollation !=
>> NULL.
>>
>> Close the memory leak and use the first UnicodeCollation instance if
>> PlatfomLang is not defined.
>>
>> Fixes: d65f2cea36d1 ("ShellPkg/CommandLib: Locate proper
>> UnicodeCollation
>> instance")
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  .../UefiShellCommandLib/UefiShellCommandLib.c | 20 +++++++++++++-----
>> -
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
>> b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
>> index ddc4bb1567..e60279e5ac 100644
>> --- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
>> +++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
>> @@ -80,12 +80,10 @@ CommandInit(
>>    EFI_STATUS                      Status;
>>    CHAR8                           *PlatformLang;
>>
>> -  GetEfiGlobalVariable2 (EFI_PLATFORM_LANG_VARIABLE_NAME,
>> (VOID**)&PlatformLang, NULL);
>> -  if (PlatformLang == NULL) {
>> -    return EFI_UNSUPPORTED;
>> -  }
>> -
>>    if (gUnicodeCollation == NULL) {
>> +
>> +    GetEfiGlobalVariable2 (EFI_PLATFORM_LANG_VARIABLE_NAME,
>> + (VOID**)&PlatformLang, NULL);
>> +
>>      Status = gBS->LocateHandleBuffer (
>>                      ByProtocol,
>>                      &gEfiUnicodeCollation2ProtocolGuid,
>> @@ -113,6 +111,14 @@ CommandInit(
>>          continue;
>>        }
>>
>> +      //
>> +      // Without clue provided use the first Unicode Collation2 protocol.
>> +      //
>> +      if (PlatformLang == NULL) {
>> +        gUnicodeCollation = Uc;
>> +        break;
>> +      }
>> +
>>        //
>>        // Find the best matching matching language from the supported
>> languages
>>        // of Unicode Collation2 protocol.
>> @@ -132,7 +138,9 @@ CommandInit(
>>      if (Handles != NULL) {
>>        FreePool (Handles);
>>      }
>> -    FreePool (PlatformLang);
>> +    if (PlatformLang != NULL) {
>> +      FreePool (PlatformLang);
>> +    }
>>    }
>>
>>    return (gUnicodeCollation == NULL) ? EFI_UNSUPPORTED : EFI_SUCCESS;
>> --
>> 2.20.1
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#38597): https://edk2.groups.io/g/devel/message/38597
>> Mute This Topic: https://groups.io/mt/30920004/1768756
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>> [zhichao.gao@intel.com] -=-=-=-=-=-=
>
>

Re: [edk2-devel] [PATCH 1/1] ShellPkg/CommandLib: avoid NULL derefence and memory leak

[Heinrich Schuchardt]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

On 4/9/19 7:33 AM, Gao, Zhichao wrote:
> The patch code looks good to me.
> But while I apply this patch, it will shows "error: corrupt patch at line 24" and " error: could not build fake ancestor". Maybe you direct change the patch file and remove some blank lines.
> For Uefi BdsDxe driver, the variable "PlatfomLang" would be initialize all the time. But maybe other manufacturers may have their own Bds solution and do not set this variable. This patch makes the Shell app more compatible.
>
> Without the patch warning.
> Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Hello Zhichao,

I am not able to reproduce the problem. I resend you the patch as
attachment.

When applying I had to use --keep-cr:

git am --keep-cr \
../patch/0001-ShellPkg-CommandLib-avoid-NULL-derefence-and-memory-.patch

I have found no way neither on Windows nor on Linux to create a patch
that applies without --keep-cr.

If you still have a problem, please, send me your global and local
gitconfig files, your .gitattributes and indicate on which operating
system you are working with which git version.

Best regards

Heinrich

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ShellPkg-CommandLib-avoid-NULL-derefence-and-memory-.patch --]
[-- Type: text/x-diff; name="0001-ShellPkg-CommandLib-avoid-NULL-derefence-and-memory-.patch", Size: 2580 bytes --]

From 9478bbcd1d8a03762924a0b4533c1b84cbfcaa57 Mon Sep 17 00:00:00 2001
From: Heinrich Schuchardt <xypron.glpk@gmx.de>
Date: Fri, 5 Apr 2019 03:06:18 +0200
Subject: [PATCH 1/1] ShellPkg/CommandLib: avoid NULL derefence and memory leak

Since TianoCore EDK2 commit d65f2cea36d1 ("ShellPkg/CommandLib: Locate
proper UnicodeCollation instance") in edk2 the UEFI Shell crashes if EFI
variable PlatformLang is not defined due to dereferencing gUnicodeCollation
gUnicodeCollation (= NULL) in ShellCommandRegisterCommandName().

Furthermore CommandInit() is leaking PlatformLang if
gUnicodeCollation != NULL.

Close the memory leak and use the first UnicodeCollation instance if
PlatfomLang is not defined.

Fixes: d65f2cea36d1 ("ShellPkg/CommandLib: Locate proper UnicodeCollation
instance")
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 .../UefiShellCommandLib/UefiShellCommandLib.c | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
index ddc4bb1567..e60279e5ac 100644
--- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
+++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
@@ -80,12 +80,10 @@ CommandInit(
   EFI_STATUS                      Status;
   CHAR8                           *PlatformLang;
 
-  GetEfiGlobalVariable2 (EFI_PLATFORM_LANG_VARIABLE_NAME, (VOID**)&PlatformLang, NULL);
-  if (PlatformLang == NULL) {
-    return EFI_UNSUPPORTED;
-  }
-
   if (gUnicodeCollation == NULL) {
+
+    GetEfiGlobalVariable2 (EFI_PLATFORM_LANG_VARIABLE_NAME, (VOID**)&PlatformLang, NULL);
+
     Status = gBS->LocateHandleBuffer (
                     ByProtocol,
                     &gEfiUnicodeCollation2ProtocolGuid,
@@ -113,6 +111,14 @@ CommandInit(
         continue;
       }
 
+      //
+      // Without clue provided use the first Unicode Collation2 protocol.
+      //
+      if (PlatformLang == NULL) {
+        gUnicodeCollation = Uc;
+        break;
+      }
+
       //
       // Find the best matching matching language from the supported languages
       // of Unicode Collation2 protocol.
@@ -132,7 +138,9 @@ CommandInit(
     if (Handles != NULL) {
       FreePool (Handles);
     }
-    FreePool (PlatformLang);
+    if (PlatformLang != NULL) {
+      FreePool (PlatformLang);
+    }
   }
 
   return (gUnicodeCollation == NULL) ? EFI_UNSUPPORTED : EFI_SUCCESS;
-- 
2.20.1

Re: [edk2-devel] [PATCH 1/1] ShellPkg/CommandLib: avoid NULL derefence and memory leak

[Gao, Zhichao]

No problem with the attachment.
I usually use the command "git am --3way --ignore-space-change --keep-cr *.patch". That make the same meaning with your 'am' options if the patch is ok.
Compare with your attachment and the patch extract form the email, seems some blank lines is missing.

Thanks,
Zhichao

> -----Original Message-----
> From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de]
> Sent: Wednesday, April 10, 2019 12:37 AM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Carsey,
> Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; Leif Lindholm
> <leif.lindholm@linaro.org>
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg/CommandLib: avoid NULL
> derefence and memory leak
> 
> On 4/9/19 7:33 AM, Gao, Zhichao wrote:
> > The patch code looks good to me.
> > But while I apply this patch, it will shows "error: corrupt patch at line 24"
> and " error: could not build fake ancestor". Maybe you direct change the
> patch file and remove some blank lines.
> > For Uefi BdsDxe driver, the variable "PlatfomLang" would be initialize all the
> time. But maybe other manufacturers may have their own Bds solution and
> do not set this variable. This patch makes the Shell app more compatible.
> >
> > Without the patch warning.
> > Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
> 
> Hello Zhichao,
> 
> I am not able to reproduce the problem. I resend you the patch as
> attachment.
> 
> When applying I had to use --keep-cr:
> 
> git am --keep-cr \
> ../patch/0001-ShellPkg-CommandLib-avoid-NULL-derefence-and-memory-
> .patch
> 
> I have found no way neither on Windows nor on Linux to create a patch that
> applies without --keep-cr.
> 
> If you still have a problem, please, send me your global and local gitconfig
> files, your .gitattributes and indicate on which operating system you are
> working with which git version.
> 
> Best regards
> 
> Heinrich