From: Andrew Burgess <aburgess@redhat.com>
To: Dr Lancelot SIX <lsix@lancelotsix.com>,
Asaf Fisher via Gdb-patches <gdb-patches@sourceware.org>
Cc: Asaf Fisher <asaffisher.dev@gmail.com>
Subject: Re: [PATCH v3 2/2] Make GDB resolve dlopen of memory mapped shared libraries
Date: Mon, 21 Nov 2022 11:55:56 +0000 [thread overview]
Message-ID: <87zgck7c8j.fsf@redhat.com> (raw)
In-Reply-To: <a4485b56-3607-8e58-8c2c-083656bd6ded@lancelotsix.com>
Dr Lancelot SIX <lsix@lancelotsix.com> writes:
> 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?
>
Removed.
>> + 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
>
Fixed.
>> {
>> 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?
Fixed.
>> }
>>
>> /* 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.
I looked at this failure a little, turns out, for some reason, I'm
seeing two solib stop events related to the dlopen call, here's what my
gdb.log looks like:
...
(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 (no libraries added or removed)
(gdb) PASS: gdb.base/solib-proc-self.exp: continue to load
continue
Continuing.
Stopped due to shared library event:
Inferior loaded /proc/2891392/fd/4
(gdb) PASS: gdb.base/solib-proc-self.exp: continue
Here's the inferior backtrace at the first (no libraries added or
removed) stop:
#0 0x00007ffff7fd85da in _dl_map_object_from_fd () from /lib64/ld-linux-x86-64.so.2
#1 0x00007ffff7fdb366 in _dl_map_object () from /lib64/ld-linux-x86-64.so.2
#2 0x00007ffff7fe5d55 in dl_open_worker () from /lib64/ld-linux-x86-64.so.2
#3 0x00007ffff7ba24f8 in _dl_catch_exception () from /lib64/libc.so.6
#4 0x00007ffff7fe58fe in _dl_open () from /lib64/ld-linux-x86-64.so.2
#5 0x00007ffff7f8c39c in dlopen_doit () from /lib64/libdl.so.2
#6 0x00007ffff7ba24f8 in _dl_catch_exception () from /lib64/libc.so.6
#7 0x00007ffff7ba25c3 in _dl_catch_error () from /lib64/libc.so.6
#8 0x00007ffff7f8cb09 in _dlerror_run () from /lib64/libdl.so.2
#9 0x00007ffff7f8c42a in dlopen@@GLIBC_2.2.5 () from /lib64/libdl.so.2
#10 0x0000000000402555 in main () at /tmp/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/solib-proc-self.cc:63
And the backtrace at the second stop:
#0 0x00007ffff7fe60a8 in dl_open_worker () from /lib64/ld-linux-x86-64.so.2
#1 0x00007ffff7ba24f8 in _dl_catch_exception () from /lib64/libc.so.6
#2 0x00007ffff7fe58fe in _dl_open () from /lib64/ld-linux-x86-64.so.2
#3 0x00007ffff7f8c39c in dlopen_doit () from /lib64/libdl.so.2
#4 0x00007ffff7ba24f8 in _dl_catch_exception () from /lib64/libc.so.6
#5 0x00007ffff7ba25c3 in _dl_catch_error () from /lib64/libc.so.6
#6 0x00007ffff7f8cb09 in _dlerror_run () from /lib64/libdl.so.2
#7 0x00007ffff7f8c42a in dlopen@@GLIBC_2.2.5 () from /lib64/libdl.so.2
#8 0x0000000000402555 in main () at /tmp/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/solib-proc-self.cc:63
I'm going to fold the two continue calls together into a
gdb_test_multiple, but I'll also dig into why I see two stop events,
maybe there's a GDB issue here.
Thanks,
Andrew
>
> 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
>> + }
>> + }
>> +}
>>
next prev parent reply other threads:[~2022-11-21 11:56 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-21 17:42 [PATCH v3 1/2] Add test to check GDB handles dlopen of /proc/self/fd/[num] correctly Asaf Fisher
2022-10-21 17:42 ` [PATCH v3 2/2] Make GDB resolve dlopen of memory mapped shared libraries Asaf Fisher
2022-11-10 19:37 ` Andrew Burgess
2022-11-11 12:35 ` Dr Lancelot SIX
2022-11-11 12:47 ` Asaf Fisher
2022-11-21 11:55 ` Andrew Burgess [this message]
2022-11-21 17:56 ` [PATCHv4] gdb: handle loading shared libraries from /proc/self/fd/ Andrew Burgess
2022-12-14 11:51 ` Andrew Burgess
2022-12-15 16:44 ` [PATCHv5] " Andrew Burgess
2022-12-16 16:59 ` Asaf Fisher
2023-01-20 12:33 ` [PATCHv6] " Andrew Burgess
2023-01-25 13:30 ` Pedro Alves
2022-10-24 10:45 ` [PATCH v3 1/2] Add test to check GDB handles dlopen of /proc/self/fd/[num] correctly Andrew Burgess
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zgck7c8j.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=asaffisher.dev@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=lsix@lancelotsix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).