From patchwork Tue Jul 28 19:58:15 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: 7902 Received: (qmail 41754 invoked by alias); 28 Jul 2015 19:58:29 -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 41741 invoked by uid 89); 28 Jul 2015 19:58:28 -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, 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 19:58:24 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id BB062A9A; Tue, 28 Jul 2015 19:58:23 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-51.yyz.redhat.com [10.15.17.51]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t6SJwIao008502; Tue, 28 Jul 2015 15:58:21 -0400 From: Sergio Durigan Junior To: GDB Patches Cc: Eli Zaretskii , Doug Evans , Sergio Durigan Junior Subject: [PATCH] Warn the user when $SHELL is invalid Date: Tue, 28 Jul 2015 15:58:15 -0400 Message-Id: <1438113495-10985-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 After a lot of discussion (really, I did not know this patch would draw so much attention), this patch has now been simplified (mostly because of Doug's suggestions). 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: In order to improve the error/warning messages emitted in this scenario, this patch proposes a new check to be performed before executing the inferior (or before executing a program using the "shell" command on GDB). This new check tries to determine if the specified $SHELL is valid or not. The default behavior has not been changed: if $SHELL is invalid, GDB will still try to execute a program using it, which will lead to an error eventually. However, now GDB will also display a message to the user saying that $SHELL is invalid. This should help when determining what went wrong. The check for a valid shell is simple and has been proposed by Doug. We just fork and then exec a simple command: $SHELL -c 'exit 42' If the return value is 42, then $SHELL is valid. Otherwise we issue the warning. It is obviously trivial to create a program that returns 42 and ignores its arguments, and it it obvious that this program would be considered a valid shell by GDB, but this should not cause any problem; the only "drawback" for the user is that she wouldn't get the warning message before the error. OK to apply? Changes from v3: - Implemented new way to check if $SHELL is valid: we fork, and execute a simple command ('exit 42') to see if the shell supports executing things. - Instead of defaulting to /bin/sh when $SHELL is invalid, just emit a warning to the user. - Removed documentation parts. They are not needed anymore now that the patch does not change the default behavior. 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-28 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: Include "fileutils.h". (valid_shell): New function. * utils.h (valid_shell): New prototype. gdb/testsuite/ChangeLog: 2015-07-28 Sergio Durigan Junior * gdb.base/invalid-shell.exp: New file. --- gdb/cli/cli-cmds.c | 6 +++ gdb/fork-child.c | 67 +++++++++++++++++++++++++------- gdb/testsuite/gdb.base/invalid-shell.exp | 48 +++++++++++++++++++++++ gdb/utils.c | 53 +++++++++++++++++++++++++ gdb/utils.h | 16 ++++++++ 5 files changed, 177 insertions(+), 13 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..1d57c94 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -754,6 +754,12 @@ shell_escape (char *arg, int from_tty) if ((user_shell = (char *) getenv ("SHELL")) == NULL) user_shell = "/bin/sh"; + else + { + if (!valid_shell (user_shell)) + warning (_("The specified shell '%s' is not valid. The command " + "will not be executed."), user_shell); + } /* Get the name of the shell for arg0. */ p = lbasename (user_shell); diff --git a/gdb/fork-child.c b/gdb/fork-child.c index 4ba62b0..36618a4 100644 --- a/gdb/fork-child.c +++ b/gdb/fork-child.c @@ -108,6 +108,58 @@ 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). + + If the specified shell is invalid (see utils.h:valid_shell for + details), this function will print a warning to the user, but *will + still return 1*. + + Return 1 if the STARTUP_WITH_SHELL is set (even if the specified + shell is invalid); zero 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) + { + /* No shell was specified, so we just stick to the default + behavior which is to use DEFAULT_SHELL_FILE. */ + *shell_file = default_shell_file; + } + else if (!valid_shell (*shell_file)) + warning (_("The specified shell '%s' is not valid. You will see\n" + "an error when trying to execute the inferior."), + *shell_file); + + /* We return 1 here even though the shell might be invalid. */ + return 1; + } + + 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 +200,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..0542a53 --- /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: The specified shell \\\'/invalid/path/to/file\\\' is not valid. You will see\r\nan error when trying to execute the inferior.\r\nCannot exec /invalid/path/to/file -c exec .*$testfile .\r\nError: No such file or directory\r\nDuring startup program exited with code $decimal." "starting with /bin/sh instead of invalid shell" + +# Invoking a shell command must also work. +gdb_test "shell echo hi" "warning: The specified shell \\\'/invalid/path/to/file\\\' is not valid. The command will not be executed.\r\nCannot execute /invalid/path/to/file: No such file or directory" "invoking shell command from prompt" + +set env(SHELL) $oldshell diff --git a/gdb/utils.c b/gdb/utils.c index acb4c7d..bf970fd 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -25,6 +25,7 @@ #include "gdbthread.h" #include "fnmatch.h" #include "gdb_bfd.h" +#include "filestuff.h" #ifdef HAVE_SYS_RESOURCE_H #include #endif /* HAVE_SYS_RESOURCE_H */ @@ -3364,6 +3365,58 @@ 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) +{ + pid_t shell_pid; + int status; + size_t len = strlen (shell); + char *shell_argv[4]; + + if (shell == NULL || access (shell, X_OK) != 0) + return 0; + + shell_argv[0] = alloca (len + 1); + strncpy (shell_argv[0], shell, len); + shell_argv[0][len] = '\0'; + shell_argv[1] = "-c"; + shell_argv[2] = "exit 42"; + shell_argv[3] = (char *) NULL; + shell_pid = fork (); + + switch (shell_pid) + { + case -1: + /* vfork failed. Let's just return 0 because we could not + figure out if the shell is valid or not. */ + return 0; + + case 0: + /* Closing everything as we will not need (and don't want) to + output any message. */ + close_most_fds (); + close (STDOUT_FILENO); + close (STDERR_FILENO); + close (STDIN_FILENO); + execvp (shell_argv[0], shell_argv); + /* If we get here, it's an error. Just return an invalid + thing. */ + _exit (0177); + + default: + waitpid (shell_pid, &status, 0); + if (WIFEXITED (status)) + return WEXITSTATUS (status) == 42; + else + { + kill (SIGKILL, shell_pid); + return 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..cc19711 100644 --- a/gdb/utils.h +++ b/gdb/utils.h @@ -369,4 +369,20 @@ 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. + + In oder to perform the check for a valid shell, this function will + fork GDB, and then execute a simple command: $SHELL -c 'exit 42'. + This command shall make the SHELL return 42; if this number is not + returned, it (probably) means that SHELL is not valid. We say + 'probably' because it is trivial to create a program that just + returns 42 and ignores its arguments; in this case, this function + would consider it as a valid shell even though it is clearly not. + But even in this case GDB would later fail to execute anything, so + that only means that the user would not get a nice warning message + in this case. */ + +extern int valid_shell (const char *shell); + #endif /* UTILS_H */