This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 2/2] Make gdbserver work with filename-only binaries
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<char *> &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 <simon.marchi@polymtl.ca>
> 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<char>
>
> extern gdb::unique_xmalloc_ptr<char> 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<char> &&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<char> m_path;
> +} program_path;
> +
> static std::vector<char *> 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<char> (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<char> (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/