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

[Michael Weghorn <m.weghorn@posteo.de>]

On 07/05/2020 22.09, Simon Marchi wrote:
> 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.
> 

As far as I understand, win32 should already be covered, so I have
switched that to using 'construct_inferior_arguments' as well in the
next version of the patch:
https://sourceware.org/pipermail/gdb-patches/2020-May/168350.html

I couldn't actually verify this myself, though.

Regarding nto, I have submitted a separate patch, since it seemed to me
that passing a string was a bit "strange" there anyway from what what I
understood:
https://sourceware.org/pipermail/gdb-patches/2020-May/168349.html

>> 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.
> 

Fixed in the new version.

>>  
>>      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)

Done in the new version.

> 
> 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);


I tried this, but as far as I understand (but I might have missed
something), this does not work here, since the gdb::array_view
constructor does not accept a const container (or reference to one) as a
parameter, so e.g.

    const std::vector<char *> vec;
    gdb::array_view<char *> view(vec);

does not work/compile, and program_args is a const reference here.
(Using a non-const vector does, but I'd personally rather not remove the
const qualifier for the 'program_args' parameter of the
'create_inferior' method.)

Michael