From: Andrew Burgess Date: Fri, 15 Dec 2023 12:46:14 +0000 (+0000) Subject: gdb: remove the !startup_with_shell path from construct_inferior_arguments X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=710f7df7da31dc6b81cdbdd7256005c51798260a;p=thirdparty%2Fbinutils-gdb.git gdb: remove the !startup_with_shell path from construct_inferior_arguments In the commit: commit 0df62bf09ecf242e3a932255d24ee54407b3c593 Date: Fri Oct 22 07:19:33 2021 +0000 gdb: Support some escaping of args with startup-with-shell being off nat/fork-inferior.c was updated such that when we are starting an inferior without a shell we now remove escape characters. The benefits of this are explained in that commit, but having made this change we can now make an additional change. Currently, in construct_inferior_arguments, when startup_with_shell is false we construct the inferior argument string differently than when startup_with_shell is true; when true we apply some escaping to special shell character, when false we don't. This commit simplifies construct_inferior_arguments by removing the !startup_with_shell case, and instead we now apply escaping in all cases. This is fine because, thanks to the above commit the escaping will be correctly removed again when we call into nat/fork-inferior.c. We should think of construct_inferior_arguments and nat/fork-inferior.c as needing to cooperate in order for argument handling to work correctly. construct_inferior_arguments converts a list of separate arguments into a single string, and nat/fork-inferior.c splits that single string back into a list of arguments. It is critical that, if nat/fork-inferior.c is expecting to remove a "layer" of escapes, then construct_inferior_arguments must add that expected "layer", otherwise, we end up stripping more escapes than expected. The great thing (I think) about the new configuration, is that GDB no longer cares about startup_with_shell at the point the arguments are being setup. We only care about startup_with_shell at the point that the inferior is started. This means that a user can set the inferior arguments, and then change the startup-with-shell setting, and GDB will do what they expect. Under the previous system, where construct_inferior_arguments changed its behaviour based on startup_with_shell, the user had to change the setting, and then set the arguments, otherwise, GDB might not do what they expect. There is one slight issue with this commit though, which will be addressed by the next commit. For GDB's native targets construct_inferior_arguments is reached via two code paths; first when GDB starts and we combine arguments from the command line, and second when the Python API is used to set the arguments from a sequence. It's the command line argument handling which we are interested in. Consider this: $ gdb --args /tmp/exec '$FOO' (gdb) show args Argument list to give program being debugged when it is started is "\$FOO". Notice that the argument has become \$FOO, the '$' is now quoted. This is because, by quoting the argument in the shell command that started GDB, GDB was passed a literal $FOO with no quotes. In order to ensure that the inferior sees this same value, GDB added the extra escape character. When GDB starts with a shell we pass \$FOO, which results in the inferior seeing a literal $FOO. But what if the user _actually_ wanted to have the shell GDB uses to start the inferior expand $FOO? Well, it appears this can't be done from the command line, but from the GDB prompt we can just do: (gdb) set args $FOO (gdb) show args Argument list to give program being debugged when it is started is "$FOO". And now the inferior will see the shell expanded version of $FOO. It might seem like we cannot achieve the same result from the GDB command line, however, it is possible with this trick: $ gdb -eiex 'set startup-with-shell off' --args /tmp/exec '$FOO' (gdb) show args Argument list to give program being debugged when it is started is "$FOO". (gdb) show startup-with-shell Use of shell to start subprocesses is off. And now the $FOO is not escaped, but GDB is no longer using a shell to start the inferior, however, we can extend our command line like this: $ gdb -eiex 'set startup-with-shell off' \ -ex 'set startup-with-shell on' \ --args /tmp/exec '$FOO' (gdb) show args Argument list to give program being debugged when it is started is "$FOO". (gdb) show startup-with-shell Use of shell to start subprocesses is on. Use an early-initialisation option to disable startup-with-shell, this is done before command line argument processing, then a normal initialisation option turns startup-with-shell back on after GDB has processed the command line arguments! Is this useful? Yes, absolutely. Is this a good user experience? Absolutely not. And I plan to add a new command line option to GDB (and gdbserver) that will allow users to achieve the same result (this trick doesn't work in gdbserver as there's no early-initialisation there) without having to toggle the startup-with-shell option. The new option can be found in the series here: https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com The problem is that, that series is pretty long, and getting it reviewed is just not possible. So instead I'm posting the individual patches in smaller blocks, to make reviews easier. So, what's the problem? Well, by removing the !startup_with_shell code path from GDB, there is no longer a construct_inferior_arguments code path that doesn't quote inferior arguments, and so there's no longer a way, from the command line, to set an unquoted '$FOO' as an inferior argument. Obviously, this can still be done from GDB's CLI prompt. The trick above is completely untested, so this regression isn't going to show up in the testsuite. And the breakage is only temporary. In the next commit I'll add a fix which restores the above trick. Of course, I hope that this fix will itself, only be temporary. Once the new command line options that I mentioned above are added, then the fix I add in the next commit can be removed, and user should start using the new command line option. After this commit a whole set of tests that were added as xfail in the above commit are now passing. A change similar to this one can be found in this series: https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/ which I reviewed before writing this patch. I don't think there's any one patch in that series that exactly corresponds with this patch though, so I've listed the author of the original series as co-author on this patch. Co-Authored-By: Michael Weghorn Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392 Tested-By: Guinevere Larsen --- diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp index 34d722a7941..363d74a1d60 100644 --- a/gdb/testsuite/gdb.base/args.exp +++ b/gdb/testsuite/gdb.base/args.exp @@ -29,15 +29,6 @@ if {[build_executable $testfile.exp $testfile $srcfile] == -1} { return -1 } -set startup_with_shell_modes { "on" } -if {![gdb_protocol_is_remote]} { - lappend startup_with_shell_modes "off" -} else { - # Some of these tests will not work when using the remote protocol - # due to bug PR gdb/28392. - unsupported "gdbserver 'startup-with-shell off' broken PR gdb/28392" -} - # NAME is the name to use for the tests and ARGLIST is the list of # arguments that are passed to GDB when it is started. # @@ -55,7 +46,7 @@ proc args_test { name arglist {re_list {}} } { set re_list $arglist } - foreach_with_prefix startup_with_shell $::startup_with_shell_modes { + foreach_with_prefix startup_with_shell { on off } { save_vars { ::GDBFLAGS } { set ::GDBFLAGS "$::GDBFLAGS --args $::binfile $arglist" diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp index 79b73e61b33..a1977dbc2e4 100644 --- a/gdb/testsuite/gdb.base/inferior-args.exp +++ b/gdb/testsuite/gdb.base/inferior-args.exp @@ -174,22 +174,48 @@ set bs "\\\\" lappend item [list "$hex \"$bs\"\"" "$hex \"$bs$bs$bs\"\""] lappend test_desc_list $item -set startup_with_shell_modes { "on" } -if {![gdb_protocol_is_remote]} { - lappend startup_with_shell_modes "off" -} else { - # Due to PR gdb/28392 gdbserver doesn't currently support having - # startup-with-shell off, and then attempting to pass arguments - # containing whitespace. - unsupported "bug gdb/28392: gdbserver doesn't support this" -} - +# test three +# ---------- +# +# This test focuses on sending special shell characters within a +# double quote argument, and each special character is prefixed with a +# backslash. +# +# In a POSIX shell, within a double quoted argument, only $ (dollar), +# ` (backtick), " (double quote), \ (backslash), and newline can be +# escaped. All other backslash characters are literal backslashes. +# +# As with the previous test, the double quotes are lost when the +# arguments are sent through gdbserver_start, as such, this test isn't +# going to work when using the native-gdbserver board, hence we set +# the second arguemnt to 'false'. +lappend test_desc_list [list "test three" \ + false \ + { "\&" "\<" "\#" "\^" "\>" "\$" "\`" } \ + [list "$hex \"\\\\\\\\&\"" \ + "$hex \"\\\\\\\\<\"" \ + "$hex \"\\\\\\\\#\"" \ + "$hex \"\\\\\\\\\\^\"" \ + "$hex \"\\\\\\\\>\"" \ + "$hex \"\\\$\"" \ + "$hex \"`\""]] + +# test four +# --------- +# +# This test passes two arguments, a single and double quote, each +# escaped with a backslash. +lappend test_desc_list [list "test four" \ + true \ + { \' \" } \ + [list "$hex \"'\"" \ + "$hex \"\\\\\"\""]] foreach desc $test_desc_list { lassign $desc name stub_suitable args re_list with_test_prefix $name { foreach_with_prefix set_method { "start" "starti" "run" "set args" } { - foreach_with_prefix startup_with_shell $startup_with_shell_modes { + foreach_with_prefix startup_with_shell { on off } { do_test $set_method $startup_with_shell $args $re_list \ $stub_suitable } diff --git a/gdb/testsuite/gdb.base/startup-with-shell.exp b/gdb/testsuite/gdb.base/startup-with-shell.exp index 495c43eeaee..e27f17aeb72 100644 --- a/gdb/testsuite/gdb.base/startup-with-shell.exp +++ b/gdb/testsuite/gdb.base/startup-with-shell.exp @@ -59,12 +59,8 @@ proc initial_setup_simple { startup_with_shell run_args } { # If PROBLEMATIC_ON is true then when startup-with-shell is on we # expect the comparison to fail, so setup an xfail. # -# If PROBLEMATIC_OFF is true then when startup-with-shell is off we -# expect the comparison to fail, so setup an xfail. -# # TESTNAME is a string used in the test names. -proc run_test { args on_re off_re testname { problematic_on false } \ - { problematic_off false } } { +proc run_test { args on_re off_re testname { problematic_on false } } { foreach startup_with_shell { "on" "off" } { with_test_prefix "$testname, startup_with_shell: ${startup_with_shell}" { if {![initial_setup_simple $startup_with_shell $args]} { @@ -76,7 +72,7 @@ proc run_test { args on_re off_re testname { problematic_on false } \ set problematic $problematic_on } else { set re $off_re - set problematic $problematic_off + set problematic false } if { $problematic } { @@ -91,9 +87,8 @@ proc run_test { args on_re off_re testname { problematic_on false } \ # This is like the run_test proc except that RE is used as the # expected argument regexp when startup-with-shell is both on and off. # For the other arguments, see run_test. -proc run_test_same { args re testname { problematic_on false } \ - { problematic_off false } } { - run_test $args $re $re $testname $problematic_on $problematic_off +proc run_test_same { args re testname } { + run_test $args $re $re $testname } # The regexp to match a single '\' character. @@ -129,13 +124,11 @@ save_vars { env(TEST) } { run_test_same "\"\\a\"" \ "\"${bs}${bs}a\"" \ - "retain backslash in double quote arg" \ - false $is_remote_p + "retain backslash in double quote arg" run_test_same "'\\a'" \ "\"${bs}${bs}a\"" \ - "retain backslash in single quote arg" \ - false $is_remote_p + "retain backslash in single quote arg" run_test_same "\"\\\$\"" \ "\"\\\$\"" \ @@ -143,8 +136,7 @@ run_test_same "\"\\\$\"" \ run_test_same "'\\\$'" \ "\"${bs}${bs}\\\$\"" \ - "'\$' is not escaped in single quote arg" \ - false $is_remote_p + "'\$' is not escaped in single quote arg" run_test_same "\"\\`\"" \ "\"\\`\"" \ @@ -152,25 +144,20 @@ run_test_same "\"\\`\"" \ run_test_same "'\\`'" \ "\"${bs}${bs}`\"" \ - "'`' is not escaped in single quote arg" \ - false $is_remote_p + "'`' is not escaped in single quote arg" run_test_same "\"\\\"\"" \ "\"${bs}\"\"" \ - "'\"' can be escaped in double quote arg" \ - false $is_remote_p + "'\"' can be escaped in double quote arg" run_test_same "'\\\"'" \ "\"${bs}${bs}${bs}\"\"" \ - "'\"' is not escaped in single quote arg" \ - false $is_remote_p + "'\"' is not escaped in single quote arg" run_test_same "\"\\\\\"" \ "\"${bs}${bs}\"" \ - "'\\' can be escaped in double quote arg" \ - false $is_remote_p + "'\\' can be escaped in double quote arg" run_test_same "'\\\\'" \ "\"${bs}${bs}${bs}${bs}\"" \ - "'\\' is not escaped in single quote arg" \ - false $is_remote_p + "'\\' is not escaped in single quote arg" diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc index 4211e049ba7..8e35f416e70 100644 --- a/gdbsupport/common-inferior.cc +++ b/gdbsupport/common-inferior.cc @@ -31,92 +31,66 @@ construct_inferior_arguments (gdb::array_view argv) { std::string result; - if (startup_with_shell) - { #ifdef __MINGW32__ - /* This holds all the characters considered special to the - Windows shells. */ - static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n"; - static const char quote = '"'; + /* This holds all the characters considered special to the + Windows shells. */ + static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n"; + static const char quote = '"'; #else - /* This holds all the characters considered special to the - typical Unix shells. We include `^' because the SunOS - /bin/sh treats it as a synonym for `|'. */ - static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n"; - static const char quote = '\''; + /* This holds all the characters considered special to the + typical Unix shells. We include `^' because the SunOS + /bin/sh treats it as a synonym for `|'. */ + static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n"; + static const char quote = '\''; #endif - for (int i = 0; i < argv.size (); ++i) + for (int i = 0; i < argv.size (); ++i) + { + if (i > 0) + result += ' '; + + /* Need to handle empty arguments specially. */ + if (argv[i][0] == '\0') { - if (i > 0) - result += ' '; + result += quote; + result += quote; + } + else + { +#ifdef __MINGW32__ + bool quoted = false; - /* Need to handle empty arguments specially. */ - if (argv[i][0] == '\0') + if (strpbrk (argv[i], special)) { - result += quote; + quoted = true; result += quote; } - else +#endif + for (char *cp = argv[i]; *cp != '\0'; ++cp) { -#ifdef __MINGW32__ - bool quoted = false; - - if (strpbrk (argv[i], special)) + if (*cp == '\n') { - quoted = true; + /* A newline cannot be quoted with a backslash (it + just disappears), only by putting it inside + quotes. */ + result += quote; + result += '\n'; result += quote; } -#endif - for (char *cp = argv[i]; *cp; ++cp) + else { - 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]; } }