Re: [Patch] ShellPkg/Shell: Add double quotes to args with white space

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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