This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3] Make sure GDB uses a valid shell when starting the inferior and to perform the "shell" command
- From: Doug Evans <xdje42 at gmail dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, Eli Zaretskii <eliz at gnu dot org>
- Date: Sun, 26 Jul 2015 10:03:17 -0700
- Subject: Re: [PATCH v3] Make sure GDB uses a valid shell when starting the inferior and to perform the "shell" command
- Authentication-results: sourceware.org; auth=none
- References: <1437761993-18758-1-git-send-email-sergiodj at redhat dot com> <1437869674-7880-1-git-send-email-sergiodj at redhat dot com> <CAP9bCMTpZUUdRJW8-V2PTFFgfuN2FrSdX=+koPcZ_aner5CL6A at mail dot gmail dot com>
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],...