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

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

[Sergio Durigan Junior]

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 /sbin/nologin

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?

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                      | 51 +++++++++++++-------
 gdb/fork-child.c                         | 82 +++++++++++++++++++++++++++-----
 gdb/testsuite/gdb.base/invalid-shell.exp | 38 +++++++++++++++
 gdb/utils.c                              | 10 ++++
 gdb/utils.h                              | 11 +++++
 6 files changed, 170 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..0c23101 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 (see @ref{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,22 @@ 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 the @file{/sbin/nologin} program.
+@end enumerate
+
+If any of those conditions are not met, the specified shell is not
+used by @value{GDBN}.
+
 @node Logging Output
 @section Logging Output
 @cindex logging @value{GDBN} output
@@ -2041,6 +2058,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 (see @ref{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 +2304,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---see @ref{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 +2414,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 (see @ref{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..252ef13
--- /dev/null
+++ b/gdb/testsuite/gdb.base/invalid-shell.exp
@@ -0,0 +1,38 @@
+# 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 { [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..78d0b8f 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3364,6 +3364,16 @@ 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
+	  && 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..ac67ea8 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -369,4 +369,15 @@ 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 /sbin/nologin;
+
+   - Executable by the user.  */
+
+extern int valid_shell (const char *shell);
+
 #endif /* UTILS_H */
-- 
2.4.3

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

[Simon Marchi]

On 15-07-24 02:19 PM, Sergio Durigan Junior wrote:
> 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 /sbin/nologin

Note that on my Ubuntu 14.04:

$ which nologin
/usr/sbin/nologin

I think that /bin/false is also commonly specified as the default shell
for system users (at least according to my /etc/passwd).

Thanks,

Simon

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

[Luis Machado]

On 07/24/2015 03:19 PM, Sergio Durigan Junior wrote:
> diff --git a/gdb/testsuite/gdb.base/invalid-shell.exp b/gdb/testsuite/gdb.base/invalid-shell.exp
> new file mode 100644
> index 0000000..252ef13
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/invalid-shell.exp
> @@ -0,0 +1,38 @@
> +# 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 { [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

I think this test, as is, will not be meaningful for non-linux targets 
or, in general, for remote targets.

Also, the assumption of a path like "/invalid/path/to/file" may not work 
for mingw32.

"run" will also not work for remote targets unless they are running in 
extended-remote mode.

We should either make tests more general or restrict them appropriately 
so they don't cause spurious failures for targets for which these should 
not be executed.

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

[Sergio Durigan Junior]

On Friday, July 24 2015, Luis Machado wrote:

> On 07/24/2015 03:19 PM, Sergio Durigan Junior wrote:
>> diff --git a/gdb/testsuite/gdb.base/invalid-shell.exp b/gdb/testsuite/gdb.base/invalid-shell.exp
>> new file mode 100644
>> index 0000000..252ef13
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/invalid-shell.exp
>> @@ -0,0 +1,38 @@
>> +# 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 { [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
>
> I think this test, as is, will not be meaningful for non-linux targets
> or, in general, for remote targets.
>
> Also, the assumption of a path like "/invalid/path/to/file" may not
> work for mingw32.
>
> "run" will also not work for remote targets unless they are running in
> extended-remote mode.
>
> We should either make tests more general or restrict them
> appropriately so they don't cause spurious failures for targets for
> which these should not be executed.

You're right, I was going to disable this test for remote targets and I
forgot, thanks for letting me know.

I'll disable the test for unsupported targets.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Sergio Durigan Junior]

On Friday, July 24 2015, Simon Marchi wrote:

> On 15-07-24 02:19 PM, Sergio Durigan Junior wrote:
>> 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 /sbin/nologin
>
> Note that on my Ubuntu 14.04:
>
> $ which nologin
> /usr/sbin/nologin

/sbin/nologin is probably a symlink to this file, isn't it?  But yeah,
the check could include /usr/sbin/nologin as well.

> I think that /bin/false is also commonly specified as the default shell
> for system users (at least according to my /etc/passwd).

Indeed.  I will include /bin/false as well.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Fri, 24 Jul 2015 14:19:53 -0400
> 
> 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 /sbin/nologin
> 
> 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?

The documentation part is OK, but please use @pxref instead of "see
@ref" for cross-references in parentheses.

Thanks.

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

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: GDB Patches <gdb-patches@sourceware.org>
> Date: Fri, 24 Jul 2015 15:10:27 -0400
> 
> > Note that on my Ubuntu 14.04:
> >
> > $ which nologin
> > /usr/sbin/nologin
> 
> /sbin/nologin is probably a symlink to this file, isn't it?  But yeah,
> the check could include /usr/sbin/nologin as well.
> 
> > I think that /bin/false is also commonly specified as the default shell
> > for system users (at least according to my /etc/passwd).
> 
> Indeed.  I will include /bin/false as well.

Since the number of valid shells is much smaller than the number of
non-shell programs, isn't it better to have a database of known shells
than to have a database of non-shells people could be expected to set
SHELL to?

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

[Sergio Durigan Junior]

On Friday, July 24 2015, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: GDB Patches <gdb-patches@sourceware.org>
>> Date: Fri, 24 Jul 2015 15:10:27 -0400
>> 
>> > Note that on my Ubuntu 14.04:
>> >
>> > $ which nologin
>> > /usr/sbin/nologin
>> 
>> /sbin/nologin is probably a symlink to this file, isn't it?  But yeah,
>> the check could include /usr/sbin/nologin as well.
>> 
>> > I think that /bin/false is also commonly specified as the default shell
>> > for system users (at least according to my /etc/passwd).
>> 
>> Indeed.  I will include /bin/false as well.
>
> Since the number of valid shells is much smaller than the number of
> non-shell programs, isn't it better to have a database of known shells
> than to have a database of non-shells people could be expected to set
> SHELL to?

My intention is not to catch all the invalid shells that can be set, but
rather make sure that the shell is at least an executable, and is not
something that is commonly used as a "non-shell", like /sbin/nologin or
/bin/false.  Other than these two I cannot think of many more options to
cover in the check.

Another good thing about doing this type of check is that every known
and unknown shell will still work.  When we explicitly check for certain
shell's as you suggest, it means that if we forget any of them its users
will be negatively impacted.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: simon.marchi@ericsson.com, gdb-patches@sourceware.org
> Date: Fri, 24 Jul 2015 15:28:58 -0400
> 
> Another good thing about doing this type of check is that every known
> and unknown shell will still work.  When we explicitly check for certain
> shell's as you suggest, it means that if we forget any of them its users
> will be negatively impacted.

I don't think there are so many shells out there that we run a real
risk of forgetting them.  And even if we do, there's plenty of time
till the next release to hear from those who might be negatively
impacted.

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

[Simon Marchi]

On 15-07-24 03:10 PM, Sergio Durigan Junior wrote:
> On Friday, July 24 2015, Simon Marchi wrote:
> 
>> On 15-07-24 02:19 PM, Sergio Durigan Junior wrote:
>>> 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 /sbin/nologin
>>
>> Note that on my Ubuntu 14.04:
>>
>> $ which nologin
>> /usr/sbin/nologin
> 
> /sbin/nologin is probably a symlink to this file, isn't it?  But yeah,
> the check could include /usr/sbin/nologin as well.

No it isn't:

$ ls -l /sbin/nologin
ls: cannot access /sbin/nologin: No such file or directory

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

[Simon Marchi]

On 15-07-24 03:53 PM, Eli Zaretskii wrote:
> I don't think there are so many shells out there that we run a real
> risk of forgetting them.

Even though you can try to list all known shells, they won't necessarily
be in the same location.  For example, the servers I work on at work have
an old version of everything, so I use my own version of bash.  SHELL won't
be /bin/bash but /home/<user>/local/bin/bash.  gdb wouldn't recognize it as
a valid shell.

> And even if we do, there's plenty of time
> till the next release to hear from those who might be negatively
> impacted.

I don't think that's true.  The number of users who go build and use gdb
from the master branch is probably negligible.  If this affects some users,
it will most likely be when it gets released in distros (months or years
after the actual release, depending on the distro).

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

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> Since the number of valid shells is much smaller than the number of
> non-shell programs, isn't it better to have a database of known shells
> than to have a database of non-shells people could be expected to set
> SHELL to?

This database will be of infinite size.  Why should it be invalid to set
SHELL=/foo/bar if that's my self-compiled super-duper shell?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Sergio Durigan Junior]

On Friday, July 24 2015, Simon Marchi wrote:

> On 15-07-24 03:53 PM, Eli Zaretskii wrote:
>> I don't think there are so many shells out there that we run a real
>> risk of forgetting them.
>
> Even though you can try to list all known shells, they won't necessarily
> be in the same location.  For example, the servers I work on at work have
> an old version of everything, so I use my own version of bash.  SHELL won't
> be /bin/bash but /home/<user>/local/bin/bash.  gdb wouldn't recognize it as
> a valid shell.
>
>> And even if we do, there's plenty of time
>> till the next release to hear from those who might be negatively
>> impacted.
>
> I don't think that's true.  The number of users who go build and use gdb
> from the master branch is probably negligible.  If this affects some users,
> it will most likely be when it gets released in distros (months or years
> after the actual release, depending on the distro).

Yeah, I don't agree with this proposal as well.  I will submit a v2
soon, without Eli's proposal, and see what others say.  If others also
prefer Eli's idea, we can discuss the details.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Paul_Koning]

> On Jul 24, 2015, at 3:53 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: simon.marchi@ericsson.com, gdb-patches@sourceware.org
>> Date: Fri, 24 Jul 2015 15:28:58 -0400
>> 
>> Another good thing about doing this type of check is that every known
>> and unknown shell will still work.  When we explicitly check for certain
>> shell's as you suggest, it means that if we forget any of them its users
>> will be negatively impacted.
> 
> I don't think there are so many shells out there that we run a real
> risk of forgetting them.  And even if we do, there's plenty of time
> till the next release to hear from those who might be negatively
> impacted.

But if you omit a shell, is the user of that shell blocked from using gdb?  That’s not a good failure mode.  It seems to me that omitting a non-shell is much more forgiving: all that happens is that you don’t get the friendly error message.

So that says the explicit list should be of non-shells.

	paul

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

[Sergio Durigan Junior]

On Friday, July 24 2015, Paul Koning wrote:

> But if you omit a shell, is the user of that shell blocked from using
> gdb?  That’s not a good failure mode.  It seems to me that omitting a
> non-shell is much more forgiving: all that happens is that you don’t
> get the friendly error message.

As I said before, I agree with this rationale.  However, I just would
like to make it clear that if the shell is invalid, the user will still
be able to start inferiors because GDB will default to /bin/sh.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Simon Marchi]

On 15-07-24 04:25 PM, Paul_Koning@Dell.com wrote:
> But if you omit a shell, is the user of that shell blocked from using gdb?  That’s not a good failure mode.  It seems to me that omitting a non-shell is much more forgiving: all that happens is that you don’t get the friendly error message.
> 
> So that says the explicit list should be of non-shells.
> 
> 	paul

With Eli's suggestion, if SHELL is valid but gdb doesn't know about it (e.g.
SHELL=/my/super/duper/shell), it will fall back to using /bin/sh.  So no,
the user wouldn't be blocked.

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

[Sergio Durigan Junior]

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 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                      | 54 ++++++++++++++-------
 gdb/fork-child.c                         | 82 +++++++++++++++++++++++++++-----
 gdb/testsuite/gdb.base/invalid-shell.exp | 48 +++++++++++++++++++
 gdb/utils.c                              | 12 +++++
 gdb/utils.h                              | 13 +++++
 6 files changed, 187 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..ea1e71b 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,25 @@ 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 the @file{/sbin/nologin} (or @file{/usr/sbin/nologin}) program.
+
+@item
+Is not the @file{/bin/false} program.
+@end enumerate
+
+If any of those conditions are not met, the specified shell is not
+used by @value{GDBN}.
+
 @node Logging Output
 @section Logging Output
 @cindex logging @value{GDBN} output
@@ -2041,6 +2061,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 +2307,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 +2417,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

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

[Paul_Koning]

> On Jul 24, 2015, at 4:38 PM, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> On 15-07-24 04:25 PM, Paul_Koning@Dell.com wrote:
>> But if you omit a shell, is the user of that shell blocked from using gdb?  That’s not a good failure mode.  It seems to me that omitting a non-shell is much more forgiving: all that happens is that you don’t get the friendly error message.
>> 
>> So that says the explicit list should be of non-shells.
>> 
>> 	paul
> 
> With Eli's suggestion, if SHELL is valid but gdb doesn't know about it (e.g.
> SHELL=/my/super/duper/shell), it will fall back to using /bin/sh.  So no,
> the user wouldn't be blocked.
> 
> 
Not unless the features in that unknown shell are needed for the application to function correctly.

	paul

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

[Matt Rice]

On Fri, Jul 24, 2015 at 1:42 PM,  <Paul_Koning@dell.com> wrote:
>
>> On Jul 24, 2015, at 4:38 PM, Simon Marchi <simon.marchi@ericsson.com> wrote:
>>
>> On 15-07-24 04:25 PM, Paul_Koning@Dell.com wrote:
>>> But if you omit a shell, is the user of that shell blocked from using gdb?  That’s not a good failure mode.  It seems to me that omitting a non-shell is much more forgiving: all that happens is that you don’t get the friendly error message.
>>>
>>> So that says the explicit list should be of non-shells.
>>>
>>>      paul
>>
>> With Eli's suggestion, if SHELL is valid but gdb doesn't know about it (e.g.
>> SHELL=/my/super/duper/shell), it will fall back to using /bin/sh.  So no,
>> the user wouldn't be blocked.
>>
>>
> Not unless the features in that unknown shell are needed for the application to function correctly.

another case of this is shells which actively restrict the application to some
 subset of available functionality

http://plash.beasts.org/index.html#section1

i'm not sure about the following being unfamiliar with it, but it
seems a likely candidate for this as well.

https://github.com/trombonehero/capsh

an unrecognised shell in this case could lead to increased authority
of the inferior process, as the general effort here is to restrict
processes to those file descriptors that the shell passes to it and
remove the ability to obtain new file descriptors the shell has not
blessed..

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

[Eli Zaretskii]

> Date: Fri, 24 Jul 2015 16:09:40 -0400
> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 15-07-24 03:53 PM, Eli Zaretskii wrote:
> > I don't think there are so many shells out there that we run a real
> > risk of forgetting them.
> 
> Even though you can try to list all known shells, they won't necessarily
> be in the same location.  For example, the servers I work on at work have
> an old version of everything, so I use my own version of bash.  SHELL won't
> be /bin/bash but /home/<user>/local/bin/bash.  gdb wouldn't recognize it as
> a valid shell.

I didn't say that, but meant to suggest testing only the basename of
the shell.

> > And even if we do, there's plenty of time
> > till the next release to hear from those who might be negatively
> > impacted.
> 
> I don't think that's true.  The number of users who go build and use gdb
> from the master branch is probably negligible.  If this affects some users,
> it will most likely be when it gets released in distros (months or years
> after the actual release, depending on the distro).

Fine, have it your way, and let's see if that flies.  I thought the
fact that we've got 2 additional candidates in less than 1 day is
evidence that more will follow, but I guess we will see.

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

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Sergio Durigan Junior <sergiodj@redhat.com>,  simon.marchi@ericsson.com,  gdb-patches@sourceware.org
> Date: Fri, 24 Jul 2015 22:18:08 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Since the number of valid shells is much smaller than the number of
> > non-shell programs, isn't it better to have a database of known shells
> > than to have a database of non-shells people could be expected to set
> > SHELL to?
> 
> This database will be of infinite size.  Why should it be invalid to set
> SHELL=/foo/bar if that's my self-compiled super-duper shell?

The same goes for non-shells as well.

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

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: <eliz@gnu.org>, <simon.marchi@ericsson.com>, <gdb-patches@sourceware.org>
> Date: Fri, 24 Jul 2015 16:35:19 -0400
> 
> On Friday, July 24 2015, Paul Koning wrote:
> 
> > But if you omit a shell, is the user of that shell blocked from using
> > gdb?  That’s not a good failure mode.  It seems to me that omitting a
> > non-shell is much more forgiving: all that happens is that you don’t
> > get the friendly error message.
> 
> As I said before, I agree with this rationale.  However, I just would
> like to make it clear that if the shell is invalid, the user will still
> be able to start inferiors because GDB will default to /bin/sh.

As would happen if we don't recognize the shell.

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

[Eli Zaretskii]

> Date: Fri, 24 Jul 2015 14:36:38 -0700
> From: Matt Rice <ratmice@gmail.com>
> Cc: simon.marchi@ericsson.com, Eli Zaretskii <eliz@gnu.org>, 	Sergio Durigan Junior <sergiodj@redhat.com>, 	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> On Fri, Jul 24, 2015 at 1:42 PM,  <Paul_Koning@dell.com> wrote:
> >
> >> On Jul 24, 2015, at 4:38 PM, Simon Marchi <simon.marchi@ericsson.com> wrote:
> >>
> >> On 15-07-24 04:25 PM, Paul_Koning@Dell.com wrote:
> >>> But if you omit a shell, is the user of that shell blocked from using gdb?  That’s not a good failure mode.  It seems to me that omitting a non-shell is much more forgiving: all that happens is that you don’t get the friendly error message.
> >>>
> >>> So that says the explicit list should be of non-shells.
> >>>
> >>>      paul
> >>
> >> With Eli's suggestion, if SHELL is valid but gdb doesn't know about it (e.g.
> >> SHELL=/my/super/duper/shell), it will fall back to using /bin/sh.  So no,
> >> the user wouldn't be blocked.
> >>
> >>
> > Not unless the features in that unknown shell are needed for the application to function correctly.
> 
> another case of this is shells which actively restrict the application to some
>  subset of available functionality

They should be included in the list of the shells we know about.

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

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andreas Schwab <schwab@linux-m68k.org>
>> Cc: Sergio Durigan Junior <sergiodj@redhat.com>,  simon.marchi@ericsson.com,  gdb-patches@sourceware.org
>> Date: Fri, 24 Jul 2015 22:18:08 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Since the number of valid shells is much smaller than the number of
>> > non-shell programs, isn't it better to have a database of known shells
>> > than to have a database of non-shells people could be expected to set
>> > SHELL to?
>> 
>> This database will be of infinite size.  Why should it be invalid to set
>> SHELL=/foo/bar if that's my self-compiled super-duper shell?
>
> The same goes for non-shells as well.

No, the set of non-shells is finite, because they will match a line in
/etc/shells.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: sergiodj@redhat.com,  simon.marchi@ericsson.com,  gdb-patches@sourceware.org
> the set of non-shells is finite, because they will match a line in
> /etc/shells.

I'm okay with looking up non-shells in /etc/shells, but that's not
what the proposed patch does.  It just compares the shell's file name
with a list of known "invalid" shells.  It doesn't even mention
/etc/shells, in fact, not even in the documentation parts of the
patch.

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

[Sergio Durigan Junior]

On Saturday, July 25 2015, Eli Zaretskii wrote:

>> > And even if we do, there's plenty of time
>> > till the next release to hear from those who might be negatively
>> > impacted.
>> 
>> I don't think that's true.  The number of users who go build and use gdb
>> from the master branch is probably negligible.  If this affects some users,
>> it will most likely be when it gets released in distros (months or years
>> after the actual release, depending on the distro).
>
> Fine, have it your way, and let's see if that flies.  I thought the
> fact that we've got 2 additional candidates in less than 1 day is
> evidence that more will follow, but I guess we will see.

Maybe I should have been more clear about what I want to achieve with
this patch.

My goal was not to match every possible invalid shell out there, nor to
make sure that the specified shell is a known and valid shell.  My goal
was to make sure that the shell exists, is an executable, and is not
something that is commonly used to disable logins (/sbin/nologin or
/bin/false are the obvious candidates here).

The 2 additional candidates that have been mentioned were actually just
1: I did not remember to include /bin/false in the list before, but
/usr/sbin/nologin is nologin (and I could even just check for the
basename as you proposed in another message, eliminating the need to
include checks for {,/usr}).

I don't think we will see the list of non-shells expanding much more.
One can always say "Hey, but /bin/ls is a not a shell!", and we will say
"Right, and it is not commonly used as shell anyway".

Finally, I don't want to forbid the user to specify her own shell to run
the inferior, and to name her shell as she wants.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Simon Marchi <simon.marchi@ericsson.com>, gdb-patches@sourceware.org
> Date: Sat, 25 Jul 2015 12:29:56 -0400
> 
> My goal was not to match every possible invalid shell out there, nor to
> make sure that the specified shell is a known and valid shell.  My goal
> was to make sure that the shell exists, is an executable, and is not
> something that is commonly used to disable logins (/sbin/nologin or
> /bin/false are the obvious candidates here).
> 
> The 2 additional candidates that have been mentioned were actually just
> 1: I did not remember to include /bin/false in the list before, but
> /usr/sbin/nologin is nologin (and I could even just check for the
> basename as you proposed in another message, eliminating the need to
> include checks for {,/usr}).
> 
> I don't think we will see the list of non-shells expanding much more.
> One can always say "Hey, but /bin/ls is a not a shell!", and we will say
> "Right, and it is not commonly used as shell anyway".

Just reading the section you proposed for the manual seems to imply
the goals are much wider than you say above.  If we only want to avoid
these 2 non-shells, why do we even need to document that obscure
detail?

> Finally, I don't want to forbid the user to specify her own shell to run
> the inferior, and to name her shell as she wants.

Her shell could be named /sbin/nologin, no?

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

[Sergio Durigan Junior]

On Saturday, July 25 2015, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Simon Marchi <simon.marchi@ericsson.com>, gdb-patches@sourceware.org
>> Date: Sat, 25 Jul 2015 12:29:56 -0400
>> 
>> My goal was not to match every possible invalid shell out there, nor to
>> make sure that the specified shell is a known and valid shell.  My goal
>> was to make sure that the shell exists, is an executable, and is not
>> something that is commonly used to disable logins (/sbin/nologin or
>> /bin/false are the obvious candidates here).
>> 
>> The 2 additional candidates that have been mentioned were actually just
>> 1: I did not remember to include /bin/false in the list before, but
>> /usr/sbin/nologin is nologin (and I could even just check for the
>> basename as you proposed in another message, eliminating the need to
>> include checks for {,/usr}).
>> 
>> I don't think we will see the list of non-shells expanding much more.
>> One can always say "Hey, but /bin/ls is a not a shell!", and we will say
>> "Right, and it is not commonly used as shell anyway".
>
> Just reading the section you proposed for the manual seems to imply
> the goals are much wider than you say above.  If we only want to avoid
> these 2 non-shells, why do we even need to document that obscure
> detail?

Because I think it is worth documenting this to the user; the more
information we give about how GDB behaves, the better (IMHO).

The new section says:

  @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 the @file{/sbin/nologin} (or @file{/usr/sbin/nologin}) program.

  @item
  Is not the @file{/bin/false} program.
  @end enumerate

  If any of those conditions are not met, the specified shell is not
  used by @value{GDBN}.

I do not see any difference from what I said above, but if you think
this text can be improved, or that this text is not needed at all, then
by all means feel free to ask this.

>> Finally, I don't want to forbid the user to specify her own shell to run
>> the inferior, and to name her shell as she wants.
>
> Her shell could be named /sbin/nologin, no?

Yes...  I should have said:

  Finally, I don't want to forbid the user to specify her own shell to
  run the inferior, and to name her shell as she wants, as long as it is
  not named {,/usr}/sbin/nologin and /bin/false, and as long as it is an
  existing file, and as long as this file can be executed by her.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: simon.marchi@ericsson.com, gdb-patches@sourceware.org
> Date: Sat, 25 Jul 2015 13:03:12 -0400
> 
> The new section says:
> 
>   @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 the @file{/sbin/nologin} (or @file{/usr/sbin/nologin}) program.
> 
>   @item
>   Is not the @file{/bin/false} program.
>   @end enumerate
> 
>   If any of those conditions are not met, the specified shell is not
>   used by @value{GDBN}.
> 
> I do not see any difference from what I said above, but if you think
> this text can be improved, or that this text is not needed at all, then
> by all means feel free to ask this.

The use of "valid" seems to imply much broader goals.  Your
description seems to say that "pseudo-shells used to disable logins"
is a better (though longer) terminology.

Also, I suggest to say "such as the following", so as not to imply
that this is necessarily an exhaustive list.

Finally, is it really OK to lump here the "cannot be executed by the
user" case?  Maybe we should error out in that case.

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

[Sergio Durigan Junior]

On Saturday, July 25 2015, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: simon.marchi@ericsson.com, gdb-patches@sourceware.org
>> Date: Sat, 25 Jul 2015 13:03:12 -0400
>> 
>> The new section says:
>> 
>>   @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 the @file{/sbin/nologin} (or @file{/usr/sbin/nologin}) program.
>> 
>>   @item
>>   Is not the @file{/bin/false} program.
>>   @end enumerate
>> 
>>   If any of those conditions are not met, the specified shell is not
>>   used by @value{GDBN}.
>> 
>> I do not see any difference from what I said above, but if you think
>> this text can be improved, or that this text is not needed at all, then
>> by all means feel free to ask this.
>
> The use of "valid" seems to imply much broader goals.  Your
> description seems to say that "pseudo-shells used to disable logins"
> is a better (though longer) terminology.
>
> Also, I suggest to say "such as the following", so as not to imply
> that this is necessarily an exhaustive list.

OK, I will make these changes and send a v3.

> Finally, is it really OK to lump here the "cannot be executed by the
> user" case?  Maybe we should error out in that case.

I don't think we should error out in this case, since we can fallback to
/bin/sh and display a warning (which is what the patch does).  Erroring
out seems too much for me.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Sergio Durigan Junior]

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

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

[Doug Evans]

On Sat, Jul 25, 2015 at 5:14 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> 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.

Hi.

I'd like to not have this patch checked in, at least not yet.

I'm going to leave security as a separate thread.
The topic here is just convenience and assistance (IIUC -
please correct me if I'm wrong).

Having an internally hardcoded list of shells (good or bad) suggests
to me there's got to be a better way.

Another thing that bothers me is that if SHELL
is set to something gdb thinks is bad, gdb will
try to be "clever" and override that setting.
If a tool is going to be helpful, I think it
also needs a mode to not be. It's hard to
work around hardwired cleverness when
you don't want it. Hopefully in this instance
we can avoid adding an option though.

As a strawman, what if gdb first tests $SHELL
(e.g., $SHELL -c 'exit 42' or some such)
and if that doesn't work warn the user,
but otherwise leave things as is?
One could defer doing the test until the first
need for $SHELL.
And if $SHELL isn't usable, leave it to the
user to fix the problem.

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

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Sat, 25 Jul 2015 20:14:34 -0400
> 
> 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.

OK for the documentation part.

Thanks.

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

[Doug Evans]

On Sun, Jul 26, 2015 at 1:05 AM, Doug Evans <xdje42@gmail.com> wrote:
> On Sat, Jul 25, 2015 at 5:14 PM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> 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.
>
> Hi.
>
> I'd like to not have this patch checked in, at least not yet.
>
> I'm going to leave security as a separate thread.
> The topic here is just convenience and assistance (IIUC -
> please correct me if I'm wrong).
>
> Having an internally hardcoded list of shells (good or bad) suggests
> to me there's got to be a better way.
>
> Another thing that bothers me is that if SHELL
> is set to something gdb thinks is bad, gdb will
> try to be "clever" and override that setting.
> If a tool is going to be helpful, I think it
> also needs a mode to not be. It's hard to
> work around hardwired cleverness when
> you don't want it. Hopefully in this instance
> we can avoid adding an option though.
>
> As a strawman, what if gdb first tests $SHELL
> (e.g., $SHELL -c 'exit 42' or some such)
> and if that doesn't work warn the user,
> but otherwise leave things as is?
> One could defer doing the test until the first
> need for $SHELL.
> And if $SHELL isn't usable, leave it to the
> user to fix the problem.

Another thing we could/should do is provide a way
to report exactly what gdb is doing.
IOW, print argv[0],[1],...

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

[Sergio Durigan Junior]

On Sunday, July 26 2015, Doug Evans wrote:

> On Sat, Jul 25, 2015 at 5:14 PM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> 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.
>
> Hi.
>
> I'd like to not have this patch checked in, at least not yet.
>
> I'm going to leave security as a separate thread.
> The topic here is just convenience and assistance (IIUC -
> please correct me if I'm wrong).

It is just assistance, indeed..  Security is definitely not the focus
here.

> Having an internally hardcoded list of shells (good or bad) suggests
> to me there's got to be a better way.

I'm definitely open to suggestions.

> Another thing that bothers me is that if SHELL
> is set to something gdb thinks is bad, gdb will
> try to be "clever" and override that setting.
> If a tool is going to be helpful, I think it
> also needs a mode to not be. It's hard to
> work around hardwired cleverness when
> you don't want it. Hopefully in this instance
> we can avoid adding an option though.

Yeah.  This can be easily fixed with (yet another) setting.  'set
use-valid-shell on/off', maybe?

> As a strawman, what if gdb first tests $SHELL
> (e.g., $SHELL -c 'exit 42' or some such)
> and if that doesn't work warn the user,
> but otherwise leave things as is?
> One could defer doing the test until the first
> need for $SHELL.
> And if $SHELL isn't usable, leave it to the
> user to fix the problem.

So you're suggesting that we only warn the user about the invalid shell,
instead of deciding to use /bin/sh without asking her?

As much as I think it *is* useful to have GDB default to /bin/sh if
$SHELL is /sbin/nologin (for example), I am OK with just warning the
user without taking any action.

So, to summarize: what would you think of a patch that:

- tested $SHELL (as you proposed; $SHELL -c 'exit 42').

- if the test fails, warn the user about it.  If 'set use-valid-shell'
  is on, continue using /bin/sh; otherwise, just error out.

?

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Doug Evans]

On Sun, Jul 26, 2015 at 12:26 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Sunday, July 26 2015, Doug Evans wrote:
>> ...
>> Hi.
>>
>> I'd like to not have this patch checked in, at least not yet.
>>
>> I'm going to leave security as a separate thread.
>> The topic here is just convenience and assistance (IIUC -
>> please correct me if I'm wrong).
>
> It is just assistance, indeed..  Security is definitely not the focus
> here.
>
>> Having an internally hardcoded list of shells (good or bad) suggests
>> to me there's got to be a better way.
>
> I'm definitely open to suggestions.
>
>> Another thing that bothers me is that if SHELL
>> is set to something gdb thinks is bad, gdb will
>> try to be "clever" and override that setting.
>> If a tool is going to be helpful, I think it
>> also needs a mode to not be. It's hard to
>> work around hardwired cleverness when
>> you don't want it. Hopefully in this instance
>> we can avoid adding an option though.
>
> Yeah.  This can be easily fixed with (yet another) setting.  'set
> use-valid-shell on/off', maybe?
>
>> As a strawman, what if gdb first tests $SHELL
>> (e.g., $SHELL -c 'exit 42' or some such)
>> and if that doesn't work warn the user,
>> but otherwise leave things as is?
>> One could defer doing the test until the first
>> need for $SHELL.
>> And if $SHELL isn't usable, leave it to the
>> user to fix the problem.
>
> So you're suggesting that we only warn the user about the invalid shell,
> instead of deciding to use /bin/sh without asking her?
>
> As much as I think it *is* useful to have GDB default to /bin/sh if
> $SHELL is /sbin/nologin (for example), I am OK with just warning the
> user without taking any action.
>
> So, to summarize: what would you think of a patch that:
>
> - tested $SHELL (as you proposed; $SHELL -c 'exit 42').
>
> - if the test fails, warn the user about it.  If 'set use-valid-shell'
>   is on, continue using /bin/sh; otherwise, just error out.
>
> ?
>
> Thanks,

Assuming others are ok with it, I'd say let's go with the test,
and leave use-valid-shell for another day.
IIUC we tripped over this because of a misconfigured build-bot,
which we can easily fix. It's not clear to me that a new user option
is warranted. They're using gdb. If they don't know about $SHELL
and /bin/sh we can educate them - and one place we can do that
is in the warning we print if the test fails.
[I'm all for having more descriptive/explanatory warnings/errors
that assist users in fixing the issue.]

[PATCH] Warn the user when $SHELL is invalid

[Sergio Durigan Junior]

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

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 2ec2dd3..1d57c94 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -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);
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 4ba62b0..36618a4 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -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)
     {
diff --git a/gdb/testsuite/gdb.base/invalid-shell.exp b/gdb/testsuite/gdb.base/invalid-shell.exp
new file mode 100644
index 0000000..0542a53
--- /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: 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
diff --git a/gdb/utils.c b/gdb/utils.c
index acb4c7d..bf970fd 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -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;
 
diff --git a/gdb/utils.h b/gdb/utils.h
index 995a1cf..cc19711 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -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 */
-- 
2.4.3

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

[Pedro Alves]

On 07/26/2015 09:48 PM, Doug Evans wrote:
> On Sun, Jul 26, 2015 at 12:26 PM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> On Sunday, July 26 2015, Doug Evans wrote:
>>> ...
>>> Hi.
>>>
>>> I'd like to not have this patch checked in, at least not yet.
>>>
>>> I'm going to leave security as a separate thread.
>>> The topic here is just convenience and assistance (IIUC -
>>> please correct me if I'm wrong).
>>
>> It is just assistance, indeed..  Security is definitely not the focus
>> here.
>>
>>> Having an internally hardcoded list of shells (good or bad) suggests
>>> to me there's got to be a better way.
>>
>> I'm definitely open to suggestions.
>>
>>> Another thing that bothers me is that if SHELL
>>> is set to something gdb thinks is bad, gdb will
>>> try to be "clever" and override that setting.
>>> If a tool is going to be helpful, I think it
>>> also needs a mode to not be. It's hard to
>>> work around hardwired cleverness when
>>> you don't want it. Hopefully in this instance
>>> we can avoid adding an option though.
>>
>> Yeah.  This can be easily fixed with (yet another) setting.  'set
>> use-valid-shell on/off', maybe?
>>
>>> As a strawman, what if gdb first tests $SHELL
>>> (e.g., $SHELL -c 'exit 42' or some such)
>>> and if that doesn't work warn the user,
>>> but otherwise leave things as is?
>>> One could defer doing the test until the first
>>> need for $SHELL.
>>> And if $SHELL isn't usable, leave it to the
>>> user to fix the problem.
>>
>> So you're suggesting that we only warn the user about the invalid shell,
>> instead of deciding to use /bin/sh without asking her?
>>
>> As much as I think it *is* useful to have GDB default to /bin/sh if
>> $SHELL is /sbin/nologin (for example), I am OK with just warning the
>> user without taking any action.
>>
>> So, to summarize: what would you think of a patch that:
>>
>> - tested $SHELL (as you proposed; $SHELL -c 'exit 42').
>>
>> - if the test fails, warn the user about it.  If 'set use-valid-shell'
>>   is on, continue using /bin/sh; otherwise, just error out.
>>
>> ?
>>
>> Thanks,
> 
> Assuming others are ok with it, I'd say let's go with the test,
> and leave use-valid-shell for another day.
> IIUC we tripped over this because of a misconfigured build-bot,
> which we can easily fix. It's not clear to me that a new user option
> is warranted. They're using gdb. If they don't know about $SHELL
> and /bin/sh we can educate them - and one place we can do that
> is in the warning we print if the test fails.
> [I'm all for having more descriptive/explanatory warnings/errors
> that assist users in fixing the issue.]
> 

I have to say that I'm a bit puzzled at the necessity of
performing any validity check upfront.

The proposed commit log says:

> 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>

But, all that confusion stems from the bogus error, which was meanwhile
fixed by:

 https://sourceware.org/ml/gdb-patches/2015-07/msg00705.html

With that in place, the original error log would look like:

220-exec-run
&"Cannot exec /sbin/nologin
-c exec /mips/proj/build-compiler/upstream-testing/mipsswbrd048/GDB-testing/debian-mips-m64/build/gdb/testsuite/outputs/gdb.mi/mi-watch/mi-watch
.\n"

which should have made the problem obvious.  I'd hazard a guess
that even if that was:

 Cannot exec /opt/whatever/bin/someshell -c exec /mips/proj/build-compiler/upstream-testing/mipsswbrd048/GDB-testing/debian-mips-m64/build/gdb/testsuite/outputs/gdb.mi/mi-watch/mi-watch

then the first think you'd do is try running that manually, and figure
out quickly what is wrong.

Should we try to take a step back and identify the use cases that
we're trying to address?  I'm all for improving the error message, but
I question the value of adding the extra fork/check-exit etc.
complexity.

Thanks,
Pedro Alves

Re: [PATCH] Warn the user when $SHELL is invalid

[Pedro Alves]

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

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

[Sergio Durigan Junior]

On Tuesday, July 28 2015, Pedro Alves wrote:

> I have to say that I'm a bit puzzled at the necessity of
> performing any validity check upfront.

The original idea was not only to check if the $SHELL is valid, but
mostly make GDB react to this by using another shell instead (/bin/sh in
this case).

> The proposed commit log says:
>
>> 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>
>
> But, all that confusion stems from the bogus error, which was meanwhile
> fixed by:
>
>  https://sourceware.org/ml/gdb-patches/2015-07/msg00705.html
>
> With that in place, the original error log would look like:
>
> 220-exec-run
> &"Cannot exec /sbin/nologin
> -c exec /mips/proj/build-compiler/upstream-testing/mipsswbrd048/GDB-testing/debian-mips-m64/build/gdb/testsuite/outputs/gdb.mi/mi-watch/mi-watch
> .\n"
>
> which should have made the problem obvious.  I'd hazard a guess
> that even if that was:
>
>  Cannot exec /opt/whatever/bin/someshell -c exec /mips/proj/build-compiler/upstream-testing/mipsswbrd048/GDB-testing/debian-mips-m64/build/gdb/testsuite/outputs/gdb.mi/mi-watch/mi-watch
>
> then the first think you'd do is try running that manually, and figure
> out quickly what is wrong.

Sure, the new error message makes things easier to figure out indeed.
And on a second thought I also agree that using the fork mechanism to
just be able to issue a warning to the user seems too much/too
dangerous.

To be completely honest I still prefer the first version of the patch,
which made GDB react when $SHELL was set to something dubious like
/sbin/nologin and choose a proper shell to start the inferior instead.
However, I understand that this patch has caused a lot of bikeshed
already, and has been "degraded" to "just warn the user" (which, as you
pointed out, is indeed not very necessary anymore), so TBH I don't have
the energy/time to keep hacking on it now.

I appreciate all the feedback received!

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Warn the user when $SHELL is invalid

[Sergio Durigan Junior]

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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/