From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119557 invoked by alias); 22 Feb 2018 18:37:18 -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 119541 invoked by uid 89); 22 Feb 2018 18:37:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_LOW,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 22 Feb 2018 18:37:15 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0B2DA4022905; Thu, 22 Feb 2018 18:37:14 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id D99C62026E03; Thu, 22 Feb 2018 18:37:13 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Simon Marchi Subject: Re: [PATCH v2 2/2] Make gdbserver work with filename-only binaries References: <20180210014241.19278-3-sergiodj@redhat.com> <20180212195733.23639-1-sergiodj@redhat.com> <20180212195733.23639-3-sergiodj@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) Date: Thu, 22 Feb 2018 18:37:00 -0000 Message-ID: <87d10w6fc6.fsf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00323.txt.bz2 On Monday, February 12 2018, Simon Marchi wrote: > 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. Yes, I'll remove it. >> @@ -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 Fixed. >> + 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 Fixed. >> + 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. OK. >> + { >> + 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. I incorporated your patch, thanks. > 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 :)). Maybe the error should be fatal. I decided to mimick what GDB already does (call perror_warning_with_name, which issues a warning, and continue). I will modify this code to call error instead on gdbserver. > 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) > { > -- > 2.16.1 -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/