This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH v3] Make sure GDB uses a valid shell when starting the inferior and to perform the "shell" command


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>

However, I think we can do better than that.  If 'startup-with-shell'
is on, which is the default, we blindly trust that the user will
provide a valid shell for us, and this may not be true all the time.
So I am proposing a patch to increment the tests made by GDB before
running the inferior to decide whether it will use $SHELL or not.
Particularly, I created a new function, called "valid_shell", which
defines the concept of a valid shell for GDB:

  - A file that exists and is executable by the user

  - A file that is not {,/usr}/sbin/nologin, nor /bin/false

For now, I think this is enough to cover most cases.  The default
action when an invalid shell is found is to use /bin/sh instead (we
already do that when $SHELL is not defined, for example), and also
issue a warning to the user.  This applies for when we are starting
the inferior and when we are executing the "shell" command.

To make things more robust, I made the code also check /bin/sh and
make sure it is also valid.  If it is not, and if we are starting the
inferior, then GDB will use fork+exec instead.  If we are executing
the "shell" command and we cannot find a valid shell, GDB will error
out.

I updated the documentation to reflect the new behavior, and created a
testcase to exercise the code.  This patch has been regression-tested
on Fedora 22 x86_64.

OK to apply?

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-24  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 (valid_shell): New function.
	* utils.h (valid_shell): New prototype.

gdb/doc/ChangeLog:
2015-07-24  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.texinfo (Shell Commands): Mention that the shell needs to be
	valid; point to "Valid Shell" subsection.
	(Valid Shell): New subsection.
	(Starting your Program): Mention that the shell needs to be valid;
	point to "Valid Shell" subsection.
	(Your Program's Arguments): Likewise.
	(Your Program's Environment): Likewise.

gdb/testsuite/ChangeLog:
2015-07-24  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/invalid-shell.exp: New file.
---
 gdb/cli/cli-cmds.c                       |  9 +++-
 gdb/doc/gdb.texinfo                      | 65 ++++++++++++++++++-------
 gdb/fork-child.c                         | 82 +++++++++++++++++++++++++++-----
 gdb/testsuite/gdb.base/invalid-shell.exp | 48 +++++++++++++++++++
 gdb/utils.c                              | 12 +++++
 gdb/utils.h                              | 13 +++++
 6 files changed, 198 insertions(+), 31 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/invalid-shell.exp

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 2ec2dd3..ed6a1df 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -752,8 +752,13 @@ shell_escape (char *arg, int from_tty)
 
       close_most_fds ();
 
-      if ((user_shell = (char *) getenv ("SHELL")) == NULL)
-	user_shell = "/bin/sh";
+      if ((user_shell = (char *) getenv ("SHELL")) == NULL
+	  || !valid_shell (user_shell))
+	{
+	  user_shell = "/bin/sh";
+	  if (!valid_shell (user_shell))
+	    error (_("Cannot use '%s' as a shell."), user_shell);
+	}
 
       /* Get the name of the shell for arg0.  */
       p = lbasename (user_shell);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9e2ecd1..6016fdf 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -1409,11 +1409,12 @@ just use the @code{shell} command.
 @cindex shell escape
 @item shell @var{command-string}
 @itemx !@var{command-string}
-Invoke a standard shell to execute @var{command-string}.
-Note that no space is needed between @code{!} and @var{command-string}.
-If it exists, the environment variable @code{SHELL} determines which
-shell to run.  Otherwise @value{GDBN} uses the default shell
-(@file{/bin/sh} on Unix systems, @file{COMMAND.COM} on MS-DOS, etc.).
+Invoke a standard shell to execute @var{command-string}.  Note that no
+space is needed between @code{!} and @var{command-string}.  If it
+exists and points to a valid shell (@pxref{Valid Shell,,}), the
+environment variable @code{SHELL} determines which shell to run.
+Otherwise @value{GDBN} uses the default shell (@file{/bin/sh} on Unix
+systems, @file{COMMAND.COM} on MS-DOS, etc.).
 @end table
 
 The utility @code{make} is often needed in development environments.
@@ -1428,6 +1429,36 @@ Execute the @code{make} program with the specified
 arguments.  This is equivalent to @samp{shell make @var{make-args}}.
 @end table
 
+@node Valid Shell
+@subsection Valid Shell
+
+@value{GDBN} considers a @emph{valid shell} a file that:
+
+@enumerate
+@item
+Exists and can be executed by the user.
+
+@item
+Is not a pseudo-shell used to disable logins, such as
+@file{/sbin/nologin} (or @file{/usr/sbin/nologin}) or
+@file{/bin/false}.
+@end enumerate
+
+Keep in mind that this check is not exhaustive and does not take into
+account every possible invalid shell.  This means that if the user
+sets a strange program like @file{/bin/ls} as the shell, @value{GDBN}
+will consider this program as a valid shell and will try to start the
+inferior using this erroneous shell, triggering an error like:
+
+@smallexample
+(@value{GDBP}) file /usr/bin/true
+...
+(@value{GDBP}) run
+Starting program: /usr/bin/true
+/bin/ls: cannot access exec /usr/bin/true : No such file or directory
+During startup program exited with code 2.
+@end smallexample
+
 @node Logging Output
 @section Logging Output
 @cindex logging @value{GDBN} output
@@ -2041,6 +2072,7 @@ is used to pass the arguments, so that you may use normal conventions
 the arguments.
 In Unix systems, you can control which shell is used with the
 @code{SHELL} environment variable.  If you do not define @code{SHELL},
+or if you define it to an invalid shell (@pxref{Valid Shell,,}),
 @value{GDBN} uses the default shell (@file{/bin/sh}).  You can disable
 use of any shell with the @code{set startup-with-shell} command (see
 below for details).
@@ -2286,9 +2318,10 @@ The arguments to your program can be specified by the arguments of the
 @code{run} command.
 They are passed to a shell, which expands wildcard characters and
 performs redirection of I/O, and thence to your program.  Your
-@code{SHELL} environment variable (if it exists) specifies what shell
-@value{GDBN} uses.  If you do not define @code{SHELL}, @value{GDBN} uses
-the default shell (@file{/bin/sh} on Unix).
+@code{SHELL} environment variable (if it exists and if it contains a
+valid shell---@pxref{Valid Shell,,}) specifies what shell
+@value{GDBN} uses.  If you do not define @code{SHELL}, @value{GDBN}
+uses the default shell (@file{/bin/sh} on Unix).
 
 On non-Unix systems, the program is usually invoked directly by
 @value{GDBN}, which emulates I/O redirection via the appropriate system
@@ -2395,14 +2428,14 @@ rather than assigning it an empty value.
 
 @emph{Warning:} On Unix systems, @value{GDBN} runs your program using
 the shell indicated by your @code{SHELL} environment variable if it
-exists (or @code{/bin/sh} if not).  If your @code{SHELL} variable
-names a shell that runs an initialization file when started
-non-interactively---such as @file{.cshrc} for C-shell, $@file{.zshenv}
-for the Z shell, or the file specified in the @samp{BASH_ENV}
-environment variable for BASH---any variables you set in that file
-affect your program.  You may wish to move setting of environment
-variables to files that are only run when you sign on, such as
-@file{.login} or @file{.profile}.
+exists and points to a valid shell (@pxref{Valid Shell,,}), or
+@code{/bin/sh} if not.  If your @code{SHELL} variable names a shell
+that runs an initialization file when started non-interactively---such
+as @file{.cshrc} for C-shell, $@file{.zshenv} for the Z shell, or the
+file specified in the @samp{BASH_ENV} environment variable for
+BASH---any variables you set in that file affect your program.  You
+may wish to move setting of environment variables to files that are
+only run when you sign on, such as @file{.login} or @file{.profile}.
 
 @node Working Directory
 @section Your Program's Working Directory
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 4ba62b0..0e7e14c 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -108,6 +108,73 @@ 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), or;
+
+   - The shell specified by the user (through the $SHELL environment
+     variable) is invalid.  An invalid shell is defined as being
+     either a non-existing file, a file the user cannot execute, or
+     the /sbin/nologin shell.
+
+   This function will return 1 when the user wants to start the
+   inferior using a shell and the shell is valid; it will return 0
+   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
+	  || !valid_shell (*shell_file))
+	{
+	  if (*shell_file != NULL)
+	    warning (_("Invalid shell '%s'; using '%s' instead."),
+		     *shell_file, default_shell_file);
+	  /* Being overzealous here.  */
+	  if (valid_shell (default_shell_file))
+	    *shell_file = default_shell_file;
+	  else
+	    {
+	      warning (_("Cannot use '%s' as the shell, using 'exec' "
+			 "to start inferior."),
+		       default_shell_file);
+	      *shell_file = NULL;
+	    }
+	}
+
+      if (*shell_file != NULL)
+	return 1;
+    }
+
+  /* We set startup_with_shell to zero here because if the specified
+     shell is invalid then no shell will be used.  */
+  startup_with_shell = 0;
+  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 +215,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)
     {
diff --git a/gdb/testsuite/gdb.base/invalid-shell.exp b/gdb/testsuite/gdb.base/invalid-shell.exp
new file mode 100644
index 0000000..6593ba6
--- /dev/null
+++ b/gdb/testsuite/gdb.base/invalid-shell.exp
@@ -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: Invalid shell \\\'/invalid/path/to/file\\\'; using \\\'/bin/sh\\\' instead.\r\n\\\[Inferior $decimal \\\(process $decimal\\\) exited normally\\\]" "starting with /bin/sh instead of invalid shell"
+
+# Invoking a shell command must also work.
+gdb_test "shell echo hi" "hi" "invoking shell command from prompt"
+
+set env(SHELL) $oldshell
diff --git a/gdb/utils.c b/gdb/utils.c
index acb4c7d..11c1134 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3364,6 +3364,18 @@ 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)
+{
+  return (shell != NULL
+	  && strcmp (shell, "/sbin/nologin") != 0
+	  && strcmp (shell, "/usr/sbin/nologin") != 0
+	  && strcmp (shell, "/bin/false") != 0
+	  && access (shell, X_OK) == 0);
+}
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_utils;
 
diff --git a/gdb/utils.h b/gdb/utils.h
index 995a1cf..2657708 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -369,4 +369,17 @@ 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.
+
+   A valid shell is defined a file that is:
+
+   - Not {,/usr}/sbin/nologin;
+
+   - Not /bin/false;
+
+   - Executable by the user.  */
+
+extern int valid_shell (const char *shell);
+
 #endif /* UTILS_H */
-- 
2.4.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]