[PATCH 0/8] Unify escaping/quoting of args in GDB/GDBserver

[PATCH 0/8] Unify escaping/quoting of args in GDB/GDBserver

[Michael Weghorn]

Historically, args passed directly on 'gdb --args <args>' invocation
were escaped (when the inferior is started in a shell), while those passed
via 'gdbserver <args>' were not.

That was changed in commit bea571ebd78ee29cb94adf648fbcda1e109e1be6
("Use construct_inferior_arguments which handles special chars",
2020-05-25), so args passed on 'gdbserver <args>' invocation were
quoted as well.
That commit accidently also resulted in args set in target
extended-remote ('set args <args>', 'run <args>')
to be passed to the shell in a quoted way.

Based on the disussion in PR 28392
("gdb server no longer supports argument globbing and variable substitution"),
this patch series addresses this and further unifies the behaviour of
how args (set either directly on GDB/GDBserver invocation or otherwise)
are treated between GDB and GDBserver, also adding a
'--no-escape-args' command line option for GDB and
GDBserver to explicitly disable escaping of args passed directly
on invocation.
It also makes handling args with spaces work in GDBserver
when the 'startup-with-shell' option is off.

(Adding more tests to explicitly test some of the edge cases
in the testsuite probably makes sense once it is clear whether
the approach taken here is considered appropriate.)

Michael Weghorn (8):
  gdbsupport: Extract escaping from 'construct_inferior_arguments'
  gdbsupport: Make escaping in construct_inferior_arguments optional
  PR27989 PR28446 gdbserver: Only escape args passed directly on
    invocation
  PR28392 Add a '--no-escape-args' option for gdb and gdbserver
  Extract helper function from 'fork_inferior'
  Add a 'fork_inferior' overload taking vector of args
  gdbserver: Support args with spaces for no-startup-with-shell case
  gdb: Support some escaping of args with startup-with-shell being off

 gdb/fork-child.c                         |   2 +-
 gdb/infcmd.c                             |   4 +-
 gdb/inferior.h                           |   2 +-
 gdb/main.c                               |   6 +-
 gdb/nat/fork-inferior.c                  | 152 +++++++++++++----------
 gdb/nat/fork-inferior.h                  |  18 ++-
 gdb/remote.c                             |  33 +++--
 gdb/testsuite/gdb.base/args.exp          |  23 +---
 gdb/testsuite/gdb.base/inferior-args.exp |   8 +-
 gdbserver/fork-child.cc                  |  10 +-
 gdbserver/linux-low.cc                   |   4 +-
 gdbserver/netbsd-low.cc                  |   4 +-
 gdbserver/server.cc                      |  74 +++++------
 gdbserver/win32-low.cc                   |   4 +-
 gdbsupport/common-inferior.cc            | 140 +++++++++++----------
 gdbsupport/common-inferior.h             |  16 ++-
 16 files changed, 276 insertions(+), 224 deletions(-)

-- 
2.33.0

[PATCH 1/8] gdbsupport: Extract escaping from 'construct_inferior_arguments'

[Michael Weghorn]

Extract the escaping of the arguments done in
'construct_inferior_arguments' to a new function
'escape_arg_as_necessary' which can be reused in
other places.
---
 gdbsupport/common-inferior.cc | 133 ++++++++++++++++++----------------
 gdbsupport/common-inferior.h  |   7 ++
 2 files changed, 77 insertions(+), 63 deletions(-)

diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index aed88409909..d6b99a89a81 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -28,7 +28,7 @@ bool startup_with_shell = true;
 /* See common-inferior.h.  */
 
 std::string
-construct_inferior_arguments (gdb::array_view<char * const> argv)
+escape_arg_as_necessary (char * arg)
 {
   std::string result;
 
@@ -46,79 +46,86 @@ construct_inferior_arguments (gdb::array_view<char * const> argv)
       static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
       static const char quote = '\'';
 #endif
-      for (int i = 0; i < argv.size (); ++i)
-	{
-	  if (i > 0)
-	    result += ' ';
-
-	  /* Need to handle empty arguments specially.  */
-	  if (argv[i][0] == '\0')
-	    {
-	      result += quote;
-	      result += quote;
-	    }
-	  else
-	    {
+      /* Need to handle empty arguments specially.  */
+      if (arg[0] == '\0')
+        {
+          result += quote;
+          result += quote;
+        }
+      else
+        {
 #ifdef __MINGW32__
-	      bool quoted = false;
+          bool quoted = false;
 
-	      if (strpbrk (argv[i], special))
-		{
-		  quoted = true;
-		  result += quote;
-		}
+          if (strpbrk (argv[i], special))
+            {
+              quoted = true;
+              result += quote;
+            }
 #endif
-	      for (char *cp = argv[i]; *cp; ++cp)
-		{
-		  if (*cp == '\n')
-		    {
-		      /* A newline cannot be quoted with a backslash (it
-			 just disappears), only by putting it inside
-			 quotes.  */
-		      result += quote;
-		      result += '\n';
-		      result += quote;
-		    }
-		  else
-		    {
+          for (char *cp = arg; *cp; ++cp)
+            {
+              if (*cp == '\n')
+                {
+                  /* A newline cannot be quoted with a backslash (it
+                     just disappears), only by putting it inside
+                     quotes.  */
+                  result += quote;
+                  result += '\n';
+                  result += quote;
+                }
+              else
+                {
 #ifdef __MINGW32__
-		      if (*cp == quote)
+                  if (*cp == quote)
 #else
-		      if (strchr (special, *cp) != NULL)
+                  if (strchr (special, *cp) != NULL)
 #endif
-			result += '\\';
-		      result += *cp;
-		    }
-		}
+                    result += '\\';
+                  result += *cp;
+                }
+            }
 #ifdef __MINGW32__
-	      if (quoted)
-		result += quote;
+          if (quoted)
+            result += quote;
 #endif
-	    }
-	}
+        }
     }
   else
     {
-      /* In this case we can't handle arguments that contain spaces,
-	 tabs, or newlines -- see breakup_args().  */
-      for (char *arg : argv)
-	{
-	  char *cp = strchr (arg, ' ');
-	  if (cp == NULL)
-	    cp = strchr (arg, '\t');
-	  if (cp == NULL)
-	    cp = strchr (arg, '\n');
-	  if (cp != NULL)
-	    error (_("can't handle command-line "
-		     "argument containing whitespace"));
-	}
-
-      for (int i = 0; i < argv.size (); ++i)
-	{
-	  if (i > 0)
-	    result += " ";
-	  result += argv[i];
-	}
+      result = arg;
+    }
+
+  return result;
+}
+
+/* See common-inferior.h.  */
+
+std::string
+construct_inferior_arguments (gdb::array_view<char * const> argv)
+{
+  std::string result;
+
+  for (int i = 0; i < argv.size (); ++i)
+    {
+      if (i > 0)
+        result += " ";
+
+      if (!startup_with_shell)
+        {
+          /* In this case we can't handle arguments that contain spaces,
+             tabs, or newlines -- see breakup_args().  */
+          char *cp = strchr (argv[i], ' ');
+          if (cp == NULL)
+            cp = strchr (argv[i], '\t');
+          if (cp == NULL)
+            cp = strchr (argv[i], '\n');
+          if (cp != NULL)
+            error (_("can't handle command-line "
+                     "argument containing whitespace"));
+        }
+
+      result += escape_arg_as_necessary (argv[i]);
     }
 
   return result;
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 92c880e1212..8f104b6c632 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -57,6 +57,13 @@ extern const std::string &get_inferior_cwd ();
    the target is started up with a shell.  */
 extern bool startup_with_shell;
 
+/* Returns a version of the argument that is quoted/escpaped as necessary.
+
+   Quoting/escaping is only necessary if the debuggee will be started in a
+   shell.  */
+extern std::string
+escape_arg_as_necessary (char * arg);
+
 /* Compute command-line string given argument vector. This does the
    same shell processing as fork_inferior.  */
 extern std::string
-- 
2.33.0

[PATCH 2/8] gdbsupport: Make escaping in construct_inferior_arguments optional

[Michael Weghorn]

Add a new bool param 'escape_args' to 'construct_inferior_arguments'
that specifies whether or not the arguments should be escaped for the
case that the debuggee is run in a shell.

The new param defaults to 'true' so the behaviour of existing
uses of the function remains unchanged.

Uses of the function with 'escape_args=false' will be added in
follow-up commits.
---
 gdbsupport/common-inferior.cc | 13 ++++++++++---
 gdbsupport/common-inferior.h  |  9 ++++++---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index d6b99a89a81..ca2355332d3 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -102,7 +102,8 @@ escape_arg_as_necessary (char * arg)
 /* See common-inferior.h.  */
 
 std::string
-construct_inferior_arguments (gdb::array_view<char * const> argv)
+construct_inferior_arguments (gdb::array_view<char * const> argv,
+                              bool escape_args)
 {
   std::string result;
 
@@ -124,8 +125,14 @@ construct_inferior_arguments (gdb::array_view<char * const> argv)
             error (_("can't handle command-line "
                      "argument containing whitespace"));
         }
-
-      result += escape_arg_as_necessary (argv[i]);
+      if (escape_args)
+        {
+          result += escape_arg_as_necessary (argv[i]);
+        }
+      else
+        {
+          result += argv[i];
+        }
     }
 
   return result;
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 8f104b6c632..db7bf82f08a 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -64,9 +64,12 @@ extern bool startup_with_shell;
 extern std::string
 escape_arg_as_necessary (char * arg);
 
-/* Compute command-line string given argument vector. This does the
-   same shell processing as fork_inferior.  */
+/* Compute command-line string given argument vector.
+
+  ESCAPE_ARGS specifies whether the arguments should be escaped for the case
+  that the debuggee is run in a shell.  */
 extern std::string
-construct_inferior_arguments (gdb::array_view<char * const>);
+construct_inferior_arguments (gdb::array_view<char * const>,
+                              bool escape_args = true);
 
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.33.0

[PATCH 3/8] PR27989 PR28446 gdbserver: Only escape args passed directly on invocation

[Michael Weghorn]

Since commit bea571ebd78ee29cb94adf648fbcda1e109e1be6
("Use construct_inferior_arguments which handles special chars",
2020-05-25), args for gdbserver were escaped when the debuggee is
run in a shell.

That commit caused not only args passed directly on gdbserver
invocation to be escaped, but also those set in
target extended-remote e.g. via
'set args <ARGS>' or 'run <ARGS>', breaking features
like globbing and variable expansion.

As is the case for 'gdb --args', only escape args
that are passed directly on gdbserver invocation,
not args set otherwise.

To do so, escape the single args in 'captured_main' before calling
'target_create_inferior' instead of doing the escaping in
the latter (so other calls to 'target_create_inferior' don't
also result in escaped args being used).

A new gdbserver/gdb option to explicitly disable escaping
also for args passed directly on invocation will be added
in a separate commit, s. discussion in PR 28392.

This fixes test-cases 'gdb.base/a2-run.exp' and
'gdb.base/startup-with-shell.exp' for target board
native-extended-gdbserver.

Properly preserving whitespace and quotes set in target
native-extended-gdbserver using e.g.

    run "first arg" second_arg third_arg

needs another modification to not result in args with spaces
not being handled correctly anymore, as is described in
PR 25893 (which was fixed by the above-mentioned commit
bea571ebd78ee29cb94adf648fbcda1e109e1be6) and is tested e.g. by
running test case 'gdb.base/inferior-args.exp' with target
board native-extended-gdbserver.

Constructing a 'gdb_argv' from the args string in
'remote_target::extended_remote_run' would result in literal
quotes in args getting lost (s. 'buildargv' in 'libiberty/argv.c',
which is called by the 'gdb_argv' ctor), so concatenating the args just
using spaces later on gdbserver side would not restore those,
meaning the behaviour of args set in target native-extended-gdbserver
would differ from the one in plain gdb:

* The above example with an arg containing a space would result
  in 'first arg second_arg third_arg' being passed to in the shell,
  rather than '"first arg" second_arg third_arg', meaning the
  arg with a space is split into two.

* Since literal quote characters are removed,

      run args '$SHELL'

  in target extended-remote also resulted in the same behaviour as

      run args "$SHELL"

  and the variable "$SHELL" would be expanded in the shell in which the
  debuggee is run also when using single quotes, which is unexpected and
  differs from what happens when using plain gdb.

To prevent this and make the behaviour more consistent,
pass the args string as is as a single argument for the case where
the debuggee is run in a shell (i.e. 'startup_with_shell' is true)
in 'remote_target::extended_remote_run'.

Since one intention of running the inferior in a
shell is to apply the usual shell processing on the
args before passing it to the inferior (like
globbing and variable expansion), passing the
unmodified string around and then just passing it
in the shell as is ensures that this actually happens.

This also makes the escaping of args in 'handle_v_run' added in
commit 2090129c36c7e582943b7d300968d19b46160d84
("Share fork_inferior et al with gdbserver", 2016-12,22)
unnecessary, so drop it.
(It was applied unconditionally, but should from my understanding
only have been applied for the case where 'startup-with-shell'
is on anyway.)

Remove the KFAILs from the tests added in commit
18263be7565782a9c07045a7a72d80c507a5be09
("gdb/testsuite: gdb.base/args.exp: add KFAIL for native-extended-gdbserver",
2021-06-17), since those now pass again with target
native-extended-gdbserver.

This changes the exact way that args are passed via the
remote serial protocol (RSP) for the case where
startup-with-shell is enabled, since only a single arg
(a string containing all args) is passed now.
Negative effects on the compatibility with older GDB/GDBserver
versions seem limited, given that only args containing
quotes are affected, and those were not really
resulting in the expected behaviour in many cases anyway
(args with spaces getting split before commit
bea571ebd78ee29cb94adf648fbcda1e109e1be6,
"Use construct_inferior_arguments which handles special chars",
escaped/quoted quotes getting an extra level of quotes after
that commit).
---
 gdb/remote.c                    | 33 ++++++++++++++--------
 gdb/testsuite/gdb.base/args.exp | 23 ++++-----------
 gdbserver/linux-low.cc          |  4 ++-
 gdbserver/netbsd-low.cc         |  4 ++-
 gdbserver/server.cc             | 50 +++++----------------------------
 gdbserver/win32-low.cc          |  4 ++-
 6 files changed, 43 insertions(+), 75 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index d5eb40ce578..2a924a6bfee 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10306,17 +10306,28 @@ remote_target::extended_remote_run (const std::string &args)
 
   if (!args.empty ())
     {
-      int i;
-
-      gdb_argv argv (args.c_str ());
-      for (i = 0; argv[i] != NULL; i++)
-	{
-	  if (strlen (argv[i]) * 2 + 1 + len >= get_remote_packet_size ())
-	    error (_("Argument list too long for run packet"));
-	  rs->buf[len++] = ';';
-	  len += 2 * bin2hex ((gdb_byte *) argv[i], rs->buf.data () + len,
-			      strlen (argv[i]));
-	}
+      if (startup_with_shell &&
+          packet_support (PACKET_QStartupWithShell) != PACKET_DISABLE)
+        {
+          /* Pass single args string to preserve whitespace and quotes.  */
+          if (strlen (args.c_str()) * 2 + 1 + len >= get_remote_packet_size ())
+            error (_("Argument list too long for run packet"));
+          rs->buf[len++] = ';';
+          len += 2 * bin2hex ((gdb_byte *) args.c_str (), rs->buf.data () + len,
+                          strlen (args.c_str ()));
+        }
+      else
+        {
+          gdb_argv argv (args.c_str ());
+          for (int i = 0; argv[i] != NULL; i++)
+            {
+              if (strlen (argv[i]) * 2 + 1 + len >= get_remote_packet_size ())
+                error (_("Argument list too long for run packet"));
+              rs->buf[len++] = ';';
+              len += 2 * bin2hex ((gdb_byte *) argv[i], rs->buf.data () + len,
+                              strlen (argv[i]));
+            }
+        }
     }
 
   rs->buf[len++] = '\0';
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 36dd19e64df..e76368f544e 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -34,10 +34,7 @@ if {[build_executable $testfile.exp $testfile \
     return -1
 }
 
-# If SINGLE_QUOTES_NEWLINE_KFAIL true, arguments made of exactly '' or a
-# newline character will fail, so kfail those tests.
-
-proc args_test { name arglist {single_quotes_newline_kfail false}} {
+proc args_test { name arglist } {
     global srcdir
     global subdir
     global testfile
@@ -55,10 +52,6 @@ proc args_test { name arglist {single_quotes_newline_kfail false}} {
 
     set i 1
     foreach arg $arglist {
-	if { $single_quotes_newline_kfail
-	     && ($arg == {''} || $arg == {\\n}) } {
-	    setup_kfail "gdb/27989" "*-*-*"
-	}
 	gdb_test "print argv\[$i\]" "\\\$$decimal = $hex \"$arg\"" \
 	    "argv\[$i\] for $name"
 	set i [expr $i + 1]
@@ -72,12 +65,6 @@ proc args_test { name arglist {single_quotes_newline_kfail false}} {
 save_vars { GDBFLAGS } {
     set old_gdbflags $GDBFLAGS
 
-    # Single quotes and newlines are not well handled by the extended-remote
-    # target:  https://sourceware.org/bugzilla/show_bug.cgi?id=27989
-    set single_quotes_newline_kfail \
-	[expr { [target_info exists gdb_protocol] \
-	        && [target_info gdb_protocol] == "extended-remote" }]
-
     set GDBFLAGS "$old_gdbflags --args $binfile 1 3"
     args_test basic {{1} {3}}
 
@@ -98,16 +85,16 @@ save_vars { GDBFLAGS } {
     # Try with arguments containing literal single quotes.
 
     set GDBFLAGS "$old_gdbflags --args $binfile 1 '' 3"
-    args_test "one empty with single quotes" {{1} {''} {3}} $single_quotes_newline_kfail
+    args_test "one empty with single quotes" {{1} {''} {3}}
 
     set GDBFLAGS "$old_gdbflags --args $binfile 1 '' '' 3"
-    args_test "two empty with single quotes" {{1} {''} {''} {3}} $single_quotes_newline_kfail
+    args_test "two empty with single quotes" {{1} {''} {''} {3}}
 
     # try with arguments containing literal newlines.
 
     set GDBFLAGS "$old_gdbflags --args $binfile 1 {\n} 3"
-    args_test "one newline" {{1} {\\n} {3}} $single_quotes_newline_kfail
+    args_test "one newline" {{1} {\\n} {3}}
 
     set GDBFLAGS "$old_gdbflags --args $binfile 1 {\n} {\n} 3"
-    args_test "two newlines" {{1} {\\n} {\\n} {3}} $single_quotes_newline_kfail
+    args_test "two newlines" {{1} {\\n} {\\n} {3}}
 }
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d27a2169029..d61bb4fe029 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -964,7 +964,9 @@ linux_process_target::create_inferior (const char *program,
   {
     maybe_disable_address_space_randomization restore_personality
       (cs.disable_randomization);
-    std::string str_program_args = construct_inferior_arguments (program_args);
+    // 'program_args' contains args already escaped as needed
+    std::string str_program_args = construct_inferior_arguments (program_args,
+                                                                 false);
 
     pid = fork_inferior (program,
 			 str_program_args.c_str (),
diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
index 84e34d0e065..3950f4cacc2 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -81,7 +81,9 @@ int
 netbsd_process_target::create_inferior (const char *program,
 					const std::vector<char *> &program_args)
 {
-  std::string str_program_args = construct_inferior_arguments (program_args);
+  // 'program_args' contains args already escaped as needed
+  std::string str_program_args = construct_inferior_arguments (program_args,
+                                                               false);
 
   pid_t pid = fork_inferior (program, str_program_args.c_str (),
 			     get_environ ()->envp (), netbsd_ptrace_fun,
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 193c3d9d7d1..3926be19fd1 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3039,55 +3039,16 @@ handle_v_run (char *own_buf)
       else
 	{
 	  size_t len = (next_p - p) / 2;
-	  /* ARG is the unquoted argument received via the RSP.  */
+	  /* ARG is the argument received via the RSP.  */
 	  char *arg = (char *) xmalloc (len + 1);
-	  /* FULL_ARGS will contain the quoted version of ARG.  */
-	  char *full_arg = (char *) xmalloc ((len + 1) * 2);
-	  /* These are pointers used to navigate the strings above.  */
-	  char *tmp_arg = arg;
-	  char *tmp_full_arg = full_arg;
-	  int need_quote = 0;
 
 	  hex2bin (p, (gdb_byte *) arg, len);
 	  arg[len] = '\0';
 
-	  while (*tmp_arg != '\0')
-	    {
-	      switch (*tmp_arg)
-		{
-		case '\n':
-		  /* Quote \n.  */
-		  *tmp_full_arg = '\'';
-		  ++tmp_full_arg;
-		  need_quote = 1;
-		  break;
-
-		case '\'':
-		  /* Quote single quote.  */
-		  *tmp_full_arg = '\\';
-		  ++tmp_full_arg;
-		  break;
-
-		default:
-		  break;
-		}
-
-	      *tmp_full_arg = *tmp_arg;
-	      ++tmp_full_arg;
-	      ++tmp_arg;
-	    }
-
-	  if (need_quote)
-	    *tmp_full_arg++ = '\'';
-
-	  /* Finish FULL_ARG and push it into the vector containing
-	     the argv.  */
-	  *tmp_full_arg = '\0';
 	  if (i == 0)
-	    new_program_name = full_arg;
+            new_program_name = arg;
 	  else
-	    new_argv.push_back (full_arg);
-	  xfree (arg);
+	    new_argv.push_back (arg);
 	}
       if (*next_p)
 	next_p++;
@@ -3971,7 +3932,10 @@ captured_main (int argc, char *argv[])
       n = argc - (next_arg - argv);
       program_path.set (make_unique_xstrdup (next_arg[0]));
       for (i = 1; i < n; i++)
-	program_args.push_back (xstrdup (next_arg[i]));
+        {
+          std::string escaped_arg = escape_arg_as_necessary (next_arg[i]);
+          program_args.push_back (xstrdup (escaped_arg.c_str ()));
+        }
 
       /* Wait till we are at first instruction in program.  */
       target_create_inferior (program_path.get (), program_args);
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 1e97f91d954..4d949357257 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -607,7 +607,9 @@ win32_process_target::create_inferior (const char *program,
   DWORD flags;
   PROCESS_INFORMATION pi;
   DWORD err;
-  std::string str_program_args = construct_inferior_arguments (program_args);
+  // 'program_args' contains args already escaped as needed
+  std::string str_program_args = construct_inferior_arguments (program_args,
+                                                               false);
   char *args = (char *) str_program_args.c_str ();
 
   /* win32_wait needs to know we're not attaching.  */
-- 
2.33.0

[PATCH 4/8] PR28392 Add a '--no-escape-args' option for gdb and gdbserver

[Michael Weghorn]

This introduces a new '--no-escape-args' option
for gdb and gdbserver.

When the startup-with-shell option is enabled, args
passed directly on 'gdb --args <args>' or
'gdbserver <args>' invocation are by default escaped
so that they are passed to the inferior as passed on
invocation, no globbing or variable substitution happens
when the args are passed to the shell that is
started by the GDB process later.
(For gdbserver, this is the case since
commit bea571ebd78ee29cb94adf648fbcda1e109e1be6,
"Use construct_inferior_arguments which handles special chars".)

Only args set e.g. via 'set args <args>' or
'run <args>' are not escaped.

For the 'gdb --args' case, directly settings unescaped
args on gdb invocation was possible e.g. by using the
"--eval-command='set args <args>'", while this possibility
did not exist for gdbserver.

The new '--no-escape-args' option disables this escaping of
args passed directly on invocation.

For gdbserver, using this new option allows having the behaviour
from before commit bea571ebd78ee29cb94adf648fbcda1e109e1be6,
while keeping the behaviour unified between GDB and GDBserver.

(For more reasoning, s. the discussion in PR28392,
"gdb server no longer supports argument globbing and variable
substitution".)
---
 gdb/infcmd.c        |  4 ++--
 gdb/inferior.h      |  2 +-
 gdb/main.c          |  6 +++++-
 gdbserver/server.cc | 28 ++++++++++++++++++++++++----
 4 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b55a56c020d..4283f3ba201 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -123,10 +123,10 @@ show_inferior_tty_command (struct ui_file *file, int from_tty,
 }
 
 void
-set_inferior_args_vector (int argc, char **argv)
+set_inferior_args_vector (int argc, char **argv, bool escape_args)
 {
   gdb::array_view<char * const> args (argv, argc);
-  std::string n = construct_inferior_arguments (args);
+  std::string n = construct_inferior_arguments (args, escape_args);
   current_inferior ()->set_args (std::move (n));
 }
 
diff --git a/gdb/inferior.h b/gdb/inferior.h
index e9210b1258d..0896bb1f459 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -204,7 +204,7 @@ extern void post_create_inferior (int from_tty);
 
 extern void attach_command (const char *, int);
 
-extern void set_inferior_args_vector (int, char **);
+extern void set_inferior_args_vector (int, char **, bool escape_args);
 
 extern void registers_info (const char *, int);
 
diff --git a/gdb/main.c b/gdb/main.c
index ca4ccc375dc..2b979d010c1 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -628,6 +628,7 @@ captured_main_1 (struct captured_main_args *context)
 
   static int quiet = 0;
   static int set_args = 0;
+  static int escape_args = 1;
   static int inhibit_home_gdbinit = 0;
 
   /* Pointers to various arguments from command line.  */
@@ -833,6 +834,7 @@ captured_main_1 (struct captured_main_args *context)
       {"statistics", no_argument, 0, OPT_STATISTICS},
       {"write", no_argument, &write_files_1, 1},
       {"args", no_argument, &set_args, 1},
+      {"no-escape-args", no_argument, &escape_args, 0},
       {"l", required_argument, 0, 'l'},
       {"return-child-result", no_argument, &return_child_result, 1},
       {0, no_argument, 0, 0}
@@ -1070,7 +1072,7 @@ captured_main_1 (struct captured_main_args *context)
       symarg = argv[optind];
       execarg = argv[optind];
       ++optind;
-      set_inferior_args_vector (argc - optind, &argv[optind]);
+      set_inferior_args_vector (argc - optind, &argv[optind], escape_args);
     }
   else
     {
@@ -1399,6 +1401,8 @@ This is the GNU debugger.  Usage:\n\n\
   fputs_unfiltered (_("\
 Selection of debuggee and its files:\n\n\
   --args             Arguments after executable-file are passed to inferior.\n\
+  --no-escape-args   Don't escape args passed via '--args' when invoking the\n\
+                     inferior in a shell.\n\
   --core=COREFILE    Analyze the core dump COREFILE.\n\
   --exec=EXECFILE    Use EXECFILE as the executable.\n\
   --pid=PID          Attach to running process PID.\n\
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 3926be19fd1..14bd1af0404 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3456,10 +3456,20 @@ gdbserver_usage (FILE *stream)
 	   "  --startup-with-shell\n"
 	   "                        Start PROG using a shell.  I.e., execs a shell that\n"
 	   "                        then execs PROG.  (default)\n"
+	   "                        To make use of globbing and variable subsitution for\n"
+	   "                        arguments passed directly on gdbserver invocation,\n"
+	   "                        see the --no-escape-args command line option in\n"
+	   "                        addition\n"
 	   "  --no-startup-with-shell\n"
 	   "                        Exec PROG directly instead of using a shell.\n"
-	   "                        Disables argument globbing and variable substitution\n"
-	   "                        on UNIX-like systems.\n"
+	   "  --no-escape-args\n"
+	   "                        If PROG is started using a shell (see the\n"
+	   "                        --[no-]startup-with-shell option),\n"
+	   "                        ARGS passed directly on gdbserver invocation are\n"
+	   "                        escaped, so no globbing or variable substitution\n"
+	   "                        happens for those. This option disables escaping, so\n"
+	   "                        globbing and variable substituation in the shell\n"
+	   "                        are done for ARGS on UNIX-like systems.\n"
 	   "\n"
 	   "Debug options:\n"
 	   "\n"
@@ -3689,6 +3699,7 @@ captured_main (int argc, char *argv[])
   volatile int attach = 0;
   int was_running;
   bool selftest = false;
+  bool escape_args = true;
 #if GDB_SELF_TEST
   std::vector<const char *> selftest_filters;
 
@@ -3825,6 +3836,8 @@ captured_main (int argc, char *argv[])
 	startup_with_shell = true;
       else if (strcmp (*next_arg, "--no-startup-with-shell") == 0)
 	startup_with_shell = false;
+      else if (strcmp (*next_arg, "--no-escape-args") == 0)
+        escape_args = false;
       else if (strcmp (*next_arg, "--once") == 0)
 	run_once = true;
       else if (strcmp (*next_arg, "--selftest") == 0)
@@ -3933,8 +3946,15 @@ captured_main (int argc, char *argv[])
       program_path.set (make_unique_xstrdup (next_arg[0]));
       for (i = 1; i < n; i++)
         {
-          std::string escaped_arg = escape_arg_as_necessary (next_arg[i]);
-          program_args.push_back (xstrdup (escaped_arg.c_str ()));
+          if (escape_args)
+            {
+              std::string escaped_arg = escape_arg_as_necessary (next_arg[i]);
+              program_args.push_back (xstrdup (escaped_arg.c_str ()));
+            }
+          else
+            {
+              program_args.push_back (xstrdup (next_arg[i]));
+            }
         }
 
       /* Wait till we are at first instruction in program.  */
-- 
2.33.0

[PATCH 5/8] Extract helper function from 'fork_inferior'

[Michael Weghorn]

Extract a helper function 'do_fork_inferior'
that takes an argument vector and
actually forks the inferior.

'fork_inferior' now only builds the argument vector
and calls the new helper function.

This is in preparation of introducing a new
'fork_inferior' overload that takes a
'std::vector<char **>' param instead of
a single 'const std::string&' one in which all of the
single args are contained.
The latter is problematic when the string
has to be split into single args again
later on, as is the case when the debuggee
is not run in a shell.

Also adapt 'prefork_hook' to take a
'char**' param instead of a 'char*' one
and concatenate the single args using an
extra space in between for the debug output.
This slightly changes debug output, since
e.g. the 'exec_file' is also contained in
the args array being used now.
---
 gdb/fork-child.c        |  2 +-
 gdb/nat/fork-inferior.c | 40 +++++++++++++++++++++++++++++-----------
 gdb/nat/fork-inferior.h |  6 +++---
 gdbserver/fork-child.cc | 10 ++++++++--
 4 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 3e995ed6f65..7a11ac2e3dc 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -59,7 +59,7 @@ static struct ui *saved_ui = NULL;
 /* See nat/fork-inferior.h.  */
 
 void
-prefork_hook (const char *args)
+prefork_hook (char **args)
 {
   gdb_assert (saved_ui == NULL);
   /* Retain a copy of our UI, since the child will replace this value
diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index c88cf4cf716..3a1abb596d9 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -29,6 +29,13 @@
 #include "gdbsupport/gdb_tilde_expand.h"
 #include <vector>
 
+pid_t do_fork_inferior (char **env, void (*traceme_fun) (),
+                        gdb::function_view<void (int)> init_trace_fun,
+                        void (*pre_trace_fun) (),
+                        void (*exec_fun)(const char *file, char * const *argv,
+                        char * const *env),
+                        char **argv);
+
 extern char **environ;
 
 /* Build the argument vector for execv(3).  */
@@ -273,14 +280,8 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
 	       void (*exec_fun)(const char *file, char * const *argv,
 				char * const *env))
 {
-  pid_t pid;
-  /* Set debug_fork then attach to the child while it sleeps, to debug.  */
-  int debug_fork = 0;
   const char *shell_file;
   const char *exec_file;
-  char **save_our_env;
-  int i;
-  int save_errno;
 
   /* If no exec file handed to us, get it from the exec-file command
      -- with a good, common error message if none is specified.  */
@@ -307,15 +308,34 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
 
   /* Build the argument vector.  */
   execv_argv child_argv (exec_file, allargs, shell_file);
+  char **argv = child_argv.argv ();
+
+  return do_fork_inferior (env, traceme_fun, init_trace_fun,
+                           pre_trace_fun, exec_fun, argv);
+}
+
+/* Helper function for 'fork_inferior'.  */
+
+ pid_t do_fork_inferior (char **env, void (*traceme_fun) (),
+                         gdb::function_view<void (int)> init_trace_fun,
+                         void (*pre_trace_fun) (),
+                         void (*exec_fun)(const char *file, char * const *argv,
+                         char * const *env),
+                         char **argv)
+ {
+  pid_t pid;
+  /* Set debug_fork then attach to the child while it sleeps, to debug.  */
+  int debug_fork = 0;
+  int save_errno;
 
   /* Retain a copy of our environment variables, since the child will
      replace the value of environ and if we're vforked, we have to
      restore it.  */
-  save_our_env = environ;
+  char **save_our_env = environ;
 
   /* Perform any necessary actions regarding to TTY before the
      fork/vfork call.  */
-  prefork_hook (allargs.c_str ());
+  prefork_hook (argv);
 
   /* It is generally good practice to flush any possible pending stdio
      output prior to doing a fork, to avoid the possibility of both
@@ -409,8 +429,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
 	 path to find $SHELL.  Rich Pixley says so, and I agree.  */
       environ = env;
 
-      char **argv = child_argv.argv ();
-
       if (exec_fun != NULL)
 	(*exec_fun) (argv[0], &argv[0], env);
       else
@@ -420,7 +438,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
       save_errno = errno;
       warning ("Cannot exec %s", argv[0]);
 
-      for (i = 1; argv[i] != NULL; i++)
+      for (int i = 1; argv[i] != NULL; i++)
 	warning (" %s", argv[i]);
 
       warning ("Error: %s", safe_strerror (save_errno));
diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
index aae2aa6a854..e56b6dac1a4 100644
--- a/gdb/nat/fork-inferior.h
+++ b/gdb/nat/fork-inferior.h
@@ -59,9 +59,9 @@ extern ptid_t startup_inferior (process_stratum_target *proc_target,
 				ptid_t *myptid);
 
 /* Perform any necessary tasks before a fork/vfork takes place.  ARGS
-   is a string containing all the arguments received by the inferior.
-   This function is mainly used by fork_inferior.  */
-extern void prefork_hook (const char *args);
+   is a char** containing all arguments passed to
+   the exec_fun/execvp by fork_inferior.  */
+extern void prefork_hook (char **args);
 
 /* Perform any necessary tasks after a fork/vfork takes place.  This
    function is mainly used by fork_inferior.  */
diff --git a/gdbserver/fork-child.cc b/gdbserver/fork-child.cc
index a431e7ef889..691e7cf652a 100644
--- a/gdbserver/fork-child.cc
+++ b/gdbserver/fork-child.cc
@@ -42,12 +42,18 @@ restore_old_foreground_pgrp (void)
 /* See nat/fork-inferior.h.  */
 
 void
-prefork_hook (const char *args)
+prefork_hook (char **args)
 {
   client_state &cs = get_client_state ();
   if (debug_threads)
     {
-      debug_printf ("args: %s\n", args);
+      std::string args_str;
+      for (char **arg = args; *arg != nullptr; arg++)
+        {
+          args_str += *arg;
+          args_str += " ";
+        }
+      debug_printf ("args: %s\n", args_str.c_str ());
       debug_flush ();
     }
 
-- 
2.33.0

[PATCH 6/8] Add a 'fork_inferior' overload taking vector of args

[Michael Weghorn]

Add an overload for 'fork_inferior' that takes a
'const std::vector<char *>&' for the program args
instead of a 'const std::string&' containing all of the
args concatenated together.

Use that overload for the gdbserver case
(for linux-low and netbsd-low).

For now, the new 'fork_inferior' overload just calls
'construct_inferior_arguments' as was previously
done in the two call sites before calling
'construct_inferior_arguments', so the behaviour remains
unchanged.
That will be changed in a follow-up commit to support
handling of args with spaces for the case where the
'startup-with-shell' option is off in gdbserver.
---
 gdb/nat/fork-inferior.c | 16 ++++++++++++++++
 gdb/nat/fork-inferior.h | 12 ++++++++++++
 gdbserver/linux-low.cc  |  6 +-----
 gdbserver/netbsd-low.cc |  6 +-----
 4 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 3a1abb596d9..8a6ec8ced6c 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -314,6 +314,22 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
                            pre_trace_fun, exec_fun, argv);
 }
 
+/* See nat/fork-inferior.h.  */
+
+pid_t
+fork_inferior (const char *exec_file_arg, const std::vector<char*> &program_args,
+               char **env, void (*traceme_fun) (),
+               gdb::function_view<void (int)> init_trace_fun,
+               void (*pre_trace_fun) (),
+               const char *shell_file_arg,
+               void (*exec_fun)(const char *file, char * const *argv,
+                                char * const *env))
+{
+  std::string allargs = construct_inferior_arguments (program_args, false);
+  return fork_inferior(exec_file_arg, allargs, env, traceme_fun, init_trace_fun,
+                       pre_trace_fun, shell_file_arg, exec_fun);
+}
+
 /* Helper function for 'fork_inferior'.  */
 
  pid_t do_fork_inferior (char **env, void (*traceme_fun) (),
diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
index e56b6dac1a4..f96b42fa64f 100644
--- a/gdb/nat/fork-inferior.h
+++ b/gdb/nat/fork-inferior.h
@@ -21,6 +21,7 @@
 #define NAT_FORK_INFERIOR_H
 
 #include <string>
+#include <vector>
 #include "gdbsupport/function-view.h"
 
 struct process_stratum_target;
@@ -50,6 +51,17 @@ extern pid_t fork_inferior (const char *exec_file_arg,
 					      char * const *argv,
 					      char * const *env));
 
+/* Overload for fork inferior that takes the program arguments as a vector.
+ * PROGRAM_ARGS is a vector containing the arguments to the program.  */
+extern pid_t fork_inferior (const char *exec_file_arg,
+                            const std::vector<char*> &program_args,
+                            char **env, void (*traceme_fun) (),
+                            gdb::function_view<void (int)> init_trace_fun,
+                            void (*pre_trace_fun) (),
+                            const char *shell_file_arg,
+                            void (*exec_fun) (const char *file, char * const *argv,
+                                              char * const *env));
+
 /* Accept NTRAPS traps from the inferior.
 
    Return the ptid of the inferior being started.  */
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d61bb4fe029..88f093e2622 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -964,12 +964,8 @@ linux_process_target::create_inferior (const char *program,
   {
     maybe_disable_address_space_randomization restore_personality
       (cs.disable_randomization);
-    // 'program_args' contains args already escaped as needed
-    std::string str_program_args = construct_inferior_arguments (program_args,
-                                                                 false);
 
-    pid = fork_inferior (program,
-			 str_program_args.c_str (),
+    pid = fork_inferior (program, program_args,
 			 get_environ ()->envp (), linux_ptrace_fun,
 			 NULL, NULL, NULL, NULL);
   }
diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
index 3950f4cacc2..6243ad4bb0e 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -81,11 +81,7 @@ int
 netbsd_process_target::create_inferior (const char *program,
 					const std::vector<char *> &program_args)
 {
-  // 'program_args' contains args already escaped as needed
-  std::string str_program_args = construct_inferior_arguments (program_args,
-                                                               false);
-
-  pid_t pid = fork_inferior (program, str_program_args.c_str (),
+  pid_t pid = fork_inferior (program, program_args,
 			     get_environ ()->envp (), netbsd_ptrace_fun,
 			     nullptr, nullptr, nullptr, nullptr);
 
-- 
2.33.0

[PATCH 7/8] gdbserver: Support args with spaces for no-startup-with-shell case

[Michael Weghorn]

Pass a vector of args directly to the 'do_fork_inferior'
helper function when the 'fork_inferior' overload
taking a vector of args is used, rather than first
concatenating the args to a single string where the
args are separated by spaces.

From such a string, it would not be possible to extract
args containing spaces anymore, which is why
'construct_inferior_arguments' would exit with an
error.

This makes handling args with spaces (as described
in PR 25893) work for the case where 'startup_with_shell'
is disabled, e.g. when gdbserver is invoked with the
'--no-startup-shell' option or when setting
args in target extended-remote, like

    set startup-with-shell off
    run "first arg" second_arg

Test cases will be added in a follow-up commit
that further unifies the behaviour between
GDB and GDBserver.
---
 gdb/nat/fork-inferior.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 8a6ec8ced6c..8e0e50157bc 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -325,9 +325,30 @@ fork_inferior (const char *exec_file_arg, const std::vector<char*> &program_args
                void (*exec_fun)(const char *file, char * const *argv,
                                 char * const *env))
 {
-  std::string allargs = construct_inferior_arguments (program_args, false);
-  return fork_inferior(exec_file_arg, allargs, env, traceme_fun, init_trace_fun,
-                       pre_trace_fun, shell_file_arg, exec_fun);
+  if (startup_with_shell)
+    {
+      // call the variant taking a single string param for all args
+      std::string allargs = construct_inferior_arguments (program_args, false);
+      return fork_inferior (exec_file_arg, allargs, env, traceme_fun,
+                            init_trace_fun, pre_trace_fun, shell_file_arg, exec_fun);
+    }
+
+  const char *exec_file;
+
+  /* If no exec file handed to us, get it from the exec-file command
+     -- with a good, common error message if none is specified.  */
+  if (exec_file_arg == NULL)
+    exec_file = get_exec_file (1);
+  else
+    exec_file = exec_file_arg;
+
+  std::vector<char*> argv;
+  argv.push_back (const_cast<char*> (exec_file));
+  argv.insert (argv.end (), program_args.begin (), program_args.end ());
+  argv.push_back (nullptr);
+
+  return do_fork_inferior (env, traceme_fun, init_trace_fun, pre_trace_fun,
+                           exec_fun, argv.data ());
 }
 
 /* Helper function for 'fork_inferior'.  */
-- 
2.33.0

[PATCH 8/8] gdb: Support some escaping of args with startup-with-shell being off

[Michael Weghorn]

Previously, 'execv_argv::init_for_no_shell', which is called
from the 'fork_inferior' overload that gets all args in a single string
had a custom implementation to split the string containing all args
into an array of single args when the 'startup-with-shell' option is
off (which is on by default).

That implementation was just splitting at whitespace characters and
ignoring any quoting, so e.g. args set for target-board unix using

    set startup-with-shell off
    set args "first arg" second_arg

would result in three args {'"first', 'arg"', 'second_arg'} being
passed to the inferior.

Replace this custom splitting of the args string into single args
and use the existing 'buildargv' function instead, which supports
some basic quoting and escaping, so e.g. the above results in two
args {'first arg', 'second_arg'} being passed to the debuggee,
as is the case when the 'startup-with-shell' option is on.
The same splitting of args was already used for target
extended-remote before (s. args splitting done in
'remote_target::extended_remote_run', and the 'fork_inferior'
overload taking args as a vector is used later on, so no additional
splitting is done then).
Thus, using the same here makes the behaviour more consistent
between GDB and GDBserver as well.

This changes the 'execv_argv' class accordingly and drops the
optimization to have all the 'char *' in 'm_argv' point to a single
string rather than allocating a separate string for each arg.

With this in place, quoting or escaping whitespace characters
is now supported with startup-with-shell being off, and literal
quotes can be inserted by escaping them, so e.g. the previous
behaviour of the above

    set startup-with-shell off
    set args "first arg" second_arg

example can now be reached by using either

    set startup-with-shell off
    set args \"first arg\" second_arg

or

    set startup-with-shell off
    set args '"first' 'arg"' second_arg

Also extend the existing 'gdb.base/inferior-args.exp' test
to cover the case where 'startup-with-shell' is off.
---
 gdb/nat/fork-inferior.c                  | 75 +++++++-----------------
 gdb/testsuite/gdb.base/inferior-args.exp |  8 ++-
 2 files changed, 27 insertions(+), 56 deletions(-)

diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 8e0e50157bc..f1cfc0d84fa 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -49,6 +49,14 @@ class execv_argv
   execv_argv (const char *exec_file, const std::string &allargs,
 	      const char *shell_file);
 
+  ~execv_argv ()
+  {
+    for (char *arg : m_argv)
+      {
+        xfree (arg);
+      }
+  }
+
   /* Return a pointer to the built argv, in the type expected by
      execv.  The result is (only) valid for as long as this execv_argv
      object is live.  We return a "char **" because that's the type
@@ -57,7 +65,7 @@ class execv_argv
      strings to which the array point.  */
   char **argv ()
   {
-    return const_cast<char **> (&m_argv[0]);
+    return m_argv.data ();
   }
 
 private:
@@ -76,66 +84,29 @@ class execv_argv
 		       const std::string &allargs,
 		       const char *shell_file);
 
-  /* The argument vector built.  Holds non-owning pointers.  Elements
-     either point to the strings passed to the execv_argv ctor, or
-     inside M_STORAGE.  */
-  std::vector<const char *> m_argv;
-
-  /* Storage.  In the no-shell case, this contains a copy of the
-     arguments passed to the ctor, split by '\0'.  In the shell case,
-     this contains the quoted shell command.  I.e., SHELL_COMMAND in
-     {"$SHELL" "-c", SHELL_COMMAND, NULL}.  */
-  std::string m_storage;
+  /* The argument vector built.  */
+  std::vector<char *> m_argv;
 };
 
 /* Create argument vector for straight call to execvp.  Breaks up
    ALLARGS into an argument vector suitable for passing to execvp and
    stores it in M_ARGV.  E.g., on "run a b c d" this routine would get
    as input the string "a b c d", and as output it would fill in
-   M_ARGV with the four arguments "a", "b", "c", "d".  Each argument
-   in M_ARGV points to a substring of a copy of ALLARGS stored in
-   M_STORAGE.  */
+   M_ARGV with the four arguments "a", "b", "c", "d".  */
 
 void
 execv_argv::init_for_no_shell (const char *exec_file,
 			       const std::string &allargs)
 {
+  m_argv.push_back (xstrdup (exec_file));
 
-  /* Save/work with a copy stored in our storage.  The pointers pushed
-     to M_ARGV point directly into M_STORAGE, which is modified in
-     place with the necessary NULL terminators.  This avoids N heap
-     allocations and string dups when 1 is sufficient.  */
-  std::string &args_copy = m_storage = allargs;
+  char **argv = buildargv (allargs.c_str ());
 
-  m_argv.push_back (exec_file);
-
-  for (size_t cur_pos = 0; cur_pos < args_copy.size ();)
+  for (int i = 0; argv[i] != nullptr; i++)
     {
-      /* Skip whitespace-like chars.  */
-      std::size_t pos = args_copy.find_first_not_of (" \t\n", cur_pos);
-
-      if (pos != std::string::npos)
-	cur_pos = pos;
-
-      /* Find the position of the next separator.  */
-      std::size_t next_sep = args_copy.find_first_of (" \t\n", cur_pos);
-
-      if (next_sep == std::string::npos)
-	{
-	  /* No separator found, which means this is the last
-	     argument.  */
-	  next_sep = args_copy.size ();
-	}
-      else
-	{
-	  /* Replace the separator with a terminator.  */
-	  args_copy[next_sep++] = '\0';
-	}
-
-      m_argv.push_back (&args_copy[cur_pos]);
-
-      cur_pos = next_sep;
+      m_argv.push_back (xstrdup (argv[i]));
     }
+  freeargv (argv);
 
   /* NULL-terminate the vector.  */
   m_argv.push_back (NULL);
@@ -189,11 +160,7 @@ execv_argv::init_for_shell (const char *exec_file,
   /* We're going to call a shell.  */
   bool escape_bang = escape_bang_in_quoted_argument (shell_file);
 
-  /* We need to build a new shell command string, and make argv point
-     to it.  So build it in the storage.  */
-  std::string &shell_command = m_storage;
-
-  shell_command = "exec ";
+  std::string shell_command = "exec ";
 
   /* Add any exec wrapper.  That may be a program name with arguments,
      so the user must handle quoting.  */
@@ -263,9 +230,9 @@ execv_argv::init_for_shell (const char *exec_file,
      "-c" says to interpret the next arg as a shell command to
      execute, and this command is "exec <target-program> <args>".  */
   m_argv.reserve (4);
-  m_argv.push_back (shell_file);
-  m_argv.push_back ("-c");
-  m_argv.push_back (shell_command.c_str ());
+  m_argv.push_back (xstrdup (shell_file));
+  m_argv.push_back (xstrdup ("-c"));
+  m_argv.push_back (xstrdup (shell_command.c_str ()));
   m_argv.push_back (NULL);
 }
 
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
index 0a5346c8aba..fe9053abac0 100644
--- a/gdb/testsuite/gdb.base/inferior-args.exp
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -27,7 +27,7 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
     return
 }
 
-proc do_test { method } {
+proc do_test { method startup_with_shell } {
     global binfile hex
 
     # The second arg is an empty string on purpose.
@@ -35,6 +35,8 @@ proc do_test { method } {
 
     clean_restart $binfile
 
+    gdb_test_no_output "set startup-with-shell $startup_with_shell"
+
     if { $method == "start" } {
 	# The start command does not make sense for a stub.
 	if { [use_gdb_stub] } {
@@ -119,5 +121,7 @@ proc do_test { method } {
 }
 
 foreach_with_prefix method { "start" "starti" "run" "set args" } {
-    do_test $method
+    foreach_with_prefix startup_with_shell { "on" "off" } {
+        do_test $method $startup_with_shell
+    }
 }
-- 
2.33.0

[PING] [PATCH 0/8] Unify escaping/quoting of args in GDB/GDBserver

[Michael Weghorn]

Ping.

On 22/10/2021 09.19, Michael Weghorn wrote:
> Historically, args passed directly on 'gdb --args <args>' invocation
> were escaped (when the inferior is started in a shell), while those passed
> via 'gdbserver <args>' were not.
> 
> That was changed in commit bea571ebd78ee29cb94adf648fbcda1e109e1be6
> ("Use construct_inferior_arguments which handles special chars",
> 2020-05-25), so args passed on 'gdbserver <args>' invocation were	
> quoted as well.
> That commit accidently also resulted in args set in target
> extended-remote ('set args <args>', 'run <args>')
> to be passed to the shell in a quoted way.
> 
> Based on the disussion in PR 28392
> ("gdb server no longer supports argument globbing and variable substitution"),
> this patch series addresses this and further unifies the behaviour of
> how args (set either directly on GDB/GDBserver invocation or otherwise)
> are treated between GDB and GDBserver, also adding a
> '--no-escape-args' command line option for GDB and
> GDBserver to explicitly disable escaping of args passed directly
> on invocation.
> It also makes handling args with spaces work in GDBserver
> when the 'startup-with-shell' option is off.
> 
> (Adding more tests to explicitly test some of the edge cases
> in the testsuite probably makes sense once it is clear whether
> the approach taken here is considered appropriate.)
> 
> Michael Weghorn (8):
>    gdbsupport: Extract escaping from 'construct_inferior_arguments'
>    gdbsupport: Make escaping in construct_inferior_arguments optional
>    PR27989 PR28446 gdbserver: Only escape args passed directly on
>      invocation
>    PR28392 Add a '--no-escape-args' option for gdb and gdbserver
>    Extract helper function from 'fork_inferior'
>    Add a 'fork_inferior' overload taking vector of args
>    gdbserver: Support args with spaces for no-startup-with-shell case
>    gdb: Support some escaping of args with startup-with-shell being off
> 
>   gdb/fork-child.c                         |   2 +-
>   gdb/infcmd.c                             |   4 +-
>   gdb/inferior.h                           |   2 +-
>   gdb/main.c                               |   6 +-
>   gdb/nat/fork-inferior.c                  | 152 +++++++++++++----------
>   gdb/nat/fork-inferior.h                  |  18 ++-
>   gdb/remote.c                             |  33 +++--
>   gdb/testsuite/gdb.base/args.exp          |  23 +---
>   gdb/testsuite/gdb.base/inferior-args.exp |   8 +-
>   gdbserver/fork-child.cc                  |  10 +-
>   gdbserver/linux-low.cc                   |   4 +-
>   gdbserver/netbsd-low.cc                  |   4 +-
>   gdbserver/server.cc                      |  74 +++++------
>   gdbserver/win32-low.cc                   |   4 +-
>   gdbsupport/common-inferior.cc            | 140 +++++++++++----------
>   gdbsupport/common-inferior.h             |  16 ++-
>   16 files changed, 276 insertions(+), 224 deletions(-)
> 

[PING 2] [PATCH 0/8] Unify escaping/quoting of args in GDB/GDBserver

[Michael Weghorn]

Ping.

On 06/12/2021 11.47, Michael Weghorn wrote:
> Ping.
> 
> On 22/10/2021 09.19, Michael Weghorn wrote:
>> Historically, args passed directly on 'gdb --args <args>' invocation
>> were escaped (when the inferior is started in a shell), while those 
>> passed
>> via 'gdbserver <args>' were not.
>>
>> That was changed in commit bea571ebd78ee29cb94adf648fbcda1e109e1be6
>> ("Use construct_inferior_arguments which handles special chars",
>> 2020-05-25), so args passed on 'gdbserver <args>' invocation were
>> quoted as well.
>> That commit accidently also resulted in args set in target
>> extended-remote ('set args <args>', 'run <args>')
>> to be passed to the shell in a quoted way.
>>
>> Based on the disussion in PR 28392
>> ("gdb server no longer supports argument globbing and variable 
>> substitution"),
>> this patch series addresses this and further unifies the behaviour of
>> how args (set either directly on GDB/GDBserver invocation or otherwise)
>> are treated between GDB and GDBserver, also adding a
>> '--no-escape-args' command line option for GDB and
>> GDBserver to explicitly disable escaping of args passed directly
>> on invocation.
>> It also makes handling args with spaces work in GDBserver
>> when the 'startup-with-shell' option is off.
>>
>> (Adding more tests to explicitly test some of the edge cases
>> in the testsuite probably makes sense once it is clear whether
>> the approach taken here is considered appropriate.)
>>
>> Michael Weghorn (8):
>>    gdbsupport: Extract escaping from 'construct_inferior_arguments'
>>    gdbsupport: Make escaping in construct_inferior_arguments optional
>>    PR27989 PR28446 gdbserver: Only escape args passed directly on
>>      invocation
>>    PR28392 Add a '--no-escape-args' option for gdb and gdbserver
>>    Extract helper function from 'fork_inferior'
>>    Add a 'fork_inferior' overload taking vector of args
>>    gdbserver: Support args with spaces for no-startup-with-shell case
>>    gdb: Support some escaping of args with startup-with-shell being off
>>
>>   gdb/fork-child.c                         |   2 +-
>>   gdb/infcmd.c                             |   4 +-
>>   gdb/inferior.h                           |   2 +-
>>   gdb/main.c                               |   6 +-
>>   gdb/nat/fork-inferior.c                  | 152 +++++++++++++----------
>>   gdb/nat/fork-inferior.h                  |  18 ++-
>>   gdb/remote.c                             |  33 +++--
>>   gdb/testsuite/gdb.base/args.exp          |  23 +---
>>   gdb/testsuite/gdb.base/inferior-args.exp |   8 +-
>>   gdbserver/fork-child.cc                  |  10 +-
>>   gdbserver/linux-low.cc                   |   4 +-
>>   gdbserver/netbsd-low.cc                  |   4 +-
>>   gdbserver/server.cc                      |  74 +++++------
>>   gdbserver/win32-low.cc                   |   4 +-
>>   gdbsupport/common-inferior.cc            | 140 +++++++++++----------
>>   gdbsupport/common-inferior.h             |  16 ++-
>>   16 files changed, 276 insertions(+), 224 deletions(-)
>>

[PING 3] [PATCH 0/8] Unify escaping/quoting of args in GDB/GDBserver

[Michael Weghorn]

Ping.

On 13/12/2021 08.49, Michael Weghorn wrote:
> Ping.
> 
> On 06/12/2021 11.47, Michael Weghorn wrote:
>> Ping.
>>
>> On 22/10/2021 09.19, Michael Weghorn wrote:
>>> Historically, args passed directly on 'gdb --args <args>' invocation
>>> were escaped (when the inferior is started in a shell), while those 
>>> passed
>>> via 'gdbserver <args>' were not.
>>>
>>> That was changed in commit bea571ebd78ee29cb94adf648fbcda1e109e1be6
>>> ("Use construct_inferior_arguments which handles special chars",
>>> 2020-05-25), so args passed on 'gdbserver <args>' invocation were
>>> quoted as well.
>>> That commit accidently also resulted in args set in target
>>> extended-remote ('set args <args>', 'run <args>')
>>> to be passed to the shell in a quoted way.
>>>
>>> Based on the disussion in PR 28392
>>> ("gdb server no longer supports argument globbing and variable 
>>> substitution"),
>>> this patch series addresses this and further unifies the behaviour of
>>> how args (set either directly on GDB/GDBserver invocation or otherwise)
>>> are treated between GDB and GDBserver, also adding a
>>> '--no-escape-args' command line option for GDB and
>>> GDBserver to explicitly disable escaping of args passed directly
>>> on invocation.
>>> It also makes handling args with spaces work in GDBserver
>>> when the 'startup-with-shell' option is off.
>>>
>>> (Adding more tests to explicitly test some of the edge cases
>>> in the testsuite probably makes sense once it is clear whether
>>> the approach taken here is considered appropriate.)
>>>
>>> Michael Weghorn (8):
>>>    gdbsupport: Extract escaping from 'construct_inferior_arguments'
>>>    gdbsupport: Make escaping in construct_inferior_arguments optional
>>>    PR27989 PR28446 gdbserver: Only escape args passed directly on
>>>      invocation
>>>    PR28392 Add a '--no-escape-args' option for gdb and gdbserver
>>>    Extract helper function from 'fork_inferior'
>>>    Add a 'fork_inferior' overload taking vector of args
>>>    gdbserver: Support args with spaces for no-startup-with-shell case
>>>    gdb: Support some escaping of args with startup-with-shell being off
>>>
>>>   gdb/fork-child.c                         |   2 +-
>>>   gdb/infcmd.c                             |   4 +-
>>>   gdb/inferior.h                           |   2 +-
>>>   gdb/main.c                               |   6 +-
>>>   gdb/nat/fork-inferior.c                  | 152 +++++++++++++----------
>>>   gdb/nat/fork-inferior.h                  |  18 ++-
>>>   gdb/remote.c                             |  33 +++--
>>>   gdb/testsuite/gdb.base/args.exp          |  23 +---
>>>   gdb/testsuite/gdb.base/inferior-args.exp |   8 +-
>>>   gdbserver/fork-child.cc                  |  10 +-
>>>   gdbserver/linux-low.cc                   |   4 +-
>>>   gdbserver/netbsd-low.cc                  |   4 +-
>>>   gdbserver/server.cc                      |  74 +++++------
>>>   gdbserver/win32-low.cc                   |   4 +-
>>>   gdbsupport/common-inferior.cc            | 140 +++++++++++----------
>>>   gdbsupport/common-inferior.h             |  16 ++-
>>>   16 files changed, 276 insertions(+), 224 deletions(-)
>>>

Re: [PING 3] [PATCH 0/8] Unify escaping/quoting of args in GDB/GDBserver

[Simon Marchi]

Hi,

I just want to acknowledge this patchset, which fixes some known bugs, thanks
for sending it.  The problems with escaping are really tricky and need careful
attention and testing while reviewing.  I don't have that time right now, and
I also don't want to make a half-assed review.  So if anybody has some time to
get familiar with the problem (explained in the bugzilla ticket) and help
review this, it would be really appreciated.

Simon

On 2022-01-03 23:22, Michael Weghorn via Gdb-patches wrote:
> Ping.
> 
> On 13/12/2021 08.49, Michael Weghorn wrote:
>> Ping.
>>
>> On 06/12/2021 11.47, Michael Weghorn wrote:
>>> Ping.
>>>
>>> On 22/10/2021 09.19, Michael Weghorn wrote:
>>>> Historically, args passed directly on 'gdb --args <args>' invocation
>>>> were escaped (when the inferior is started in a shell), while those passed
>>>> via 'gdbserver <args>' were not.
>>>>
>>>> That was changed in commit bea571ebd78ee29cb94adf648fbcda1e109e1be6
>>>> ("Use construct_inferior_arguments which handles special chars",
>>>> 2020-05-25), so args passed on 'gdbserver <args>' invocation were
>>>> quoted as well.
>>>> That commit accidently also resulted in args set in target
>>>> extended-remote ('set args <args>', 'run <args>')
>>>> to be passed to the shell in a quoted way.
>>>>
>>>> Based on the disussion in PR 28392
>>>> ("gdb server no longer supports argument globbing and variable substitution"),
>>>> this patch series addresses this and further unifies the behaviour of
>>>> how args (set either directly on GDB/GDBserver invocation or otherwise)
>>>> are treated between GDB and GDBserver, also adding a
>>>> '--no-escape-args' command line option for GDB and
>>>> GDBserver to explicitly disable escaping of args passed directly
>>>> on invocation.
>>>> It also makes handling args with spaces work in GDBserver
>>>> when the 'startup-with-shell' option is off.
>>>>
>>>> (Adding more tests to explicitly test some of the edge cases
>>>> in the testsuite probably makes sense once it is clear whether
>>>> the approach taken here is considered appropriate.)
>>>>
>>>> Michael Weghorn (8):
>>>>    gdbsupport: Extract escaping from 'construct_inferior_arguments'
>>>>    gdbsupport: Make escaping in construct_inferior_arguments optional
>>>>    PR27989 PR28446 gdbserver: Only escape args passed directly on
>>>>      invocation
>>>>    PR28392 Add a '--no-escape-args' option for gdb and gdbserver
>>>>    Extract helper function from 'fork_inferior'
>>>>    Add a 'fork_inferior' overload taking vector of args
>>>>    gdbserver: Support args with spaces for no-startup-with-shell case
>>>>    gdb: Support some escaping of args with startup-with-shell being off
>>>>
>>>>   gdb/fork-child.c                         |   2 +-
>>>>   gdb/infcmd.c                             |   4 +-
>>>>   gdb/inferior.h                           |   2 +-
>>>>   gdb/main.c                               |   6 +-
>>>>   gdb/nat/fork-inferior.c                  | 152 +++++++++++++----------
>>>>   gdb/nat/fork-inferior.h                  |  18 ++-
>>>>   gdb/remote.c                             |  33 +++--
>>>>   gdb/testsuite/gdb.base/args.exp          |  23 +---
>>>>   gdb/testsuite/gdb.base/inferior-args.exp |   8 +-
>>>>   gdbserver/fork-child.cc                  |  10 +-
>>>>   gdbserver/linux-low.cc                   |   4 +-
>>>>   gdbserver/netbsd-low.cc                  |   4 +-
>>>>   gdbserver/server.cc                      |  74 +++++------
>>>>   gdbserver/win32-low.cc                   |   4 +-
>>>>   gdbsupport/common-inferior.cc            | 140 +++++++++++----------
>>>>   gdbsupport/common-inferior.h             |  16 ++-
>>>>   16 files changed, 276 insertions(+), 224 deletions(-)
>>>>