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

[Pedro Alves <palves@redhat.com>]

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