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

[Doug Evans <xdje42@gmail.com>]

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.