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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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/