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

[Dr Lancelot SIX <lsix@lancelotsix.com>]

[-- Attachment #1: Type: text/plain, Size: 16148 bytes --]

Hi Asaf and Andrew,

Thanks for working on this! I have included comments below in the patch.

On 10/11/2022 19:37, Andrew Burgess via Gdb-patches wrote:
> 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
>      gdb.base/solib-proc-self.exp
>      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));

I am not sure how this assert can ever fail as the test just before 
checked that so_name starts with proc_self_prefix.  How can it have a 
smaller length?

> +  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)

Just a nit, but you could s/NULL/nullptr while at updating this line

>       {
>         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));
Same here. Also, could nullptr make it so the "(char *)" cast becomes 
useless?
>       }
>   
>     /* 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"

I've been running this patch (as modified by Andrew) and see a failure 
in the tests.

(gdb) PASS: gdb.base/solib-proc-self.exp: continue to breakpoint: break-here
p mem_fd
$1 = 4
(gdb) PASS: gdb.base/solib-proc-self.exp: Get file descriptor
continue
Continuing.
Stopped due to shared library event:
   Inferior loaded /proc/7331/fd/4
(gdb) PASS: gdb.base/solib-proc-self.exp: continue to load
continue
Continuing.
Stopped due to shared library event (no libraries added or removed)
(gdb) FAIL: gdb.base/solib-proc-self.exp: continue

I believe this "continue" should be removed.  The test passes cleanly 
without it.

Best,
Lancelot.

> +
> +    # 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
> +	}
> +    }
> +}
>