From patchwork Tue Feb 13 04:35:45 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 25921 Received: (qmail 26474 invoked by alias); 13 Feb 2018 04:35:50 -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 26463 invoked by uid 89); 13 Feb 2018 04:35:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=Suggestion X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 13 Feb 2018 04:35:47 +0000 Received: from [10.0.0.11] (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 87C801E520; Mon, 12 Feb 2018 23:35:45 -0500 (EST) Subject: Re: [PATCH v2 2/2] Make gdbserver work with filename-only binaries To: Sergio Durigan Junior , GDB Patches Cc: Simon Marchi References: <20180210014241.19278-3-sergiodj@redhat.com> <20180212195733.23639-1-sergiodj@redhat.com> <20180212195733.23639-3-sergiodj@redhat.com> From: Simon Marchi Message-ID: Date: Mon, 12 Feb 2018 23:35:45 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180212195733.23639-3-sergiodj@redhat.com> On 2018-02-12 02:57 PM, Sergio Durigan Junior wrote: > Changes from v1: > > - Moved "is_regular_file" from "source.c" to "common/common-utils.c". > > - Made "adjust_program_name_path" use "is_regular_file" in order to > check if there is a file named PROGRAM_NAME in CWD, and prefix it > with CURRENT_DIRECTORY if it exists. Otherwise, don't prefix it and > let gdbserver try to find the binary in $PATH. > > > Simon mentioned on IRC that, after the startup-with-shell feature has > been implemented on gdbserver, it is not possible to specify a > filename-only binary, like: > > $ gdbserver :1234 a.out > /bin/bash: line 0: exec: a.out: not found > During startup program exited with code 127. > Exiting > > This happens on systems where the current directory "." is not listed > in the PATH environment variable. Although include "." in the PATH > variable is a possible workaround, this can be considered a regression > because before startup-with-shell it was possible to use only the > filename (due to reason that gdbserver used "exec*" directly). > > The idea of the patch is to perform a call to "gdb_abspath" and adjust > the PROGRAM_NAME variable before the call to "create_inferior". This > adjustment will consist of tilde-expansion or prefixing PROGRAM_NAME > using the CURRENT_DIRECTORY (a variable that was specific to GDB, but > has been put into common/common-defs.h and now is set/used by > gdbserver as well), thus transforming PROGRAM_NAME in an absolute > path. > > This mimicks the behaviour seen on GDB (look at "openp" and > "attach_inferior", for example). Now, we'll always execute the binary > using its full path on gdbserver. > > I am also submitting a testcase which exercises the scenario described > above. Because the test requires copying (and deleting) files > locally, I decided to restrict its execution to non-remote > targets/hosts. I've also had to do a minor adjustment on > gdb.server/non-existing-program.exp's regexp in order to match the > correct error message. This last part is not valid anymore I believe. > @@ -408,3 +409,34 @@ stringify_argv (const std::vector &args) > > return ret; > } > + > +/* See common/common-utils.h. */ > + > +bool > +is_regular_file (const char *name, int *errno_ptr) > +{ > + struct stat st; > + const int status = stat (name, &st); > + > + /* Stat should never fail except when the file does not exist. > + If stat fails, analyze the source of error and return True Nit: True -> true > + unless the file does not exist, to avoid returning false results > + on obscure systems where stat does not work as expected. */ > + > + if (status != 0) > + { > + if (errno != ENOENT) > + return true; > + *errno_ptr = ENOENT; > + return false; > + } > + > + if (S_ISREG (st.st_mode)) > + return true; > + > + if (S_ISDIR (st.st_mode)) > + *errno_ptr = EISDIR; > + else > + *errno_ptr = EINVAL; > + return false; > +} > diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h > index 2320318de7..888396637e 100644 > --- a/gdb/common/common-utils.h > +++ b/gdb/common/common-utils.h > @@ -146,4 +146,9 @@ in_inclusive_range (T value, T low, T high) > return value >= low && value <= high; > } > > +/* Return True if the file NAME exists and is a regular file. Nit: True -> true > + If the result is false then *ERRNO_PTR is set to a useful value assuming > + we're expecting a regular file. */ > +extern bool is_regular_file (const char *name, int *errno_ptr); > + > #endif > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c > index f931273fa3..24b3e4d4ad 100644 > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -39,6 +39,8 @@ > #include "common-inferior.h" > #include "job-control.h" > #include "environ.h" > +#include "filenames.h" > +#include "pathstuff.h" > > #include "common/selftest.h" > > @@ -283,6 +285,31 @@ get_environ () > return &our_environ; > } > > +/* Verify if PROGRAM_NAME is an absolute path, and perform path > + adjustment/expansion if not. */ > + > +static void > +adjust_program_name_path () > +{ > + /* Make sure we're using the absolute path of the inferior when > + creating it. */ > + if (!IS_ABSOLUTE_PATH (program_name)) I think we should modify the passed path only if really necessary. If the path is relative but contains a directory component, we don't need to change it. I think it's slightly better, because the argv[0] of the spawned process as well as the gdbserver output will be true to what the user typed. I also don't really like the resulting path in: $ ./gdbserver/gdbserver --once :1234 ../gdb/test Process /home/simark/build/binutils-gdb/gdb/../gdb/test created; pid = 12321 So the check would be if (!contains_dir_separator (program_name)) where contains_dir_separator would be a new function. > + { > + int reg_file_errno; > + > + /* Check if the file is in our CWD. If it is, then we prefix > + its name with CURRENT_DIRECTORY. Otherwise, we leave the > + name as-is because we'll try searching for it in $PATH. */ > + if (is_regular_file (program_name, ®_file_errno)) > + { > + char *tmp_program_name = program_name; > + > + program_name = gdb_abspath (program_name).release (); > + xfree (tmp_program_name); > + } > + } > +} > + > static int > attach_inferior (int pid) > { > @@ -3016,6 +3043,8 @@ handle_v_run (char *own_buf) > program_name = new_program_name; > } > > + adjust_program_name_path (); > + > /* Free the old argv and install the new one. */ > free_vector_argv (program_args); > program_args = new_argv; > @@ -3770,6 +3799,8 @@ captured_main (int argc, char *argv[]) > program_args.push_back (xstrdup (next_arg[i])); > program_args.push_back (NULL); > > + adjust_program_name_path (); > + > /* Wait till we are at first instruction in program. */ > create_inferior (program_name, program_args); > > @@ -4290,6 +4321,8 @@ process_serial_event (void) > /* Wait till we are at 1st instruction in prog. */ > if (program_name != NULL) > { > + adjust_program_name_path (); > + I thought I pointed it out in v1, but apparently I didn't. I don't think we need this call to adjust_program_name_path here. We only need it at the places that set program_name, which is not the case of this code. Also, to avoid being able to set program_name and forget to call adjust_program_name_path, I think it would be nice to have a small class that holds the program path in a private field. The only way to change it would be through the setter, which would ensure the adjustment is applied. See the patch below for an example containing both of my suggestions. Finally, I am wondering if maybe the error from getcwd should be fatal. If you change: - current_directory = getcwd (NULL, 0); + current_directory = NULL; to see what would happen if getcwd failed, and launch gdbserver with a filename-only path, you get a segfault: $ ./gdbserver/gdbserver --once :1234 test gdbserver: Success: error finding working directory [1] 21945 segmentation fault (core dumped) ./gdbserver/gdbserver --once :1234 test That's because a NULL current_directory is passed to strlen in gdb_abspath. I think it's rare enough and critical enough situation that we can just exit with an error if getcwd fails (I didn't include this in the patch below because I thought about it after pasting the patch :)). Thanks, Simon From 00a3fc9e340cf6fae1fcb926d39d43594df16643 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 12 Feb 2018 23:02:11 -0500 Subject: [PATCH] Suggestion --- gdb/common/pathstuff.c | 12 ++++++++ gdb/common/pathstuff.h | 4 +++ gdb/gdbserver/server.c | 79 ++++++++++++++++++++++---------------------------- 3 files changed, 51 insertions(+), 44 deletions(-) diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c index fc51edffa9..59e2f7a004 100644 --- a/gdb/common/pathstuff.c +++ b/gdb/common/pathstuff.c @@ -138,3 +138,15 @@ gdb_abspath (const char *path) ? "" : SLASH_STRING, path, (char *) NULL)); } + +bool +contains_dir_separator (const char *path) +{ + for (; *path != '\0'; path++) + { + if (IS_DIR_SEPARATOR (*path)) + return true; + } + + return false; +} diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h index 909fd786bb..e0a7fd5f50 100644 --- a/gdb/common/pathstuff.h +++ b/gdb/common/pathstuff.h @@ -36,4 +36,8 @@ extern gdb::unique_xmalloc_ptr extern gdb::unique_xmalloc_ptr gdb_abspath (const char *path); +/* Return whether PATH contains a directory separator character. */ + +extern bool contains_dir_separator (const char *path); + #endif /* PATHSTUFF_H */ diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c index 24b3e4d4ad..cbfd2d8c99 100644 --- a/gdb/gdbserver/server.c +++ b/gdb/gdbserver/server.c @@ -114,7 +114,32 @@ static int vCont_supported; space randomization feature before starting an inferior. */ int disable_randomization = 1; -static char *program_name = NULL; +static struct { + void set (gdb::unique_xmalloc_ptr &&path) + { + m_path = std::move (path); + + /* Make sure we're using the absolute path of the inferior when + creating it. */ + if (!contains_dir_separator (m_path.get ())) + { + int reg_file_errno; + + /* Check if the file is in our CWD. If it is, then we prefix + its name with CURRENT_DIRECTORY. Otherwise, we leave the + name as-is because we'll try searching for it in $PATH. */ + if (is_regular_file (m_path.get (), ®_file_errno)) + m_path = gdb_abspath (m_path.get ()); + } + } + + char *get () + { return m_path.get (); } + +private: + gdb::unique_xmalloc_ptr m_path; +} program_path; + static std::vector program_args; static std::string wrapper_argv; @@ -271,10 +296,10 @@ get_exec_wrapper () char * get_exec_file (int err) { - if (err && program_name == NULL) + if (err && program_path.get () == NULL) error (_("No executable file specified.")); - return program_name; + return program_path.get (); } /* See server.h. */ @@ -285,31 +310,6 @@ get_environ () return &our_environ; } -/* Verify if PROGRAM_NAME is an absolute path, and perform path - adjustment/expansion if not. */ - -static void -adjust_program_name_path () -{ - /* Make sure we're using the absolute path of the inferior when - creating it. */ - if (!IS_ABSOLUTE_PATH (program_name)) - { - int reg_file_errno; - - /* Check if the file is in our CWD. If it is, then we prefix - its name with CURRENT_DIRECTORY. Otherwise, we leave the - name as-is because we'll try searching for it in $PATH. */ - if (is_regular_file (program_name, ®_file_errno)) - { - char *tmp_program_name = program_name; - - program_name = gdb_abspath (program_name).release (); - xfree (tmp_program_name); - } - } -} - static int attach_inferior (int pid) { @@ -3030,7 +3030,7 @@ handle_v_run (char *own_buf) { /* GDB didn't specify a program to run. Use the program from the last run with the new argument list. */ - if (program_name == NULL) + if (program_path.get () == NULL) { write_enn (own_buf); free_vector_argv (new_argv); @@ -3038,18 +3038,13 @@ handle_v_run (char *own_buf) } } else - { - xfree (program_name); - program_name = new_program_name; - } - - adjust_program_name_path (); + program_path.set (gdb::unique_xmalloc_ptr (new_program_name)); /* Free the old argv and install the new one. */ free_vector_argv (program_args); program_args = new_argv; - create_inferior (program_name, program_args); + create_inferior (program_path.get (), program_args); if (last_status.kind == TARGET_WAITKIND_STOPPED) { @@ -3794,15 +3789,13 @@ captured_main (int argc, char *argv[]) int i, n; n = argc - (next_arg - argv); - program_name = xstrdup (next_arg[0]); + program_path.set (gdb::unique_xmalloc_ptr (xstrdup (next_arg[0]))); for (i = 1; i < n; i++) program_args.push_back (xstrdup (next_arg[i])); program_args.push_back (NULL); - adjust_program_name_path (); - /* Wait till we are at first instruction in program. */ - create_inferior (program_name, program_args); + create_inferior (program_path.get (), program_args); /* We are now (hopefully) stopped at the first instruction of the target process. This assumes that the target process was @@ -4319,11 +4312,9 @@ process_serial_event (void) fprintf (stderr, "GDBserver restarting\n"); /* Wait till we are at 1st instruction in prog. */ - if (program_name != NULL) + if (program_path.get () != NULL) { - adjust_program_name_path (); - - create_inferior (program_name, program_args); + create_inferior (program_path.get (), program_args); if (last_status.kind == TARGET_WAITKIND_STOPPED) {