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

Michael Weghorn
2020-05-12T16:07:35Z

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