From patchwork Fri Jul 24 20:37:59 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 7843 Received: (qmail 54579 invoked by alias); 24 Jul 2015 20:38:22 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 54569 invoked by uid 89); 24 Jul 2015 20:38:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 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; Fri, 24 Jul 2015 20:38:11 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 5AAC5319B78 for ; Fri, 24 Jul 2015 20:38:10 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-51.yyz.redhat.com [10.15.17.51]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t6OKc9Mn024759; Fri, 24 Jul 2015 16:38:09 -0400 From: Sergio Durigan Junior To: GDB Patches Cc: Sergio Durigan Junior Subject: [PATCH v2] Make sure GDB uses a valid shell when starting the inferior and to perform the "shell" command Date: Fri, 24 Jul 2015 16:37:59 -0400 Message-Id: <1437770279-13072-1-git-send-email-sergiodj@redhat.com> In-Reply-To: <1437761993-18758-1-git-send-email-sergiodj@redhat.com> References: <1437761993-18758-1-git-send-email-sergiodj@redhat.com> X-IsSubscribed: yes 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 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. --- gdb/cli/cli-cmds.c | 9 +++- gdb/doc/gdb.texinfo | 54 ++++++++++++++------- gdb/fork-child.c | 82 +++++++++++++++++++++++++++----- gdb/testsuite/gdb.base/invalid-shell.exp | 48 +++++++++++++++++++ gdb/utils.c | 12 +++++ gdb/utils.h | 13 +++++ 6 files changed, 187 insertions(+), 31 deletions(-) create mode 100644 gdb/testsuite/gdb.base/invalid-shell.exp diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c index 2ec2dd3..ed6a1df 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -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); diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 9e2ecd1..ea1e71b 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -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,25 @@ 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 the @file{/sbin/nologin} (or @file{/usr/sbin/nologin}) program. + +@item +Is not the @file{/bin/false} program. +@end enumerate + +If any of those conditions are not met, the specified shell is not +used by @value{GDBN}. + @node Logging Output @section Logging Output @cindex logging @value{GDBN} output @@ -2041,6 +2061,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 +2307,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 +2417,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 diff --git a/gdb/fork-child.c b/gdb/fork-child.c index 4ba62b0..0e7e14c 100644 --- a/gdb/fork-child.c +++ b/gdb/fork-child.c @@ -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) { diff --git a/gdb/testsuite/gdb.base/invalid-shell.exp b/gdb/testsuite/gdb.base/invalid-shell.exp new file mode 100644 index 0000000..6593ba6 --- /dev/null +++ b/gdb/testsuite/gdb.base/invalid-shell.exp @@ -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 . + +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 diff --git a/gdb/utils.c b/gdb/utils.c index acb4c7d..11c1134 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -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; diff --git a/gdb/utils.h b/gdb/utils.h index 995a1cf..2657708 100644 --- a/gdb/utils.h +++ b/gdb/utils.h @@ -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 */