[v3,5/6,PR,gdbserver/25893] : Use construct_inferior_arguments which handles special chars

Message ID 20200512154211.1311364-5-m.weghorn@posteo.de
State Superseded
Headers show
Series
  • [v3,1/6] gdb: Move construct_inferior_arguments to gdbsupport
Related show

Commit Message

Kevin Buettner via Gdb-patches May 12, 2020, 3:42 p.m.
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 (linux-low, lyn-low), 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".

Since construct_inferior_args has been extended to do
proper quoting for Windows shells in commit
5d60742e2dd3c9b475dce54b56043a358751bbb8
("Fix quoting of special characters for the MinGW build.",
2012-06-12), use it for the Windows case as well.
(I could not test that case myself, though.)

Adapt handling of empty args in function 'handle_v_run'
in gdbserver/server.cc to just insert an empty string
for an empty arg, since that one is now properly handled
in 'construct_inferior_arguments' already (and inserting
a "''" string in 'handle_v_run' would otherwise
cause that one to be treated as a string literally
containing two quote characters, which
'construct_inferior_args' would preserve by adding
extra escaping).

This makes gdbserver properly handle program args containing special
characters (like spaces), 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").

2020-05-12  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),
        win32-low.cc (win32_process_target::create_inferior): Use
        construct_inferior_arguments instead of stringify_argv
        to get string representation which properly escapes
        special characters.
        * server.cc (handle_v_run): Just pass empty program arg
        as such, since any further processing is now handled via
        construct_inferior_arguments
---
 gdbserver/linux-low.cc | 3 ++-
 gdbserver/lynx-low.cc  | 3 ++-
 gdbserver/server.cc    | 2 +-
 gdbserver/win32-low.cc | 3 ++-
 4 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.26.2

Comments

Simon Marchi May 13, 2020, 12:52 a.m. | #1
On 2020-05-12 11:42 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 (linux-low, lyn-low), 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".

> 

> Since construct_inferior_args has been extended to do

> proper quoting for Windows shells in commit

> 5d60742e2dd3c9b475dce54b56043a358751bbb8

> ("Fix quoting of special characters for the MinGW build.",

> 2012-06-12), use it for the Windows case as well.

> (I could not test that case myself, though.)

> 

> Adapt handling of empty args in function 'handle_v_run'

> in gdbserver/server.cc to just insert an empty string

> for an empty arg, since that one is now properly handled

> in 'construct_inferior_arguments' already (and inserting

> a "''" string in 'handle_v_run' would otherwise

> cause that one to be treated as a string literally

> containing two quote characters, which

> 'construct_inferior_args' would preserve by adding

> extra escaping).

> 

> This makes gdbserver properly handle program args containing special

> characters (like spaces), 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").

> 

> 2020-05-12  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),

>         win32-low.cc (win32_process_target::create_inferior): Use

>         construct_inferior_arguments instead of stringify_argv

>         to get string representation which properly escapes

>         special characters.

>         * server.cc (handle_v_run): Just pass empty program arg

>         as such, since any further processing is now handled via

>         construct_inferior_arguments


This patch LGTM.

Simon
Simon Marchi May 13, 2020, 12:54 a.m. | #2
On 2020-05-12 8:52 p.m., Simon Marchi wrote:
> On 2020-05-12 11:42 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 (linux-low, lyn-low), 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".

>>

>> Since construct_inferior_args has been extended to do

>> proper quoting for Windows shells in commit

>> 5d60742e2dd3c9b475dce54b56043a358751bbb8

>> ("Fix quoting of special characters for the MinGW build.",

>> 2012-06-12), use it for the Windows case as well.

>> (I could not test that case myself, though.)

>>

>> Adapt handling of empty args in function 'handle_v_run'

>> in gdbserver/server.cc to just insert an empty string

>> for an empty arg, since that one is now properly handled

>> in 'construct_inferior_arguments' already (and inserting

>> a "''" string in 'handle_v_run' would otherwise

>> cause that one to be treated as a string literally

>> containing two quote characters, which

>> 'construct_inferior_args' would preserve by adding

>> extra escaping).

>>

>> This makes gdbserver properly handle program args containing special

>> characters (like spaces), 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").

>>

>> 2020-05-12  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),

>>         win32-low.cc (win32_process_target::create_inferior): Use

>>         construct_inferior_arguments instead of stringify_argv

>>         to get string representation which properly escapes

>>         special characters.

>>         * server.cc (handle_v_run): Just pass empty program arg

>>         as such, since any further processing is now handled via

>>         construct_inferior_arguments

> 

> This patch LGTM.

> 

> Simon

> 


I forgot to mention, I built this on mingw-w64 and confirmed that it
fixed the same problem.  It also handles empty arguments correctly, both
on gdbserver's command line and "run".

Unfortunately, I wasn't able to run my test case, but that's because of
separate issues.

Simon
Kevin Buettner via Gdb-patches May 13, 2020, 9:58 a.m. | #3
On 13/05/2020 02.54, Simon Marchi wrote:
> On 2020-05-12 8:52 p.m., Simon Marchi wrote:

>> On 2020-05-12 11:42 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 (linux-low, lyn-low), 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".

>>>

>>> Since construct_inferior_args has been extended to do

>>> proper quoting for Windows shells in commit

>>> 5d60742e2dd3c9b475dce54b56043a358751bbb8

>>> ("Fix quoting of special characters for the MinGW build.",

>>> 2012-06-12), use it for the Windows case as well.

>>> (I could not test that case myself, though.)

>>>

>>> Adapt handling of empty args in function 'handle_v_run'

>>> in gdbserver/server.cc to just insert an empty string

>>> for an empty arg, since that one is now properly handled

>>> in 'construct_inferior_arguments' already (and inserting

>>> a "''" string in 'handle_v_run' would otherwise

>>> cause that one to be treated as a string literally

>>> containing two quote characters, which

>>> 'construct_inferior_args' would preserve by adding

>>> extra escaping).

>>>

>>> This makes gdbserver properly handle program args containing special

>>> characters (like spaces), 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").

>>>

>>> 2020-05-12  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),

>>>         win32-low.cc (win32_process_target::create_inferior): Use

>>>         construct_inferior_arguments instead of stringify_argv

>>>         to get string representation which properly escapes

>>>         special characters.

>>>         * server.cc (handle_v_run): Just pass empty program arg

>>>         as such, since any further processing is now handled via

>>>         construct_inferior_arguments

>>

>> This patch LGTM.

>>

>> Simon

>>

> 

> I forgot to mention, I built this on mingw-w64 and confirmed that it

> fixed the same problem.  It also handles empty arguments correctly, both

> on gdbserver's command line and "run".

> 

> Unfortunately, I wasn't able to run my test case, but that's because of

> separate issues.


Thanks a lot for testing it actually works and all reviewing!

Michael

Patch

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 3cd8d5594d..5479f02f42 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -984,7 +984,8 @@  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 ());
 
     pid = fork_inferior (program,
 			 str_program_args.c_str (),
diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc
index 9aa140c129..361ee5b48f 100644
--- a/gdbserver/lynx-low.cc
+++ b/gdbserver/lynx-low.cc
@@ -253,7 +253,8 @@  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 ());
 
   lynx_debug ("create_inferior ()");
 
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 27d0931f79..0313d18bb2 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2957,7 +2957,7 @@  handle_v_run (char *own_buf)
       else if (p == next_p)
 	{
 	  /* Empty argument.  */
-	  new_argv.push_back (xstrdup ("''"));
+	  new_argv.push_back (xstrdup (""));
 	}
       else
 	{
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 4eb63b7ca2..7a012ca81d 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -693,7 +693,8 @@  win32_process_target::create_inferior (const char *program,
   DWORD flags;
   PROCESS_INFORMATION pi;
   DWORD err;
-  std::string str_program_args = stringify_argv (program_args);
+  std::string str_program_args
+    = construct_inferior_arguments (program_args.size (), program_args.data ());
   char *args = (char *) str_program_args.c_str ();
 
   /* win32_wait needs to know we're not attaching.  */