From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id DA5973858402 for ; Wed, 4 Oct 2023 01:29:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DA5973858402 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1696382976; bh=7eorbdx7mJVpHV6TFkQr/ZtzviaWS5D843pZQ1v0nZE=; h=Date:Subject:To:References:From:In-Reply-To:From; b=jE2g1lkM8fEERjijDibyphnWKndY/meW8sMdIE+JoHyvIx9PKUZSCDQM3Bhyllb8a jhhFDGSZM60uNPpIR4kEuXvNiFFjmwV9TDFtY3hPAy2qho6Jse9HKokawqY/uhkIR1 7HU30YotwDHtFaCgNI4ou3362+ov+FrDYvsEURAo= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 3E67D1E092; Tue, 3 Oct 2023 21:29:36 -0400 (EDT) Message-ID: <0074e2b4-7ff8-4688-af51-289d9a3b5711@simark.ca> Date: Tue, 3 Oct 2023 21:29:35 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 3/3] Bail out of "attach" if a thread cannot be traced Content-Language: en-US To: Tom Tromey , gdb-patches@sourceware.org References: <20231003-attach-bug-v1-0-f5de2e583c5d@adacore.com> <20231003-attach-bug-v1-3-f5de2e583c5d@adacore.com> From: Simon Marchi In-Reply-To: <20231003-attach-bug-v1-3-f5de2e583c5d@adacore.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,SPF_HELO_PASS,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: On 2023-10-03 13:56, Tom Tromey via Gdb-patches wrote: > On Linux, threads are treated much like separate processes by the > kernel. In particular, it's possible to ptrace just a single thread. > If gdb tries to attach to a multi-threaded inferior, where a non-main > thread is already being traced (e.g., by strace), then gdb will get > into an infinite loop attempting to attach. Can you explain why GDB falls in an infinite loop? > > This patch fixes this problem by having the attach fail if ptrace > fails to attach to any thread of the inferior. I don't rely on that behavior myself, but I would think that the current behavior of just warning when failing to attach to a thread was on purpose, at least it allows the user to debug the other threads. It might be useful in some edge case scenarios I guess? > --- > gdb/linux-nat.c | 49 +++++++++++---- > gdb/testsuite/gdb.base/traced-thread.c | 105 +++++++++++++++++++++++++++++++ > gdb/testsuite/gdb.base/traced-thread.exp | 54 ++++++++++++++++ > gdbserver/linux-low.cc | 15 ++++- > 4 files changed, 210 insertions(+), 13 deletions(-) > > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c > index 7e36ced6292..5974ab95390 100644 > --- a/gdb/linux-nat.c > +++ b/gdb/linux-nat.c > @@ -1024,8 +1024,8 @@ attach_proc_task_lwp_callback (ptid_t ptid) > std::string reason > = linux_ptrace_attach_fail_reason_string (ptid, err); > > - warning (_("Cannot attach to lwp %d: %s"), > - lwpid, reason.c_str ()); > + error (_("Cannot attach to lwp %d: %s"), > + lwpid, reason.c_str ()); > } > } > else > @@ -1045,13 +1045,6 @@ attach_proc_task_lwp_callback (ptid_t ptid) > > /* So that wait collects the SIGSTOP. */ > lp->resumed = 1; > - > - /* Also add the LWP to gdb's thread list, in case a > - matching libthread_db is not found (or the process uses > - raw clone). */ > - add_thread (linux_target, lp->ptid); > - set_running (linux_target, lp->ptid, true); > - set_executing (linux_target, lp->ptid, true); > } > > return 1; > @@ -1146,8 +1139,42 @@ linux_nat_target::attach (const char *args, int from_tty) > of threads/LWPs, and those structures may well be corrupted. > Note that once thread_db is loaded, we'll still use it to list > threads and associate pthread info with each LWP. */ > - linux_proc_attach_tgid_threads (lp->ptid.pid (), > - attach_proc_task_lwp_callback); > + try > + { > + linux_proc_attach_tgid_threads (lp->ptid.pid (), > + attach_proc_task_lwp_callback); > + } > + catch (const gdb_exception_error &) > + { > + /* Failed to attach to some LWP. Detach any we've already > + attached to. */ > + iterate_over_lwps (ptid_t (ptid.pid ()), > + [] (struct lwp_info *lwp) -> int > + { > + /* Ignore errors when detaching. */ > + ptrace (PTRACE_DETACH, lwp->ptid.lwp (), 0, 0); > + delete_lwp (lwp->ptid); > + return 0; > + }); > + > + target_terminal::ours (); > + target_mourn_inferior (inferior_ptid); I'm wondering if the target_mourn_inferior could be done in back in generic code, it sounds relevant for any target failing to attach. Another thing to consider when attach fails: inf_ptrace_target::attach takes care of unpushing itself if it fails and it wasn't pushed initially. Should linux_nat_target::attach do the same? This is something that could maybe get checked in the test by checking the output of "maint print target-stack". > + > + throw; > + } > + > + /* Add all the LWPs to gdb's thread list. */ > + iterate_over_lwps (ptid_t (ptid.pid ()), > + [] (struct lwp_info *lwp) -> int > + { > + if (lwp->ptid.pid () != lwp->ptid.lwp ()) > + { > + add_thread (linux_target, lwp->ptid); > + set_running (linux_target, lwp->ptid, true); > + set_executing (linux_target, lwp->ptid, true); > + } > + return 0; > + }); > } > > /* Ptrace-detach the thread with pid PID. */ > diff --git a/gdb/testsuite/gdb.base/traced-thread.c b/gdb/testsuite/gdb.base/traced-thread.c > new file mode 100644 > index 00000000000..f30e09b1fc5 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/traced-thread.c > @@ -0,0 +1,105 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2023 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 . */ > + > +#define _GNU_SOURCE > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +int fds[2]; > + > +_Atomic pid_t bg_tid = 0; > + > +pthread_barrier_t barrier; > + > +#define FIVE_MINUTES (5 * 60) > + > +/* One thread of the child process. This is traced by the parent > + process. */ > +void * > +block (void *ignore) > +{ > + bg_tid = gettid (); > + pthread_barrier_wait (&barrier); > + sleep (FIVE_MINUTES); > + return 0; > +} > + > +/* The parent process blocks in this function. */ > +void > +parent_stop (pid_t child_thread_pid) > +{ > + sleep (FIVE_MINUTES); > +} > + > +int > +main () > +{ > + int result; > + > + pthread_barrier_init (&barrier, NULL, 2); > + > + result = pipe (fds); > + assert (result != -1); > + > + pid_t child = fork (); > + if (child != 0) > + { > + /* Parent. */ > + close (fds[1]); > + > + pid_t the_tid; > + result = read (fds[0], &the_tid, sizeof (the_tid)); > + assert (result == sizeof (the_tid)); > + > + /* Trace a single, non-main thread of the child. This should > + prevent gdb from attaching to the child at all. The bug here > + was that gdb would get into an infinite loop repeatedly > + trying to attach to this thread. */ > + result = ptrace (PTRACE_SEIZE, the_tid, (void *) 0, (void *) 0); > + if (result == -1) > + perror ("ptrace"); > + > + parent_stop (child); > + } > + else > + { > + /* Child. */ > + > + close (fds[0]); > + > + pthread_t thr; > + result = pthread_create (&thr, 0, block, 0); > + assert (result == 0); > + > + /* Wait until the TID has been assigned. */ > + pthread_barrier_wait (&barrier); > + assert (bg_tid != 0); > + > + result = write (fds[1], &bg_tid, sizeof (bg_tid)); > + assert (result == sizeof (bg_tid)); > + > + sleep (FIVE_MINUTES); > + } > + > + exit (0); > +} > diff --git a/gdb/testsuite/gdb.base/traced-thread.exp b/gdb/testsuite/gdb.base/traced-thread.exp > new file mode 100644 > index 00000000000..d590ab1bc5e > --- /dev/null > +++ b/gdb/testsuite/gdb.base/traced-thread.exp > @@ -0,0 +1,54 @@ > +# Copyright 2023 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 . */ > + > +# This test only works on GNU/Linux. > +require !use_gdb_stub isnative > +require {!is_remote host} > +require {istarget *-linux*} Can you add a short description at the top to indicate the intent of the test? > + > +standard_testfile > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile \ > + {debug pthreads}]} { > + return -1 > +} > + > +if {![runto "parent_stop"]} { > + return -1 > +} > + > +# We don't want to be bothered. > +gdb_test_no_output "set confirm off" > + > +set child_pid [get_integer_valueof child_thread_pid -1 "get child pid"] > + > +# We should not be able to attach to the child at all, because one > +# thread is being traced. The bug here was that gdb would get into an > +# infinite loop trying to attach to this thread. > +gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2" > +gdb_test "inferior 2" "Switching to inferior 2.*" "switch to inferior 2" > +# Recognize failures from either gdb or gdbserver. > +gdb_test "attach $child_pid" \ > + "(Cannot attach to|Attaching to process $decimal failed).*" \ > + "should not attach to child process" > + > + > +# Now kill the parent process, ending the trace. > +gdb_test "inferior 1" "Switching to inferior 1.*" "switch to inferior 1" > +gdb_test "kill" ".*" "kill the parent process" > + > +# Kill the child process as well. Use the shell to avoid funny > +# business with gdbserver testing. > +remote_exec target "kill -9 $child_pid" To further verify that the child we failed to attach to was left in a good state, it might be good to check if GDB can successfully attach it after killing the parent. And while at it, we could kill the child using GDB's kill command too. Simon