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

  It is known that GDB needs a valid shell to start the inferior and to
offer the "shell" command to the user.  This has recently been the
cause of a problem on the MIPS buildslave, because $SHELL was set to
/sbin/nologin and several tests were failing.  The thread is here:

  <https://sourceware.org/ml/gdb-patches/2015-07/msg00535.html>

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

  - A file that exists and is executable by the user

  - A file that is not /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
  

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
  

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.
  

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.
  

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,
  

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

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

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.
  

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

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
  

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

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.
  

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.
  

> 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
  

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.
  

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.
  

> 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
  

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

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

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

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

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

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.
  

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

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,
  

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

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,
  

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

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.