From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101124 invoked by alias); 28 Jul 2015 23:11:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 101106 invoked by uid 89); 28 Jul 2015 23:11:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 28 Jul 2015 23:11:10 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 6B53A92381; Tue, 28 Jul 2015 23:11:09 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t6SNB7L9025428; Tue, 28 Jul 2015 19:11:08 -0400 Message-ID: <55B80C0A.5020205@redhat.com> Date: Tue, 28 Jul 2015 23:11:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Doug Evans , Sergio Durigan Junior CC: GDB Patches , Eli Zaretskii Subject: Re: [PATCH v3] Make sure GDB uses a valid shell when starting the inferior and to perform the "shell" command References: <1437761993-18758-1-git-send-email-sergiodj@redhat.com> <1437869674-7880-1-git-send-email-sergiodj@redhat.com> <874mkq4t58.fsf@redhat.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-07/txt/msg00846.txt.bz2 On 07/26/2015 09:48 PM, Doug Evans wrote: > On Sun, Jul 26, 2015 at 12:26 PM, Sergio Durigan Junior > 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: > > 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