Warn the user when $SHELL is invalid
Commit Message
After a lot of discussion (really, I did not know this patch would
draw so much attention), this patch has now been simplified (mostly
because of Doug's suggestions).
It is known that GDB needs a valid shell to start the inferior and to
offer the "shell" command to the user. This has recently been the
cause of a problem on the MIPS buildslave, because $SHELL was set to
/sbin/nologin and several tests were failing. The thread is here:
<https://sourceware.org/ml/gdb-patches/2015-07/msg00535.html>
In order to improve the error/warning messages emitted in this
scenario, this patch proposes a new check to be performed before
executing the inferior (or before executing a program using the
"shell" command on GDB). This new check tries to determine if the
specified $SHELL is valid or not. The default behavior has not been
changed: if $SHELL is invalid, GDB will still try to execute a program
using it, which will lead to an error eventually. However, now GDB
will also display a message to the user saying that $SHELL is invalid.
This should help when determining what went wrong.
The check for a valid shell is simple and has been proposed by Doug.
We just fork and then exec a simple command:
$SHELL -c 'exit 42'
If the return value is 42, then $SHELL is valid. Otherwise we issue
the warning. It is obviously trivial to create a program that returns
42 and ignores its arguments, and it it obvious that this program
would be considered a valid shell by GDB, but this should not cause
any problem; the only "drawback" for the user is that she wouldn't get
the warning message before the error.
OK to apply?
Changes from v3:
- Implemented new way to check if $SHELL is valid: we fork, and
execute a simple command ('exit 42') to see if the shell supports
executing things.
- Instead of defaulting to /bin/sh when $SHELL is invalid, just emit
a warning to the user.
- Removed documentation parts. They are not needed anymore now that
the patch does not change the default behavior.
Changes from v2:
- Rewrote "Valid Shell" section in the documentation to mention that
the tests performed are not exhaustive. Included a small example
to demonstrate what happens if the user tries to use /bin/ls as
the $SHELL (a "valid shell", in theory).
Changes from v1:
- Using @pxref instead of @ref in the documentation
- Don't run the testcase when the target is mingw, cygwin, or remote
- Including /usr/sbin/nologin and /bin/false in the list of invalid
shells
gdb/ChangeLog:
2015-07-28 Sergio Durigan Junior <sergiodj@redhat.com>
* cli/cli-cmds.c (shell_escape): Check if the selected shell is
valid.
* fork-child.c (check_startup_with_shell): New function.
(fork_inferior): Move code to the new function above. Call it.
* utils.c: Include "fileutils.h".
(valid_shell): New function.
* utils.h (valid_shell): New prototype.
gdb/testsuite/ChangeLog:
2015-07-28 Sergio Durigan Junior <sergiodj@redhat.com>
* gdb.base/invalid-shell.exp: New file.
---
gdb/cli/cli-cmds.c | 6 +++
gdb/fork-child.c | 67 +++++++++++++++++++++++++-------
gdb/testsuite/gdb.base/invalid-shell.exp | 48 +++++++++++++++++++++++
gdb/utils.c | 53 +++++++++++++++++++++++++
gdb/utils.h | 16 ++++++++
5 files changed, 177 insertions(+), 13 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/invalid-shell.exp
Comments
On 07/28/2015 08:58 PM, Sergio Durigan Junior wrote:
> After a lot of discussion (really, I did not know this patch would
> draw so much attention), this patch has now been simplified (mostly
> because of Doug's suggestions).
>
> It is known that GDB needs a valid shell to start the inferior and to
> offer the "shell" command to the user. This has recently been the
> cause of a problem on the MIPS buildslave, because $SHELL was set to
> /sbin/nologin and several tests were failing. The thread is here:
>
> <https://sourceware.org/ml/gdb-patches/2015-07/msg00535.html>
>
> In order to improve the error/warning messages emitted in this
> scenario, this patch proposes a new check to be performed before
> executing the inferior (or before executing a program using the
> "shell" command on GDB). This new check tries to determine if the
> specified $SHELL is valid or not. The default behavior has not been
> changed: if $SHELL is invalid, GDB will still try to execute a program
> using it, which will lead to an error eventually. However, now GDB
> will also display a message to the user saying that $SHELL is invalid.
> This should help when determining what went wrong.
>
> The check for a valid shell is simple and has been proposed by Doug.
> We just fork and then exec a simple command:
>
> $SHELL -c 'exit 42'
>
> If the return value is 42, then $SHELL is valid. Otherwise we issue
> the warning. It is obviously trivial to create a program that returns
> 42 and ignores its arguments, and it it obvious that this program
> would be considered a valid shell by GDB, but this should not cause
> any problem; the only "drawback" for the user is that she wouldn't get
> the warning message before the error.
>
> OK to apply?
As mentioned in the other thread, myself, I'm not convinced of the
value of the extra fork/exit complexity for this. IMO, a wider
potential bug surface for a not-so-clear benefit. We currently get:
$ SHELL=/nonexisting gdb /home/pedro/a.out
(gdb) r
Starting program: /home/pedro/a.out
Cannot exec /nonexisting -c exec /home/pedro/a.out .
Error: No such file or directory
During startup program exited with code 127.
(gdb)
If we're starting with a shell, then if the exec fails, it was
obviously because execing the shell failed. I'd suggest simply trying
to make the error message clearer. E.g.,:
$ SHELL=/nonexisting gdb /home/pedro/a.out
(gdb) r
Starting program: /home/pedro/a.out
"set startup-with-shell" is on, but failed to exec:
/nonexisting -c exec /home/pedro/a.out
Error: No such file or directory
If set, the SHELL environment variable must point at a valid shell.
SHELL is currently set to "/nonexisting".
During startup program exited with code 127.
(gdb)
Thanks,
Pedro Alves
On Tuesday, July 28 2015, Pedro Alves wrote:
> As mentioned in the other thread, myself, I'm not convinced of the
> value of the extra fork/exit complexity for this. IMO, a wider
> potential bug surface for a not-so-clear benefit. We currently get:
>
> $ SHELL=/nonexisting gdb /home/pedro/a.out
> (gdb) r
> Starting program: /home/pedro/a.out
> Cannot exec /nonexisting -c exec /home/pedro/a.out .
> Error: No such file or directory
> During startup program exited with code 127.
> (gdb)
>
> If we're starting with a shell, then if the exec fails, it was
> obviously because execing the shell failed. I'd suggest simply trying
> to make the error message clearer. E.g.,:
>
> $ SHELL=/nonexisting gdb /home/pedro/a.out
> (gdb) r
> Starting program: /home/pedro/a.out
> "set startup-with-shell" is on, but failed to exec:
> /nonexisting -c exec /home/pedro/a.out
> Error: No such file or directory
> If set, the SHELL environment variable must point at a valid shell.
> SHELL is currently set to "/nonexisting".
> During startup program exited with code 127.
> (gdb)
Yeah, if it's just to warn the user, then I agree that it should be
possible to extend the existing error message. I'll send a patch for
this later.
Thanks,
@@ -754,6 +754,12 @@ shell_escape (char *arg, int from_tty)
if ((user_shell = (char *) getenv ("SHELL")) == NULL)
user_shell = "/bin/sh";
+ else
+ {
+ if (!valid_shell (user_shell))
+ warning (_("The specified shell '%s' is not valid. The command "
+ "will not be executed."), user_shell);
+ }
/* Get the name of the shell for arg0. */
p = lbasename (user_shell);
@@ -108,6 +108,58 @@ escape_bang_in_quoted_argument (const char *shell_file)
return 0;
}
+/* Check if the user has specified any shell to start the inferior,
+ and if the specified shell is valid (i.e., it exists and can be
+ executed by the user).
+
+ The shell specified by the user, if any, is SHELL_FILE_ARG.
+
+ DEFAULT_SHELL_FILE is the default shell that will be used by GDB if
+ the user said she wants to start the inferior using a shell (i.e.,
+ STARTUP_WITH_SHELL is 1) but no shell has been specified by the
+ user (i.e., the $SHELL environment variable is NULL).
+
+ If the specified shell is invalid (see utils.h:valid_shell for
+ details), this function will print a warning to the user, but *will
+ still return 1*.
+
+ Return 1 if the STARTUP_WITH_SHELL is set (even if the specified
+ shell is invalid); zero otherwise. */
+
+static int
+check_startup_with_shell (char **shell_file, char *shell_file_arg,
+ char *default_shell_file)
+{
+ /* 'startup_with_shell' is declared in inferior.h and bound to the
+ "set startup-with-shell" option. If 0, we'll just do a
+ fork/exec, no shell, so don't bother figuring out what shell. */
+ if (startup_with_shell)
+ {
+ gdb_assert (shell_file != NULL);
+ gdb_assert (default_shell_file != NULL);
+ *shell_file = shell_file_arg;
+
+ /* Figure out what shell to start up the user program under. */
+ if (*shell_file == NULL)
+ *shell_file = getenv ("SHELL");
+ if (*shell_file == NULL)
+ {
+ /* No shell was specified, so we just stick to the default
+ behavior which is to use DEFAULT_SHELL_FILE. */
+ *shell_file = default_shell_file;
+ }
+ else if (!valid_shell (*shell_file))
+ warning (_("The specified shell '%s' is not valid. You will see\n"
+ "an error when trying to execute the inferior."),
+ *shell_file);
+
+ /* We return 1 here even though the shell might be invalid. */
+ return 1;
+ }
+
+ return 0;
+}
+
/* Start an inferior Unix child process and sets inferior_ptid to its
pid. EXEC_FILE is the file to run. ALLARGS is a string containing
the arguments to the program. ENV is the environment vector to
@@ -148,19 +200,8 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
if (exec_file == 0)
exec_file = get_exec_file (1);
- /* 'startup_with_shell' is declared in inferior.h and bound to the
- "set startup-with-shell" option. If 0, we'll just do a
- fork/exec, no shell, so don't bother figuring out what shell. */
- shell_file = shell_file_arg;
- if (startup_with_shell)
- {
- /* Figure out what shell to start up the user program under. */
- if (shell_file == NULL)
- shell_file = getenv ("SHELL");
- if (shell_file == NULL)
- shell_file = default_shell_file;
- shell = 1;
- }
+ shell = check_startup_with_shell (&shell_file, shell_file_arg,
+ default_shell_file);
if (!shell)
{
new file mode 100644
@@ -0,0 +1,48 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile normal.c
+
+if { [is_remote target] || ![isnative] } {
+ untested "cannot test on remote targets"
+ return -1
+}
+
+if { [istarget "*-*-mingw*"] || [istarget "*-*-cygwin*"] } {
+ untested "cannot test on mingw or cygwin"
+ return -1
+}
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+ untested "could not compile test program"
+ return -1
+}
+
+gdb_exit
+
+# Set the $SHELL to an invalid file. This will cause GDB to use
+# /bin/sh instead.
+set oldshell $env(SHELL)
+set env(SHELL) "/invalid/path/to/file"
+
+clean_restart $binfile
+
+# Running the inferior must work.
+gdb_test "run" "Starting program: .*\r\nwarning: The specified shell \\\'/invalid/path/to/file\\\' is not valid. You will see\r\nan error when trying to execute the inferior.\r\nCannot exec /invalid/path/to/file -c exec .*$testfile .\r\nError: No such file or directory\r\nDuring startup program exited with code $decimal." "starting with /bin/sh instead of invalid shell"
+
+# Invoking a shell command must also work.
+gdb_test "shell echo hi" "warning: The specified shell \\\'/invalid/path/to/file\\\' is not valid. The command will not be executed.\r\nCannot execute /invalid/path/to/file: No such file or directory" "invoking shell command from prompt"
+
+set env(SHELL) $oldshell
@@ -25,6 +25,7 @@
#include "gdbthread.h"
#include "fnmatch.h"
#include "gdb_bfd.h"
+#include "filestuff.h"
#ifdef HAVE_SYS_RESOURCE_H
#include <sys/resource.h>
#endif /* HAVE_SYS_RESOURCE_H */
@@ -3364,6 +3365,58 @@ gdb_filename_fnmatch (const char *pattern, const char *string, int flags)
return fnmatch (pattern, string, flags);
}
+/* See utils.h. */
+
+int
+valid_shell (const char *shell)
+{
+ pid_t shell_pid;
+ int status;
+ size_t len = strlen (shell);
+ char *shell_argv[4];
+
+ if (shell == NULL || access (shell, X_OK) != 0)
+ return 0;
+
+ shell_argv[0] = alloca (len + 1);
+ strncpy (shell_argv[0], shell, len);
+ shell_argv[0][len] = '\0';
+ shell_argv[1] = "-c";
+ shell_argv[2] = "exit 42";
+ shell_argv[3] = (char *) NULL;
+ shell_pid = fork ();
+
+ switch (shell_pid)
+ {
+ case -1:
+ /* vfork failed. Let's just return 0 because we could not
+ figure out if the shell is valid or not. */
+ return 0;
+
+ case 0:
+ /* Closing everything as we will not need (and don't want) to
+ output any message. */
+ close_most_fds ();
+ close (STDOUT_FILENO);
+ close (STDERR_FILENO);
+ close (STDIN_FILENO);
+ execvp (shell_argv[0], shell_argv);
+ /* If we get here, it's an error. Just return an invalid
+ thing. */
+ _exit (0177);
+
+ default:
+ waitpid (shell_pid, &status, 0);
+ if (WIFEXITED (status))
+ return WEXITSTATUS (status) == 42;
+ else
+ {
+ kill (SIGKILL, shell_pid);
+ return 0;
+ }
+ }
+}
+
/* Provide a prototype to silence -Wmissing-prototypes. */
extern initialize_file_ftype _initialize_utils;
@@ -369,4 +369,20 @@ extern void dump_core (void);
extern char *make_hex_string (const gdb_byte *data, size_t length);
+/* Return 1 if SHELL is a valid shell to execute a command, zero
+ otherwise.
+
+ In oder to perform the check for a valid shell, this function will
+ fork GDB, and then execute a simple command: $SHELL -c 'exit 42'.
+ This command shall make the SHELL return 42; if this number is not
+ returned, it (probably) means that SHELL is not valid. We say
+ 'probably' because it is trivial to create a program that just
+ returns 42 and ignores its arguments; in this case, this function
+ would consider it as a valid shell even though it is clearly not.
+ But even in this case GDB would later fail to execute anything, so
+ that only means that the user would not get a nice warning message
+ in this case. */
+
+extern int valid_shell (const char *shell);
+
#endif /* UTILS_H */