public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch] ShellPkg/Shell: Add double quotes to args with white space
@ 2017-01-07 20:26 Michael Kinney
  2017-01-09  6:17 ` Ni, Ruiyu
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Kinney @ 2017-01-07 20:26 UTC (permalink / raw)
  To: edk2-devel; +Cc: Jaben Carsey, Ruiyu Ni, Michael D Kinney

https://bugzilla.tianocore.org/show_bug.cgi?id=332

When the ShellLib ShellExecute() API or the Shell
Protocol Execute() API are used to execute a
command, the arguments are parsed to produce the
Argc/Argv list in the Shell Parameters Protocol and
double quotes are removed from arguments that are
surrounded by double quotes.  This is the required
behavior of the Shell Parameters Protocol.

The ProcessCommandLine() function in the shell
implementation uses the Argc/Argv list from the
Shell Parameters Protocol to assemble a new command
line, but the double quotes that may have been
originally present for an argument are not preserved.

ProcessCommandLine() is updated to check if an
argument added to the generated command line
contains one or more white space characters, and
if it does, double quotes are added around the
argument.

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 D Kinney <michael.d.kinney@intel.com>
---
 ShellPkg/Application/Shell/Shell.c | 64 +++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 8 deletions(-)

diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
index b39d81d..29e68db 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1042,11 +1042,31 @@ ProcessCommandLine(
         continue;
       }
 
-      ShellInfoObject.ShellInitSettings.FileName = AllocateCopyPool(StrSize(CurrentArg), CurrentArg);
+      ShellInfoObject.ShellInitSettings.FileName = NULL;
+      Size = 0;
+      //
+      // If first argument contains a space, then add double quotes before the argument
+      //
+      if (StrStr (CurrentArg, L" ") != NULL) {
+        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size, L"\"", 0);
+        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
+          return (EFI_OUT_OF_RESOURCES);
+        }
+      }
+      StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size, CurrentArg, 0);
       if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
         return (EFI_OUT_OF_RESOURCES);
       }
       //
+      // If first argument contains a space, then add double quotes after the argument
+      //
+      if (StrStr (CurrentArg, L" ") != NULL) {
+        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size, L"\"", 0);
+        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
+          return (EFI_OUT_OF_RESOURCES);
+        }
+      }
+      //
       // We found `file-name`.
       //
       ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoStartup = 1;
@@ -1055,13 +1075,28 @@ ProcessCommandLine(
       // Add `file-name-options`
       for (Size = 0 ; LoopVar < gEfiShellParametersProtocol->Argc ; LoopVar++) {
         ASSERT((ShellInfoObject.ShellInitSettings.FileOptions == NULL && Size == 0) || (ShellInfoObject.ShellInitSettings.FileOptions != NULL));
-        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
-                    &Size,
-                    L" ",
-                    0);
-        if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
-          SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
-          return (EFI_OUT_OF_RESOURCES);
+        //
+        // Add a space between arguments
+        //
+        if (ShellInfoObject.ShellInitSettings.FileOptions != NULL) {
+          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions, &Size, L" ", 0);
+          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
+            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
+            return (EFI_OUT_OF_RESOURCES);
+          }
+        }
+        //
+        // If an argumnent contains a space, then add double quotes before the argument
+        //
+        if (StrStr (gEfiShellParametersProtocol->Argv[LoopVar], L" ") != NULL) {
+          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
+                      &Size,
+                      L"\"",
+                      0);
+          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
+            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
+            return (EFI_OUT_OF_RESOURCES);
+          }
         }
         StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
                     &Size,
@@ -1071,6 +1106,19 @@ ProcessCommandLine(
           SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
           return (EFI_OUT_OF_RESOURCES);
         }
+        //
+        // If an argumnent contains a space, then add double quotes after the argument
+        //
+        if (StrStr (gEfiShellParametersProtocol->Argv[LoopVar], L" ") != NULL) {
+          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
+                      &Size,
+                      L"\"",
+                      0);
+          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
+            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
+            return (EFI_OUT_OF_RESOURCES);
+          }
+        }
       }
     }
   }
-- 
2.6.3.windows.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Patch] ShellPkg/Shell: Add double quotes to args with white space
  2017-01-07 20:26 [Patch] ShellPkg/Shell: Add double quotes to args with white space Michael Kinney
@ 2017-01-09  6:17 ` Ni, Ruiyu
  2017-01-10 21:09   ` Kinney, Michael D
  0 siblings, 1 reply; 4+ messages in thread
From: Ni, Ruiyu @ 2017-01-09  6:17 UTC (permalink / raw)
  To: Kinney, Michael D, edk2-devel@lists.01.org
  Cc: Carsey, Jaben, Kinney, Michael D

Mike,
According to the wording below copied from Shell spec, quotes are removed before saving
To Argv.
"Argv
Points to an Argc-element array of points to null-terminated strings
containing the command-line parameters. The first entry in the array is
always the full file path of the executable. Any quotation marks that were
used to preserve whitespace have been removed."

"Double-quotation marks that surround arguments are stripped before they are
passed to the entry point of a shell application. For more information, see the Argv
member of the EFI_SHELL_PARAMETERS_PROTOCOL."

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Michael Kinney
> Sent: Sunday, January 8, 2017 4:26 AM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [edk2] [Patch] ShellPkg/Shell: Add double quotes to args with white
> space
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=332
> 
> When the ShellLib ShellExecute() API or the Shell Protocol Execute() API are
> used to execute a command, the arguments are parsed to produce the
> Argc/Argv list in the Shell Parameters Protocol and double quotes are
> removed from arguments that are surrounded by double quotes.  This is the
> required behavior of the Shell Parameters Protocol.
> 
> The ProcessCommandLine() function in the shell implementation uses the
> Argc/Argv list from the Shell Parameters Protocol to assemble a new
> command line, but the double quotes that may have been originally present
> for an argument are not preserved.
> 
> ProcessCommandLine() is updated to check if an argument added to the
> generated command line contains one or more white space characters, and if
> it does, double quotes are added around the argument.
> 
> 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 D Kinney <michael.d.kinney@intel.com>
> ---
>  ShellPkg/Application/Shell/Shell.c | 64
> +++++++++++++++++++++++++++++++++-----
>  1 file changed, 56 insertions(+), 8 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/Shell.c
> b/ShellPkg/Application/Shell/Shell.c
> index b39d81d..29e68db 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -1042,11 +1042,31 @@ ProcessCommandLine(
>          continue;
>        }
> 
> -      ShellInfoObject.ShellInitSettings.FileName =
> AllocateCopyPool(StrSize(CurrentArg), CurrentArg);
> +      ShellInfoObject.ShellInitSettings.FileName = NULL;
> +      Size = 0;
> +      //
> +      // If first argument contains a space, then add double quotes before the
> argument
> +      //
> +      if (StrStr (CurrentArg, L" ") != NULL) {
> +        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size, L"\"",
> 0);
> +        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
> +          return (EFI_OUT_OF_RESOURCES);
> +        }
> +      }
> +      StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size,
> + CurrentArg, 0);
>        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
>          return (EFI_OUT_OF_RESOURCES);
>        }
>        //
> +      // If first argument contains a space, then add double quotes after the
> argument
> +      //
> +      if (StrStr (CurrentArg, L" ") != NULL) {
> +        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size, L"\"",
> 0);
> +        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
> +          return (EFI_OUT_OF_RESOURCES);
> +        }
> +      }
> +      //
>        // We found `file-name`.
>        //
>        ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoStartup = 1; @@ -
> 1055,13 +1075,28 @@ ProcessCommandLine(
>        // Add `file-name-options`
>        for (Size = 0 ; LoopVar < gEfiShellParametersProtocol->Argc ; LoopVar++) {
>          ASSERT((ShellInfoObject.ShellInitSettings.FileOptions == NULL && Size
> == 0) || (ShellInfoObject.ShellInitSettings.FileOptions != NULL));
> -        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> -                    &Size,
> -                    L" ",
> -                    0);
> -        if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> -          SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> -          return (EFI_OUT_OF_RESOURCES);
> +        //
> +        // Add a space between arguments
> +        //
> +        if (ShellInfoObject.ShellInitSettings.FileOptions != NULL) {
> +          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions, &Size, L"
> ", 0);
> +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> +            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> +            return (EFI_OUT_OF_RESOURCES);
> +          }
> +        }
> +        //
> +        // If an argumnent contains a space, then add double quotes before the
> argument
> +        //
> +        if (StrStr (gEfiShellParametersProtocol->Argv[LoopVar], L" ") != NULL) {
> +          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> +                      &Size,
> +                      L"\"",
> +                      0);
> +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> +            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> +            return (EFI_OUT_OF_RESOURCES);
> +          }
>          }
>          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
>                      &Size,
> @@ -1071,6 +1106,19 @@ ProcessCommandLine(
>            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
>            return (EFI_OUT_OF_RESOURCES);
>          }
> +        //
> +        // If an argumnent contains a space, then add double quotes after the
> argument
> +        //
> +        if (StrStr (gEfiShellParametersProtocol->Argv[LoopVar], L" ") != NULL) {
> +          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> +                      &Size,
> +                      L"\"",
> +                      0);
> +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> +            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> +            return (EFI_OUT_OF_RESOURCES);
> +          }
> +        }
>        }
>      }
>    }
> --
> 2.6.3.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Patch] ShellPkg/Shell: Add double quotes to args with white space
  2017-01-09  6:17 ` Ni, Ruiyu
@ 2017-01-10 21:09   ` Kinney, Michael D
  2017-01-11  4:58     ` Ni, Ruiyu
  0 siblings, 1 reply; 4+ messages in thread
From: Kinney, Michael D @ 2017-01-10 21:09 UTC (permalink / raw)
  To: Ni, Ruiyu, edk2-devel@lists.01.org, Kinney, Michael D; +Cc: Carsey, Jaben

Ray,

This change does not impact the Argc/Argv list from the Shell
Parameters Protocol.  The double quotes are always stripped
for those and the entry point to the shell app still sees the
same input as before.  This change conforms to the shell 
specification section you reference. 

This change applied to the case when the Argc/Argv list is 
used to generate a new command line string from the Argc/Argv
list provided by the Shell Parameters Protocol.  The command 
line string is then passed to a function that re-parses the 
command line arguments into the Argc/Argv list in the Shell
Parameters Protocol.

I am seeing a real failure case when the ShellExecute()API
is used to execute a command line.  The Bugzilla issue
provided below contains sample code that reproduces the
issue.  The sample code is a request to edit a file whose
filename contains a space character.  In order to pass that
filename to the edit command, the name of the file must be 
surrounded by double quotes.

  Status = ShellExecute (
             &ImageHandle,
             L"edit \"a b.txt\"",
             FALSE,
             NULL,
             &ExecuteStatus
             );

Before this change, the following command line is
generated, and the edit command fails with an error
that there are too many parameters.

	edit a b.txt

After this change, the correct command line is generated
and the edit command correctly opens the file "a b.txt".

	edit "a b.txt"

What is the specific concern you have with this patch?

Best regards,

Mike


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Sunday, January 8, 2017 10:17 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2] [Patch] ShellPkg/Shell: Add double quotes to args with white
> space
> 
> Mike,
> According to the wording below copied from Shell spec, quotes are removed before
> saving
> To Argv.
> "Argv
> Points to an Argc-element array of points to null-terminated strings
> containing the command-line parameters. The first entry in the array is
> always the full file path of the executable. Any quotation marks that were
> used to preserve whitespace have been removed."
> 
> "Double-quotation marks that surround arguments are stripped before they are
> passed to the entry point of a shell application. For more information, see the
> Argv
> member of the EFI_SHELL_PARAMETERS_PROTOCOL."
> 
> Thanks/Ray
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Michael Kinney
> > Sent: Sunday, January 8, 2017 4:26 AM
> > To: edk2-devel@lists.01.org
> > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> > Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: [edk2] [Patch] ShellPkg/Shell: Add double quotes to args with white
> > space
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=332
> >
> > When the ShellLib ShellExecute() API or the Shell Protocol Execute() API are
> > used to execute a command, the arguments are parsed to produce the
> > Argc/Argv list in the Shell Parameters Protocol and double quotes are
> > removed from arguments that are surrounded by double quotes.  This is the
> > required behavior of the Shell Parameters Protocol.
> >
> > The ProcessCommandLine() function in the shell implementation uses the
> > Argc/Argv list from the Shell Parameters Protocol to assemble a new
> > command line, but the double quotes that may have been originally present
> > for an argument are not preserved.
> >
> > ProcessCommandLine() is updated to check if an argument added to the
> > generated command line contains one or more white space characters, and if
> > it does, double quotes are added around the argument.
> >
> > 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 D Kinney <michael.d.kinney@intel.com>
> > ---
> >  ShellPkg/Application/Shell/Shell.c | 64
> > +++++++++++++++++++++++++++++++++-----
> >  1 file changed, 56 insertions(+), 8 deletions(-)
> >
> > diff --git a/ShellPkg/Application/Shell/Shell.c
> > b/ShellPkg/Application/Shell/Shell.c
> > index b39d81d..29e68db 100644
> > --- a/ShellPkg/Application/Shell/Shell.c
> > +++ b/ShellPkg/Application/Shell/Shell.c
> > @@ -1042,11 +1042,31 @@ ProcessCommandLine(
> >          continue;
> >        }
> >
> > -      ShellInfoObject.ShellInitSettings.FileName =
> > AllocateCopyPool(StrSize(CurrentArg), CurrentArg);
> > +      ShellInfoObject.ShellInitSettings.FileName = NULL;
> > +      Size = 0;
> > +      //
> > +      // If first argument contains a space, then add double quotes before the
> > argument
> > +      //
> > +      if (StrStr (CurrentArg, L" ") != NULL) {
> > +        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size, L"\"",
> > 0);
> > +        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
> > +          return (EFI_OUT_OF_RESOURCES);
> > +        }
> > +      }
> > +      StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size,
> > + CurrentArg, 0);
> >        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
> >          return (EFI_OUT_OF_RESOURCES);
> >        }
> >        //
> > +      // If first argument contains a space, then add double quotes after the
> > argument
> > +      //
> > +      if (StrStr (CurrentArg, L" ") != NULL) {
> > +        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName, &Size, L"\"",
> > 0);
> > +        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
> > +          return (EFI_OUT_OF_RESOURCES);
> > +        }
> > +      }
> > +      //
> >        // We found `file-name`.
> >        //
> >        ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoStartup = 1; @@ -
> > 1055,13 +1075,28 @@ ProcessCommandLine(
> >        // Add `file-name-options`
> >        for (Size = 0 ; LoopVar < gEfiShellParametersProtocol->Argc ; LoopVar++)
> {
> >          ASSERT((ShellInfoObject.ShellInitSettings.FileOptions == NULL && Size
> > == 0) || (ShellInfoObject.ShellInitSettings.FileOptions != NULL));
> > -        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> > -                    &Size,
> > -                    L" ",
> > -                    0);
> > -        if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> > -          SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > -          return (EFI_OUT_OF_RESOURCES);
> > +        //
> > +        // Add a space between arguments
> > +        //
> > +        if (ShellInfoObject.ShellInitSettings.FileOptions != NULL) {
> > +          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions, &Size,
> L"
> > ", 0);
> > +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> > +            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > +            return (EFI_OUT_OF_RESOURCES);
> > +          }
> > +        }
> > +        //
> > +        // If an argumnent contains a space, then add double quotes before the
> > argument
> > +        //
> > +        if (StrStr (gEfiShellParametersProtocol->Argv[LoopVar], L" ") != NULL)
> {
> > +          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> > +                      &Size,
> > +                      L"\"",
> > +                      0);
> > +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> > +            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > +            return (EFI_OUT_OF_RESOURCES);
> > +          }
> >          }
> >          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> >                      &Size,
> > @@ -1071,6 +1106,19 @@ ProcessCommandLine(
> >            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> >            return (EFI_OUT_OF_RESOURCES);
> >          }
> > +        //
> > +        // If an argumnent contains a space, then add double quotes after the
> > argument
> > +        //
> > +        if (StrStr (gEfiShellParametersProtocol->Argv[LoopVar], L" ") != NULL)
> {
> > +          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> > +                      &Size,
> > +                      L"\"",
> > +                      0);
> > +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> > +            SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > +            return (EFI_OUT_OF_RESOURCES);
> > +          }
> > +        }
> >        }
> >      }
> >    }
> > --
> > 2.6.3.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Patch] ShellPkg/Shell: Add double quotes to args with white space
  2017-01-10 21:09   ` Kinney, Michael D
@ 2017-01-11  4:58     ` Ni, Ruiyu
  0 siblings, 0 replies; 4+ messages in thread
From: Ni, Ruiyu @ 2017-01-11  4:58 UTC (permalink / raw)
  To: Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Carsey, Jaben

Mike,
I have no other concerns now.

Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Wednesday, January 11, 2017 5:10 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: [edk2] [Patch] ShellPkg/Shell: Add double quotes to args with
> white space
> 
> Ray,
> 
> This change does not impact the Argc/Argv list from the Shell Parameters
> Protocol.  The double quotes are always stripped for those and the entry
> point to the shell app still sees the same input as before.  This change
> conforms to the shell specification section you reference.
> 
> This change applied to the case when the Argc/Argv list is used to generate a
> new command line string from the Argc/Argv list provided by the Shell
> Parameters Protocol.  The command line string is then passed to a function
> that re-parses the command line arguments into the Argc/Argv list in the
> Shell Parameters Protocol.
> 
> I am seeing a real failure case when the ShellExecute()API is used to execute
> a command line.  The Bugzilla issue provided below contains sample code
> that reproduces the issue.  The sample code is a request to edit a file whose
> filename contains a space character.  In order to pass that filename to the
> edit command, the name of the file must be surrounded by double quotes.
> 
>   Status = ShellExecute (
>              &ImageHandle,
>              L"edit \"a b.txt\"",
>              FALSE,
>              NULL,
>              &ExecuteStatus
>              );
> 
> Before this change, the following command line is generated, and the edit
> command fails with an error that there are too many parameters.
> 
> 	edit a b.txt
> 
> After this change, the correct command line is generated and the edit
> command correctly opens the file "a b.txt".
> 
> 	edit "a b.txt"
> 
> What is the specific concern you have with this patch?
> 
> Best regards,
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Sunday, January 8, 2017 10:17 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > edk2-devel@lists.01.org
> > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Subject: RE: [edk2] [Patch] ShellPkg/Shell: Add double quotes to args
> > with white space
> >
> > Mike,
> > According to the wording below copied from Shell spec, quotes are
> > removed before saving To Argv.
> > "Argv
> > Points to an Argc-element array of points to null-terminated strings
> > containing the command-line parameters. The first entry in the array
> > is always the full file path of the executable. Any quotation marks
> > that were used to preserve whitespace have been removed."
> >
> > "Double-quotation marks that surround arguments are stripped before
> > they are passed to the entry point of a shell application. For more
> > information, see the Argv member of the
> > EFI_SHELL_PARAMETERS_PROTOCOL."
> >
> > Thanks/Ray
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Michael Kinney
> > > Sent: Sunday, January 8, 2017 4:26 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu
> > > <ruiyu.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Subject: [edk2] [Patch] ShellPkg/Shell: Add double quotes to args
> > > with white space
> > >
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=332
> > >
> > > When the ShellLib ShellExecute() API or the Shell Protocol Execute()
> > > API are used to execute a command, the arguments are parsed to
> > > produce the Argc/Argv list in the Shell Parameters Protocol and
> > > double quotes are removed from arguments that are surrounded by
> > > double quotes.  This is the required behavior of the Shell Parameters
> Protocol.
> > >
> > > The ProcessCommandLine() function in the shell implementation uses
> > > the Argc/Argv list from the Shell Parameters Protocol to assemble a
> > > new command line, but the double quotes that may have been
> > > originally present for an argument are not preserved.
> > >
> > > ProcessCommandLine() is updated to check if an argument added to the
> > > generated command line contains one or more white space characters,
> > > and if it does, double quotes are added around the argument.
> > >
> > > 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 D Kinney <michael.d.kinney@intel.com>
> > > ---
> > >  ShellPkg/Application/Shell/Shell.c | 64
> > > +++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 56 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/ShellPkg/Application/Shell/Shell.c
> > > b/ShellPkg/Application/Shell/Shell.c
> > > index b39d81d..29e68db 100644
> > > --- a/ShellPkg/Application/Shell/Shell.c
> > > +++ b/ShellPkg/Application/Shell/Shell.c
> > > @@ -1042,11 +1042,31 @@ ProcessCommandLine(
> > >          continue;
> > >        }
> > >
> > > -      ShellInfoObject.ShellInitSettings.FileName =
> > > AllocateCopyPool(StrSize(CurrentArg), CurrentArg);
> > > +      ShellInfoObject.ShellInitSettings.FileName = NULL;
> > > +      Size = 0;
> > > +      //
> > > +      // If first argument contains a space, then add double quotes
> > > + before the
> > > argument
> > > +      //
> > > +      if (StrStr (CurrentArg, L" ") != NULL) {
> > > +        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName,
> > > + &Size, L"\"",
> > > 0);
> > > +        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
> > > +          return (EFI_OUT_OF_RESOURCES);
> > > +        }
> > > +      }
> > > +      StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName,
> > > + &Size, CurrentArg, 0);
> > >        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
> > >          return (EFI_OUT_OF_RESOURCES);
> > >        }
> > >        //
> > > +      // If first argument contains a space, then add double quotes
> > > + after the
> > > argument
> > > +      //
> > > +      if (StrStr (CurrentArg, L" ") != NULL) {
> > > +        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileName,
> > > + &Size, L"\"",
> > > 0);
> > > +        if (ShellInfoObject.ShellInitSettings.FileName == NULL) {
> > > +          return (EFI_OUT_OF_RESOURCES);
> > > +        }
> > > +      }
> > > +      //
> > >        // We found `file-name`.
> > >        //
> > >        ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoStartup =
> > > 1; @@ -
> > > 1055,13 +1075,28 @@ ProcessCommandLine(
> > >        // Add `file-name-options`
> > >        for (Size = 0 ; LoopVar < gEfiShellParametersProtocol->Argc ;
> > > LoopVar++)
> > {
> > >          ASSERT((ShellInfoObject.ShellInitSettings.FileOptions ==
> > > NULL && Size == 0) || (ShellInfoObject.ShellInitSettings.FileOptions !=
> NULL));
> > > -        StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> > > -                    &Size,
> > > -                    L" ",
> > > -                    0);
> > > -        if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> > > -
> SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > > -          return (EFI_OUT_OF_RESOURCES);
> > > +        //
> > > +        // Add a space between arguments
> > > +        //
> > > +        if (ShellInfoObject.ShellInitSettings.FileOptions != NULL) {
> > > +
> > > + StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions, &Size,
> > L"
> > > ", 0);
> > > +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> > > +
> SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > > +            return (EFI_OUT_OF_RESOURCES);
> > > +          }
> > > +        }
> > > +        //
> > > +        // If an argumnent contains a space, then add double quotes
> > > + before the
> > > argument
> > > +        //
> > > +        if (StrStr (gEfiShellParametersProtocol->Argv[LoopVar], L"
> > > + ") != NULL)
> > {
> > > +          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> > > +                      &Size,
> > > +                      L"\"",
> > > +                      0);
> > > +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> > > +
> SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > > +            return (EFI_OUT_OF_RESOURCES);
> > > +          }
> > >          }
> > >          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> > >                      &Size,
> > > @@ -1071,6 +1106,19 @@ ProcessCommandLine(
> > >
> SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > >            return (EFI_OUT_OF_RESOURCES);
> > >          }
> > > +        //
> > > +        // If an argumnent contains a space, then add double quotes
> > > + after the
> > > argument
> > > +        //
> > > +        if (StrStr (gEfiShellParametersProtocol->Argv[LoopVar], L"
> > > + ") != NULL)
> > {
> > > +          StrnCatGrow(&ShellInfoObject.ShellInitSettings.FileOptions,
> > > +                      &Size,
> > > +                      L"\"",
> > > +                      0);
> > > +          if (ShellInfoObject.ShellInitSettings.FileOptions == NULL) {
> > > +
> SHELL_FREE_NON_NULL(ShellInfoObject.ShellInitSettings.FileName);
> > > +            return (EFI_OUT_OF_RESOURCES);
> > > +          }
> > > +        }
> > >        }
> > >      }
> > >    }
> > > --
> > > 2.6.3.windows.1
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-01-11  4:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-07 20:26 [Patch] ShellPkg/Shell: Add double quotes to args with white space Michael Kinney
2017-01-09  6:17 ` Ni, Ruiyu
2017-01-10 21:09   ` Kinney, Michael D
2017-01-11  4:58     ` Ni, Ruiyu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox