[v3] Make sure GDB uses a valid shell when starting the inferior and to perform the "shell" command
Commit Message
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
Comments
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.
> 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.
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],...
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,
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.]
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
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,
@@ -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);
@@ -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
@@ -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)
{
new file mode 100644
@@ -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
@@ -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;
@@ -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 */