Re: [PATCH v3 2/2] Make GDB resolve dlopen of memory mapped shared libraries

[Andrew Burgess <aburgess@redhat.com>]

Asaf Fisher via Gdb-patches <gdb-patches@sourceware.org> writes:

> Introduced `check_proc_self_file` that checks if a path used by
> inferior in dlopen is in the form of `/proc/self/...` and if so resolves
> it to `/proc/[pid]/...`
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29586

Hi Asaf,

Thanks for providing a fix for this, and thanks for submitting the
copyright assignment paperwork.

I took a look through your patch and ended up tweaking it a little.
I've attached a revised version below, which I'd love to hear your
feedback on.

When making changes my goal was to extend your fix to work with
gdbserver just like it works with native targets.  You can test
different gdbserver setups like this:

  make check-gdb TESTS="gdb.base/solib-proc-self.exp"  \
                 RUNTESTFLAGS="--target_board=native-extended-gdbserver"

This will setup gdbserver and connect GDB to it with 'target
extended-remote', you can also test using the 'native-gdbserver' board,
which will connect to gdbserver as just 'target remote'.

Ideally any new test will pass in all three configurations (the default
plus the two listed above), and initially, your change only fixed the
native target case.  By moving the fix elsewhere in the shared library
loading process I think I new have all three cases working.

I also extended the test to test using a 'target:' sysroot.  By default
when testing with the two gdbserver boards above, we set the sysroot to
"" (the empty string), this tells GDB that remote files can be found on
the local machine, and avoids all files accesses having to go over the
remote protocol.  However, given the nature of this change, I figured it
was worth testing with the 'target:' sysroot too, this means we re-run
the tests bu sending all file accesses over the remote protocol.  That
case is also fixed with the patch below.

Where I'd most appreciate your feedback is for the algorithm by which
the /proc/self path is spotted and converted to /proc/PID.  I've cut the
code back a bit from what you had originally, mostly because I couldn't
find a way to test that the extra complexity was required.  If you have
any additional test cases that could show that the slimmed down code is
not good enough, then that would be great.

Thanks,
Andrew

---

commit 0bf4c98bc225c89a0a1ddcea727eca178ad69710
Author: Asaf Fisher <asaffisher.dev@gmail.com>
Date:   Fri Oct 21 17:42:05 2022 +0000

    gdb: handle loading shared libraries from /proc/self/fd/
    
    Bug PR gdb/29586 describes a situations where a shared library is
    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.
    
    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>

diff --git a/gdb/solib.c b/gdb/solib.c
index 7cfdd81114c..cf2d0d3bc3a 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -83,6 +83,35 @@ 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)
+{
+  static const char *proc_self_prefix = "/proc/self";
+
+  /* Is the path really a /proc/self?  */
+  if (!startswith (so_name.get (), proc_self_prefix))
+    return so_name;
+
+  /* Get the part of the path after /proc/self.  For example given
+     '/proc/self/fd' we find the '/fd' part.  */
+  gdb_assert (strlen (so_name.get ()) >= strlen (proc_self_prefix));
+  const char *tail = so_name.get () + strlen (proc_self_prefix);
+
+  /* Build a replacement path.  */
+  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.
@@ -172,6 +201,12 @@ solib_find_1 (const char *in_pathname, int *fd, bool is_solib)
 	}
     }
 
+  /* If the path starts /proc/self then rewrite this as /proc/PID using the
+     current inferior's pid.  Failing to do this will cause GDB to try and
+     open files within its proc directory, rather than the inferiors.  */
+  temp_pathname.reset (xstrdup (in_pathname));
+  temp_pathname = filter_proc_self_filenames (std::move (temp_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
@@ -184,9 +219,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 != NULL)
     {
       bool need_dir_separator;
 
@@ -213,7 +246,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 (), (char *) NULL));
     }
 
   /* 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..f2439d738a3
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-proc-self.cc
@@ -0,0 +1,73 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+#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"
+#else
+#include <dlfcn.h>
+#endif
+
+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))
+    {
+      fprintf (stderr, "Failed to load solib\n");
+      exit (1);
+    }
+
+  /* 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 ());
+
+  /* Generate the /proc/self/fd/[num] path for the memory mapped file.  */
+  std::string proc_self_fd_path;		/* break-here */
+  std::stringstream proc_self_fd_path_stream
+    = std::stringstream (proc_self_fd_path);
+  proc_self_fd_path_stream << "/proc/self/fd/" << mem_fd;
+
+  /* Call dlopen on it.  */
+  void *handle = dlopen (proc_self_fd_path_stream.str ().c_str (), RTLD_LAZY);
+  if (!handle)
+    {
+      fprintf (stderr, "%s\n", dlerror ());
+      exit (1);
+    }
+  /* It worked.  */
+  dlclose (handle);
+
+  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..1c845822490
--- /dev/null
+++ b/gdb/testsuite/gdb.base/solib-proc-self.exp
@@ -0,0 +1,130 @@
+# 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.
+
+if {[skip_shlib_tests]} {
+    untested "could not run to main"
+    return 0
+}
+
+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
+}
+
+# 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]
+
+    # Turn on the solib-events so we can see that gdb resolves everything
+    # correctly.
+    gdb_test_no_output "set stop-on-solib-events 1"
+
+    # 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" {
+	-re -wrap "\\\$$::decimal = (\[^\r\n\]*)" {
+	    set memfd $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+
+    gdb_test "continue" "Stopped due to shared library event.*" \
+	"continue to load"
+
+    # Check if inferior resolved the /proc/self/fd/[num] to /proc/[pid]/fd/[num].
+    gdb_test "continue" \
+	[multi_line \
+	     "Stopped due to shared library event:" \
+	     "  Inferior loaded (?:target:)?/proc/${inferior_pid}/fd/$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
+	}
+    }
+}