From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27845 invoked by alias); 26 Jul 2015 08:05:48 -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 27831 invoked by uid 89); 26 Jul 2015 08:05:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-yk0-f180.google.com Received: from mail-yk0-f180.google.com (HELO mail-yk0-f180.google.com) (209.85.160.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sun, 26 Jul 2015 08:05:45 +0000 Received: by ykdu72 with SMTP id u72so48696494ykd.2 for ; Sun, 26 Jul 2015 01:05:43 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.13.221.17 with SMTP id g17mr24068461ywe.11.1437897943367; Sun, 26 Jul 2015 01:05:43 -0700 (PDT) Received: by 10.13.233.198 with HTTP; Sun, 26 Jul 2015 01:05:43 -0700 (PDT) In-Reply-To: <1437869674-7880-1-git-send-email-sergiodj@redhat.com> References: <1437761993-18758-1-git-send-email-sergiodj@redhat.com> <1437869674-7880-1-git-send-email-sergiodj@redhat.com> Date: Sun, 26 Jul 2015 08:05:00 -0000 Message-ID: Subject: Re: [PATCH v3] Make sure GDB uses a valid shell when starting the inferior and to perform the "shell" command From: Doug Evans To: Sergio Durigan Junior Cc: GDB Patches , Eli Zaretskii Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg00767.txt.bz2 On Sat, Jul 25, 2015 at 5:14 PM, Sergio Durigan Junior 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: > > > > 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 > > * 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 > > * 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 > > * 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.