Re: [PATCHv6] gdb: handle loading shared libraries from /proc/self/fd/

[Pedro Alves <pedro@palves.net>]

On 2023-01-20 12:33 p.m., Andrew Burgess via Gdb-patches wrote:
> From: Asaf Fisher via Gdb-patches <gdb-patches@sourceware.org>
> 
> Changes since v5:
> 
>   - Rebased onto HEAD of master, updated the test script to use
>     'require allow_shlib_tests'.
> 
> Changes since v4:
> 
>   - filter_proc_self_filenames now makes use of gdb_realpath_keepfile to
>     (partially) canonicalize the path, this should fix the non-canonical
>     path issues,
> 
>   - extended the test to include the non-canonical path case.
> 
> Changes since v3:
> 
>   - Fixed NULL/nullptr issues Lancelot pointed out,
> 
>   - Updated the test case to take account of non-probes based shared
>     library event handling.
> 
> ---
> 
> Bug PR gdb/29586 describes a situations where a shared library is

"a situations" -> "a situation"

> created in memory, then written to a memory mapped file.  The memory
> mapped file will show up as a file descriptor within /proc/self/fd/,
> and this path is then used with dlopen in order to call functions
> within the in-memory shared library.
> 
> When attempting to debug this GDB hangs.  The problem is that, GDB
> stops at the shared-library event, reads the shared library path from
> the inferior, which is "/proc/self/fd/<NUM>", and then GDB attempts to
> open this file.
> 
> Unfortunately, this means GDB tries to open a file within GDB's
> /proc/self/fd/ directory, not within the inferior's directory.  In the
> case of our hang it turns out that the file descriptor that is opened
> is a pipe, and GDB hangs trying to read from the pipe.
> 
> However, the behaviour is really just undefined, depending on which
> file descriptor the inferior tries to open, GDB will open, or fail to
> open, random files within its /proc/self/fd directory.
> 
> The solution proposed in this commit is to hook into solib_find_1, and
> spot when GDB is looking for any file in /proc/self/, if this is the
> case, then the filename is rewritten as /proc/<PID>, where <PID> is
> the process-id of the current inferior.
> 
> One complexity that we need to consider is how to handle non-canonical
> paths, for example, if the user opens /proc/../proc/self/fd/<NUM> then
> GDB also needs to be able to spot this.
> 
> My solution is to call gdb_realpath_keepfile, this canonicalizes
> everything except the filename (the '/proc/../proc/self/fd' part in
> the above example).  The problem this then introduces is that the
> 'self' is replaces with the pid of GDB itself.  I therefore check,

"is replaces" -> "is replaced"


> after canonicalization, for a prefix of /proc/<PID OF GDB>, and if
> this is the case, I replace this with /proc/<PID OF INFERIOR>.
> 
> There is one remaining issue, which I consider very unlikely, and I
> don't propose to fix.  What if the inferior actually did try to dlopen
> a file from /proc/<PID OF GDB>?  If this happened then GDB will not be
> able to distinguish this from the above case and would still replace
> the pid of GDB with the pid of the inferior.  However, this seems
> really unlikely - why would the inferior be trying to load parts of
> GDB?  So I propose to ignore this case until someone can come up with
> an actual scenario where that's useful.
> 
> The test passes for the unix, native-gdbserver, and
> native-extended-gdbserver targets.  When testing with either of the
> gdbserver targets, the test is run using the default empty sysroot,
> and also using the 'target:' sysroot.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29586
> 
> Co-authored-by: Andrew Burgess <aburgess@redhat.com>
> ---
>  gdb/solib.c                                |  58 +++++-
>  gdb/testsuite/gdb.base/solib-proc-self.cc  |  89 ++++++++++
>  gdb/testsuite/gdb.base/solib-proc-self.exp | 197 +++++++++++++++++++++
>  3 files changed, 340 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/solib-proc-self.cc
>  create mode 100644 gdb/testsuite/gdb.base/solib-proc-self.exp
> 
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 60bdf46cb91..3181b4d5f1f 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -50,6 +50,7 @@
>  #include "gdb_bfd.h"
>  #include "gdbsupport/filestuff.h"
>  #include "gdbsupport/scoped_fd.h"
> +#include "gdbsupport/pathstuff.h"
>  #include "debuginfod-support.h"
>  #include "source.h"
>  #include "cli/cli-style.h"
> @@ -79,6 +80,54 @@ show_solib_search_path (struct ui_file *file, int from_tty,
>  #  define DOS_BASED_FILE_SYSTEM 0
>  #endif
>  
> +/* Fix references to files in /proc/self/fd/ when opening a shared library.
> +
> +   SO_NAME is the name of the shared library being loaded.  This function
> +   returns a possibly modified name which should be used as the path to the
> +   shared library.
> +
> +   If SO_NAME starts with /proc/self, then the returned name will be
> +   modified to start with /proc/PID where 'PID' is the pid of the current
> +   inferior.  */
> +
> +static gdb::unique_xmalloc_ptr<char>
> +filter_proc_self_filenames (gdb::unique_xmalloc_ptr<char> so_name)
> +{
> +  /* In order to figure out if SO_NAME points at a file in /proc/self we
> +     need to canonicalize the path.  However, the whole point here is that
> +     if SO_NAME does point at /proc/self then GDB will think this is its
> +     self, while the inferior actually means its /proc/self.  A result of
> +     this is that the file being referenced within the inferior's
> +     /proc/self, might not exist within GDB's /proc/self, a call to
> +     gdb_realpath would then fail.
> +
> +     To avoid this problem we call gdb_realpath_keepfile, this only tries
> +     to canonicalize the basename of SO_NAME, and leaves the final filename
> +     untouched.  */
> +  so_name
> +    = make_unique_xstrdup (gdb_realpath_keepfile (so_name.get ()).c_str ());

gdb_realpath_keepfile resolves symlinks on the local filesystem, so how can
we be using it on target: files?  There's a comment exactly about this in exec.c:

      /* gdb_realpath_keepfile resolves symlinks on the local
	 filesystem and so cannot be used for "target:" files.  */
      gdb_assert (current_program_space->exec_filename == nullptr);
      if (load_via_target)
	current_program_space->exec_filename
	  = (make_unique_xstrdup
	     (bfd_get_filename (current_program_space->exec_bfd ())));
      else
	current_program_space->exec_filename
	  = make_unique_xstrdup (gdb_realpath_keepfile
				   (scratch_pathname).c_str ());


Also, what if the host is not a Linux host, if it is one where /proc/ does
not exist at all, such as e.g., a Windows host debugging a remote Linux target?
Won't gdb_realpath_keepfile fail in that scenario?

Also, do we get here with a relative pathname?  In that case, this will use
the CWD of GDB for path expansion, which just looks wrong.

I wonder whether using the linux namespace support wouldn't be a better approach.

Now how linux_nat_target::fileio_readlink calls linux_mntns_readlink:

 gdb::optional<std::string>
 linux_nat_target::fileio_readlink (struct inferior *inf, const char *filename,
 				   fileio_error *target_errno)
 {
 ...
   len = linux_mntns_readlink (linux_nat_fileio_pid_of (inf),
 			      filename, buf, sizeof (buf));
 ...

That means that the link is resolved in the namespace of the inferior.

GDBserver has similar code.

Also note:

 $ readlink -f /proc/self
 /proc/1957246

That suggests that we would just need to call target_fileio_readlink somewhere
to resolve the path.

That would leave core files to address, as you can't enter the namespace of
a process that is already gone.  But for cores, your solution is also not safe,
as opening "/proc/inferior_pid/" from the live host system is incorrect, that
PID might as well have been assigned to some other process, if it even exists.
Reading from /proc/pid/ files must be handled specially for core files.
So that leaves me to think that the target_fileio_readlink approach could work.

> +
> +  /* A result of the above canonicalization is that /proc/self will have
> +     been replaced with /proc/<pid of gdb>, so that's what we need to check
> +     for.  Of course, it could be the case that the inferior really did try
> +     to dlopen a file within GDB's /proc/<pid> directory, in which case
> +     we're going to do the wrong thing here, but that seems far less likely
> +     than calling dlopen on a file within the inferior's own directory.  */
> +  std::string prefix = string_printf ("/proc/%ld", (long) getpid ());
> +
> +  /* Is the canonical path inside GDB's /proc/<pid> directory?  */
> +  if (!startswith (so_name.get (), prefix.c_str ()))
> +    return so_name;

If GDB's pid is "1234", and so_name is "/proc/12345/something", this will think
the path is GDB's /proc/<pid> directory.  Need to append a slash to
PREFIX to avoid this.  Also need to check for exact "/proc/$GDB_PID" then.

> +
> +  /* Get the part of the path after /proc/<pid>.  For example given
> +     '/proc/123/fd' we find the '/fd' part.  */
> +  const char *tail = so_name.get () + strlen (prefix.c_str ());
> +
> +  /* Build a replacement path within the inferiors directory.  */
> +  int inferior_pid = inferior_ptid.pid ();
> +  return xstrprintf ("/proc/%d%s", inferior_pid, tail);
> +}
> +
>  /* Return the full pathname of a binary file (the main executable or a
>     shared library file), or NULL if not found.  If FD is non-NULL, *FD
>     is set to either -1 or an open file handle for the binary file.
> @@ -168,6 +217,9 @@ solib_find_1 (const char *in_pathname, int *fd, bool is_solib)
>  	}
>      }
>  
> +  temp_pathname.reset (xstrdup (in_pathname));
> +  temp_pathname = filter_proc_self_filenames (std::move (temp_pathname));

FWIW, this could be written as:

  temp_pathname = filter_proc_self_filenames (make_unique_xtrdup (in_pathname));

> +
>    /* Note, we're interested in IS_TARGET_ABSOLUTE_PATH, not
>       IS_ABSOLUTE_PATH.  The latter is for host paths only, while
>       IN_PATHNAME is a target path.  For example, if we're supposed to
> @@ -180,9 +232,7 @@ solib_find_1 (const char *in_pathname, int *fd, bool is_solib)
>         3rd attempt, c:/foo/bar.dll ==> /sysroot/foo/bar.dll
>    */
>  
> -  if (!IS_TARGET_ABSOLUTE_PATH (fskind, in_pathname) || sysroot == NULL)
> -    temp_pathname.reset (xstrdup (in_pathname));
> -  else
> +  if (IS_TARGET_ABSOLUTE_PATH (fskind, in_pathname) && sysroot != nullptr)
>      {
>        bool need_dir_separator;
>  
> @@ -209,7 +259,7 @@ solib_find_1 (const char *in_pathname, int *fd, bool is_solib)
>        /* Cat the prefixed pathname together.  */
>        temp_pathname.reset (concat (sysroot,
>  				   need_dir_separator ? SLASH_STRING : "",
> -				   in_pathname, (char *) NULL));
> +				   temp_pathname.get (), nullptr));
>      }
>  
>    /* Handle files to be accessed via the target.  */
> diff --git a/gdb/testsuite/gdb.base/solib-proc-self.cc b/gdb/testsuite/gdb.base/solib-proc-self.cc
> new file mode 100644
> index 00000000000..aea397a66f9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-proc-self.cc
> @@ -0,0 +1,89 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 Free Software Foundation, Inc.

Dates need update to 2022-2023.

> +
> +   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 <http://www.gnu.org/licenses/>.  */
> +
> +#include <sys/mman.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <iostream>
> +#include <fstream>
> +#include <sstream>
> +#include <vector>
> +#include <unistd.h>
> +
> +#ifdef __WIN32__
> +#include <windows.h>
> +#define dlopen(name, mode) LoadLibrary (name)
> +#define dlclose(handle) FreeLibrary (handle)
> +#define dlerror() "an error occurred"

/proc/self/ doesn't exit on Windows, so this seems pointless.

Maybe also add some require !istarget windows to the .exp file.


> +#else
> +#include <dlfcn.h>
> +#endif
> +
> +/* Combine PREFIX and NUM into a single string, joined with a '/', and
> +   return the string.  */
> +
> +std::string
> +make_library_path (const char *prefix, int num)
> +{
> +  std::stringstream stream;
> +  stream << prefix << "/" << num;
> +  return stream.str ();
> +}
> +
> +/* Call dlopen on the library pointed to by FILENAME.  */
> +
> +void
> +open_library (const std::string &filename)
> +{
> +  /* Call dlopen on FILENAME.  */
> +  void *handle = dlopen (filename.c_str (), RTLD_LAZY);
> +  if (handle == nullptr)
> +    abort ();
> +
> +  /* It worked.  */
> +  dlclose (handle);
> +}
> +
> +int
> +main ()
> +{
> +  /* Read the shared libraries contents into a buffer.  */
> +  std::ifstream read_so_file = std::ifstream (SHLIB_NAME);
> +  read_so_file.seekg (0, std::ios::end);
> +  std::streamsize size = read_so_file.tellg ();
> +  read_so_file.seekg (0, std::ios::beg);
> +  std::vector<char> buffer (size);
> +  if (!read_so_file.read (buffer.data (), size))
> +    abort ();
> +
> +  /* Create a memory mapped file, then write the shared library to that
> +     new memory mapped file.  */
> +  int mem_fd = memfd_create ("test", 0);
> +  write (mem_fd, buffer.data (), buffer.size ());

Missing error checking here.

> +
> +  /* Generate a canonical /proc/self/fd/[num] path for the memory mapped
> +     file, and call dlopen on it.  */
> +  std::string filename
> +    = make_library_path ("/proc/self/fd", mem_fd);	/* break-here */
> +  open_library (filename);
> +
> +  /* Now generate a new, non-canonical filename, and call dlopen on it.  */
> +  filename = make_library_path ("/proc/../proc/self/fd", mem_fd);
> +  open_library (filename);
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/solib-proc-self.exp b/gdb/testsuite/gdb.base/solib-proc-self.exp
> new file mode 100644
> index 00000000000..f026d2544e6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/solib-proc-self.exp
> @@ -0,0 +1,197 @@
> +# Copyright 2022 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 <http://www.gnu.org/licenses/>.  */
> +
> +# Test connecting and disconnecting at shared library events.

This comment is stale.

> +
> +require allow_shlib_tests
> +
> +standard_testfile .cc
> +
> +# Reuse an existing library, we don't care about the library contents
> +# for this test.
> +set libfile so-disc-shr
> +set libsrc "${srcdir}/${subdir}/${libfile}.c"
> +set libname "${libfile}.so"
> +set libobj [standard_output_file ${libname}]
> +
> +# Compile the shared library.
> +if { [gdb_compile_shlib $libsrc $libobj {debug}] != ""} {
> +    return -1
> +}
> +
> +# Compile the test executable.
> +if [ build_executable "failed to prepare" $testfile $srcfile \
> +	 [list shlib_load debug c++ additional_flags=-DSHLIB_NAME="${libobj}"]] {
> +    return -1
> +}
> +
> +proc validate_library_load {inferior_pid memfd} {
> +
> +    # Turn on the solib-events so we can see that gdb resolves
> +    # everything correctly.
> +    gdb_test_no_output "set stop-on-solib-events 1"
> +
> +    # Now continue forward until the solib event is detected, and
> +    # check that the loaded library is found through the /proc/PID/fd
> +    # rather than /proc/self/fd.
> +    #
> +    # We need to handle the possibility of the interesting event
> +    # showing the first, or second time we stop, as depending on which
> +    # mechanism GDB is using to handle the shared library events (the
> +    # newer probes based interface, or the old non-probes interface),
> +    # the library will be reported at the first or second stop.
> +    set saw_expected_event false
> +    set saw_no_event_stop false
> +    gdb_test_multiple "continue" "continue to solib evnt" {

"solib evnt" -> "solib event"

> +	-re "^continue\r\n" {
> +	    exp_continue
> +	}
> +
> +	-re "^Continuing\\.\r\n" {
> +	    exp_continue
> +	}
> +
> +	-early -re "Stopped due to shared library event \\(no libraries added or removed\\)\r\n" {
> +	    # This non-interesting event shows up first when using the
> +	    # probes based mechanism for dealing with shared library
> +	    # events.
> +	    #
> +	    # We set a flag here, and, once the prompt has appeared,
> +	    # we send another continue, the next event will contain
> +	    # the information we want.
> +	    set saw_no_event_stop true
> +	    exp_continue
> +	}
> +
> +	-re "Stopped due to shared library event:\r\n  Inferior loaded (?:target:)?/proc/${inferior_pid}/fd/$memfd\r\n" {
> +	    # This event, which includes the information we are
> +	    # looking for, occurs first when using the non-probes
> +	    # based mechanism for handling shared library events, and
> +	    # occurs second when using the probes mechanism.
> +	    #
> +	    # Either way, record here that we say the output we expect.
> +	    set saw_expected_event true
> +	    exp_continue
> +	}
> +
> +	-re "$::gdb_prompt $" {
> +	    if {$saw_no_event_stop} {
> +		set saw_no_event_stop false
> +		send_gdb "continue\n"
> +		exp_continue
> +	    } else {
> +		gdb_assert {$saw_expected_event} $gdb_test_name
> +	    }
> +	}
> +    }
> +
> +    # Turn off solib events.  We're only interested in validating the
> +    # loads for now.
> +    gdb_test_no_output "set stop-on-solib-events 0"
> +}
> +
> +# Start GDB and run to the point where the test program tries to dlopen a file
> +# from within /proc/self/fd/.  Catch the shared library event and check that
> +# we actually try to load a file from /proc/<INFERIOR-PID>/fd/.
> +#
> +# If SYSROOT is not the empty string, then this is set as the value of GDB's
> +# sysroot immediately after starting GDB.  The only value that is (currently)
> +# supported, other than the empty string, is 'target:'.
> +proc do_test { {sysroot ""} } {
> +    clean_restart $::binfile
> +
> +    if {$sysroot != ""} {
> +	gdb_test_no_output "set sysroot ${sysroot}"
> +    }
> +
> +    gdb_load_shlib $::libobj
> +
> +    if ![runto_main] then {
> +	return 0
> +    }
> +
> +    # Get inferior's PID for later.
> +    set inferior_pid [get_inferior_pid]
> +
> +    # Run to the 'break-here' marker.
> +    gdb_breakpoint [gdb_get_line_number "break-here"]
> +    gdb_continue_to_breakpoint "break-here" ".* break-here .*"
> +
> +    set memfd ""
> +    gdb_test_multiple "p mem_fd" "Get file descriptor" {

Lowercase "Get", please.

Pedro Alves

> +	-re -wrap "\\\$$::decimal = (\[^\r\n\]*)" {
> +	    set memfd $expect_out(1,string)
> +	    pass $gdb_test_name
> +	}
> +    }
> +
> +    # The first call to open_library (in the test program) is done
> +    # with a canonical path.
> +    with_test_prefix "canonical path" {
> +	gdb_breakpoint "open_library"
> +	gdb_continue_to_breakpoint "open_library"
> +	validate_library_load $inferior_pid $memfd
> +    }
> +
> +    # The second call to open_library (in the test program) is done
> +    # with a non-canonical path, however, GDB should resolve this to a
> +    # canonical path for display to the user, so the output we see
> +    # should be unchanged.
> +    with_test_prefix "non-canonical path" {
> +	gdb_breakpoint "open_library"
> +	gdb_continue_to_breakpoint "open_library"
> +	validate_library_load $inferior_pid $memfd
> +    }
> +}
> +
> +# First run of the test.
> +do_test
> +
> +# Possible second run of the test.  If we are using a remote target then we
> +# should consider setting the sysroot to 'target:' and re-running the test.
> +if {[target_info exists gdb_protocol]
> +    && ([target_info gdb_protocol] == "remote"
> +	|| [target_info gdb_protocol] == "extended-remote")} {
> +    # GDB will already be running after the first call to do_test, so we can
> +    # take a peek at the current sysroot setting, and decide if we should
> +    # repeat the test with a different setting.
> +
> +    set new_sysroot ""
> +    gdb_test_multiple "show sysroot" "" {
> +	-wrap -re "The current system root is \"\"\\." {
> +	    pass $gdb_test_name
> +
> +	    # Repeat the test with 'target:' sysroot.
> +	    set new_sysroot "target:"
> +	}
> +	-wrap -re "The current system root is \"target:\"\\." {
> +	    pass $gdb_test_name
> +
> +	    # Nothing else to do, we already tested with target: sysroot.
> +	}
> +	-re "$gdb_prompt $" {
> +	    pass $gdb_test_name
> +
> +	    # If already testing with any other sysroot, we probably should
> +	    # not try to adjust things, so don't do any further testing.
> +	}
> +    }
> +
> +    with_test_prefix "sysroot $new_sysroot" {
> +	if { $new_sysroot != "" } {
> +	    do_test $new_sysroot
> +	}
> +    }
> +}
> 
> base-commit: 7d02a94c8f74613edb0cdde11982b22eaaa9affe
>