Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Simon Marchi <simark@simark.ca>]

On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
> Use the construct_inferior_arguments function instead of
> stringify_argv to construct a string from the program
> arguments in those places where that one is then passed
> to fork_inferior, since construct_inferior_arguments
> properly takes care of special characters, while
> stringify_argv does not.
> Using construct_inferior_arguments seems "natural", since its
> documentation also mentions that it "does the
> same shell processing as fork_inferior".
> 
> This makes gdbserver properly handle program args containing special
> characters, e.g. (example from PR25893)
> 
>   $ gdbserver localhost:50505 myprogram "hello world"
> 
> now properly handles "hello world" as a single arg, not two separate
> ones ("hello", "world").
> 
> I'm not sure regarding the two remaining uses of function stringify_argv
> (in win32_process_target::create_inferior and
> nto_process_target::create_inferior) whose result is not passed to
> fork_inferior and don't know how the arg processing in the creation
> of processes on those targets exactly works, so left them unchanged
> for now.

I'd say, do the change the best you can for win32 and nto.  We'll be
able to test win32.  Regarding nto, it hasn't seen patches for years,
it most likely doesn't even build.  And the toolchain not being easily
available, we can't do much about it.  I think we should actually
consider removing this port, unless somebody steps up to maintain it.
Same for lynx.

This will allow removing stringify_argv, and avoid having duplicate
broken functionality of construct_inferior_arguments.

> 2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         PR gdbserver/25893
>         * linux-low.cc (linux_process_target::create_inferior),
>         lynx-low.cc (lynx_process_target::create_inferior): Use
>         construct_inferior_arguments instead of stringify_argv
>         to get string representation which properly escapes
>         special characters.
> ---
>  gdbserver/linux-low.cc | 2 +-
>  gdbserver/lynx-low.cc  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 3cd8d5594d..3f3ffc6c43 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -984,7 +984,7 @@ linux_process_target::create_inferior (const char *program,
>    {
>      maybe_disable_address_space_randomization restore_personality
>        (cs.disable_randomization);
> -    std::string str_program_args = stringify_argv (program_args);
> +    std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());

This line is too long, see below.

>  
>      pid = fork_inferior (program,
>  			 str_program_args.c_str (),
> diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc
> index 9aa140c129..61f692f0c0 100644
> --- a/gdbserver/lynx-low.cc
> +++ b/gdbserver/lynx-low.cc
> @@ -253,7 +253,7 @@ lynx_process_target::create_inferior (const char *program,
>  				      const std::vector<char *> &program_args)
>  {
>    int pid;
> -  std::string str_program_args = stringify_argv (program_args);
> +  std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());;

Extra semi-colon, and spaces before parenthesis.  The line should fit in 80 columns, here's how
we'd typically break it:

  std::string str_program_args
    = construct_inferior_arguments (program_args.size (), program_args.data ());

(with the equal sign on the second line, I know that may seem odd)

If you are game, you could make another preparatory patch (anywhere in the series
before this one) that would make construct_inferior_arguments take a
gdb::array_view<char *> parameter.  That would allow you to call it more simply
here:

  construct_inferior_arguments (program_args);

Simon