From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oo1-xc2c.google.com (mail-oo1-xc2c.google.com [IPv6:2607:f8b0:4864:20::c2c]) by sourceware.org (Postfix) with ESMTPS id 589E93858D1E for ; Fri, 11 Nov 2022 12:47:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 589E93858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oo1-xc2c.google.com with SMTP id j1-20020a4ad181000000b0049e6e8c13b4so641413oor.1 for ; Fri, 11 Nov 2022 04:47:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=uMiqR/LCuHzx0WGlVrBtLDGwz4eWWP45IKI/O1KEWmU=; b=kDB5WC+wBhE5d0yuaSTyw/S0l0AT7t1f3Nf8upcwKZpev/xMLp+2nD4LUHG40T+aDA 9X0OtdvEXdin9+NsHPGaGEX0xKtWv3UeFq+5/wHBayXp1Mp0ZlBzz5uba4D8UNkiW2f1 UDO8P/JfEQvAoS3HjSTg88wfF7FPgH72m0qswmhWb6tm9XrBd8EGO2r5K46NcsbYpRHQ PUjiOU5tqj6SuZ5V36KTT4qs/bHPlciq4gj8YQUw3TK5Bnj0mwh8QA4D0DhvhbzYfvC3 0nKrbejPN9ikYFd0rqeucDGjBk/JnQUn7qR8UxCDno//1IaApOMm+l9llufcc8liU2i7 Loqg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=uMiqR/LCuHzx0WGlVrBtLDGwz4eWWP45IKI/O1KEWmU=; b=hGy9DLOX5skaszJ9QrbkdiVpDxqycyG3e3yeL2Kk8CCCL4VGuMSGtdCjLwyb97+2yF 18dyU9LsNjBKDJOXKUmq6HVaOF+58FPMHQ0+5ChQT0bq1aibXEVjB2BhIiKJ2gijLrit itfHUOmNp76MjnjRx/zf0L6IFG5Iofe3eI1Y8x9T2ogJPFitH2ctVjTFxuUsqo2KJtpf IpAbQ2p7QmkdsUQzxBy7xYB5NxQvYtYLbFpHXiUSvHjDu94oxns3/Q+NlIY6b/SgI/ED DPblshFCEK/uYhsBmpyPI/C7USXUHsVeWWctCFGXbXpUHtcQ2SnVJdJJmWIRMld4HOar bpuQ== X-Gm-Message-State: ANoB5plpWExMYjYWbQyxvrzLbEjcrEn/E0T8cbFcwFOY67c90IBUUsDS C2CcFdt6CBDafi2oaKPRCSrCt3wrJQ3hAUnFFdjXMcbI3Ak= X-Google-Smtp-Source: AA0mqf5uWEkwwNyr+IJmhg3PQuJnOEpDAX6XCvJIgeNTVhTQ0OTKYfPt8Q/OC1TbN6+m642DbCfOPkBtgycJLy7/FmE= X-Received: by 2002:a4a:3c5a:0:b0:47f:9499:931c with SMTP id p26-20020a4a3c5a000000b0047f9499931cmr706083oof.81.1668170856406; Fri, 11 Nov 2022 04:47:36 -0800 (PST) MIME-Version: 1.0 References: <20221021174205.5389-1-asaffisher.dev@gmail.com> <20221021174205.5389-2-asaffisher.dev@gmail.com> <87sfiqa9ec.fsf@redhat.com> In-Reply-To: From: Asaf Fisher Date: Fri, 11 Nov 2022 14:47:25 +0200 Message-ID: Subject: Re: [PATCH v3 2/2] Make GDB resolve dlopen of memory mapped shared libraries To: Dr Lancelot SIX Cc: Andrew Burgess , Asaf Fisher via Gdb-patches Content-Type: multipart/alternative; boundary="00000000000052236805ed314bf1" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,HTML_MESSAGE,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --00000000000052236805ed314bf1 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I=E2=80=99ll be able to review it right after the 22nd of November. On Fri, Nov 11, 2022 at 2:35 PM Dr Lancelot SIX wrote: > 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 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=3D29586 > > 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=3D"gdb.base/solib-proc-self.exp" \ > RUNTESTFLAGS=3D"--target_board=3Dnative-extended-gdbserv= er" > > 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 > 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/", 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/, where 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=3D29586 > > gdb.base/solib-proc-self.exp > Co-authored-by: Andrew Burgess > > > 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 libra= ry. > + > + 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 > +filter_proc_self_filenames (gdb::unique_xmalloc_ptr so_name) > +{ > + static const char *proc_self_prefix =3D "/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 ()) >=3D 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 =3D so_name.get () + strlen (proc_self_prefix); > + > + /* Build a replacement path. */ > + int inferior_pid =3D 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 a= nd > + open files within its proc directory, rather than the inferiors. */ > + temp_pathname.reset (xstrdup (in_pathname)); > + temp_pathname =3D 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 =3D=3D> /sysroot/foo/bar.dll > */ > > - if (!IS_TARGET_ABSOLUTE_PATH (fskind, in_pathname) || sysroot =3D=3D N= ULL) > - temp_pathname.reset (xstrdup (in_pathname)); > - else > + if (IS_TARGET_ABSOLUTE_PATH (fskind, in_pathname) && sysroot !=3D 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/gd= b.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 = . */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#ifdef __WIN32__ > +#include > +#define dlopen(name, mode) LoadLibrary (name) > +#define dlclose(handle) FreeLibrary (handle) > +#define dlerror() "an error occurred" > +#else > +#include > +#endif > + > +int > +main () > +{ > + /* Read the shared libraries contents into a buffer. */ > + std::ifstream read_so_file =3D std::ifstream (SHLIB_NAME); > + read_so_file.seekg (0, std::ios::end); > + std::streamsize size =3D read_so_file.tellg (); > + read_so_file.seekg (0, std::ios::beg); > + std::vector 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 =3D 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 > + =3D std::stringstream (proc_self_fd_path); > + proc_self_fd_path_stream << "/proc/self/fd/" << mem_fd; > + > + /* Call dlopen on it. */ > + void *handle =3D dlopen (proc_self_fd_path_stream.str ().c_str (), RTL= D_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/g= db.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}] !=3D ""} { > + return -1 > +} > + > +# Compile the test executable. > +if [ build_executable "failed to prepare" $testfile $srcfile \ > + [list shlib_load debug c++ additional_flags=3D-DSHLIB_NAME=3D"${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//fd/. > +# > +# If SYSROOT is not the empty string, then this is set as the value of G= DB's > +# sysroot immediately after starting GDB. The only value that is (curre= ntly) > +# supported, other than the empty string, is 'target:'. > +proc do_test { {sysroot ""} } { > + clean_restart $::binfile > + > + if {$sysroot !=3D ""} { > + 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 =3D (\[^\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-h= ere > p mem_fd > $1 =3D 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 te= st. > +if {[target_info exists gdb_protocol] > + && ([target_info gdb_protocol] =3D=3D "remote" > + || [target_info gdb_protocol] =3D=3D "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 !=3D "" } { > + do_test $new_sysroot > + } > + } > +} > > > --00000000000052236805ed314bf1--