From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id 4526B3857C79 for ; Wed, 24 Nov 2021 20:04:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4526B3857C79 X-ASG-Debug-ID: 1637784285-0c856e2e472af070001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id hcCl0DdpINKllgpz (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 24 Nov 2021 15:04:45 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@efficios.com X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from smarchi-efficios.internal.efficios.com (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) by smtp.ebox.ca (Postfix) with ESMTP id A60E2441D64; Wed, 24 Nov 2021 15:04:45 -0500 (EST) From: Simon Marchi X-Barracuda-RBL-IP: 192.222.180.24 X-Barracuda-Effective-Source-IP: 192-222-180-24.qc.cable.ebox.net[192.222.180.24] X-Barracuda-Apparent-Source-IP: 192.222.180.24 To: gdb-patches@sourceware.org Subject: [PATCH 1/3] gdbserver: hide fork child threads from GDB Date: Wed, 24 Nov 2021 15:04:42 -0500 X-ASG-Orig-Subj: [PATCH 1/3] gdbserver: hide fork child threads from GDB Message-Id: <20211124200444.614978-2-simon.marchi@efficios.com> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20211124200444.614978-1-simon.marchi@efficios.com> References: <20211029203332.69894-1-simon.marchi@polymtl.ca> <20211124200444.614978-1-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1637784285 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 17021 X-Barracuda-BRTS-Status: 1 X-Barracuda-BRTS-Evidence: simark.ca X-Barracuda-Spam-Score: 0.40 X-Barracuda-Spam-Status: No, SCORE=0.40 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests=BSF_SC0_SA085b X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.94175 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.40 BSF_SC0_SA085b Custom Rule SA085b X-Spam-Status: No, score=-19.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_SOFTFAIL, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Nov 2021 20:05:01 -0000 From: Simon Marchi This patch aims at fixing a bug where an inferior is unexpectedly created when a fork happens at the same time as another event, and that other event is reported to GDB first (and the fork event stays pending in GDBserver). This happens for example when we step a thread and another thread forks at the same time. The bug looks like (if I reproduce the included test by hand): (gdb) show detach-on-fork Whether gdb will detach the child of a fork is on. (gdb) show follow-fork-mode Debugger response to a program call of fork or vfork is "parent". (gdb) si [New inferior 2] Reading /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread from remote target... Reading /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread from remote target... Reading symbols from target:/home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread... [New Thread 965190.965190] [Switching to Thread 965190.965190] Remote 'g' packet reply is too long (expected 560 bytes, got 816 bytes): ... The sequence of events leading to the problem is: - We are using the all-stop user-visible mode as well as the synchronous / all-stop variant of the remote protocol - We have two threads, thread A that we single-step and thread B that calls fork at the same time - GDBserver's linux_process_target::wait pulls the "single step complete SIGTRAP" and the "fork" events from the kernel. It arbitrarily choses one event to report, it happens to be the single-step SIGTRAP. The fork stays pending in the thread_info. - GDBserver send that SIGTRAP as a stop reply to GDB - While in stop_all_threads, GDB calls update_thread_list, which ends up querying the remote thread list using qXfer:threads:read. - In the reply, GDBserver includes the fork child created as a result of thread B's fork. - GDB-side, the remote target sees the new PID, calls remote_notice_new_inferior, which ends up unexpectedly creating a new inferior, and things go downhill from there. The problem here is that as long as GDB did not process the fork event, it should pretend the fork child does not exist. Ultimately, this event will be reported, we'll go through follow_fork, and that process will be detached. The remote target (GDB-side), has some code to remove from the reported thread list the threads that are the result of forks not processed by GDB yet. But that only works for fork events that have made their way to the remote target (GDB-side), but haven't been consumed by the core yet, so are still lingering as pending stop replies in the remote target (see remove_new_fork_children in remote.c). But in our case, the fork event hasn't made its way to the GDB-side remote target. We need to implement the same kind of logic GDBserver-side: if there exists a thread / inferior that is the result of a fork event GDBserver hasn't reported yet, it should exclude that thread / inferior from the reported thread list. This was actually discussed a while ago, but not implemented AFAIK: https://pi.simark.ca/gdb-patches/1ad9f5a8-d00e-9a26-b0c9-3f4066af5142@redhat.com/#t https://sourceware.org/pipermail/gdb-patches/2016-June/133906.html Implementation details-wise, the fix for this is all in GDBserver. The Linux layer of GDBserver already tracks unreported fork parent / child relationships using the lwp_info::fork_relative, in order to avoid wildcard actions resuming fork childs unknown to GDB. Move this information to the common thread_info structure, so that it can be used in handle_qxfer_threads_worker to filter out unreported fork child threads. Plus, that makes it usable for other OSes that use fork if need be. I split the field in two (fork_parent / fork_child), I think it's clearer this way, easier to follow which thread is the parent and which is the child, and helps ensure things are consistent. That simplifies things a bit in linux_set_resume_request. Currently, when lwp->fork_relative is set, we have to deduce whether this is a parent or child using the pending event. With separate fork_parent and fork_child fields, it becomes more obvious. If the thread has a fork parent, then it means it's a fork child, and vice-versa. Testing-wise, the test replicates pretty-much the sequence of events shown above. The setup of the test makes it such that the main thread is about to fork. We stepi the other thread, so that the step completes very quickly, in a single event. Meanwhile, the main thread is resumed, so very likely has time to call fork. This means that the bug may not reproduce every time (if the main thread does not have time to call fork), but it will reproduce more often than not. The test fails without the fix applied on the native-gdbserver and native-extended-gdbserver boards. At some point I suspected that which thread called fork and which thread did the step influenced the order in which the events were reported, and therefore the reproducibility of the bug. So I made the test try both combinations: main thread forks while other thread steps, and vice versa. I'm not sure this is still necessary, but I left it there anyway. It doesn't hurt to test a few more combinations. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28288 Change-Id: I2158d5732fc7d7ca06b0eb01f88cf27bf527b990 --- .../gdb.threads/pending-fork-event.c | 82 ++++++++++++++++++ .../gdb.threads/pending-fork-event.exp | 86 +++++++++++++++++++ gdbserver/gdbthread.h | 9 ++ gdbserver/linux-low.cc | 36 ++++---- gdbserver/linux-low.h | 6 -- gdbserver/server.cc | 6 ++ 6 files changed, 202 insertions(+), 23 deletions(-) create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.c create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.exp diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.c b/gdb/testsuite/gdb.threads/pending-fork-event.c new file mode 100644 index 000000000000..a39ca75a49ac --- /dev/null +++ b/gdb/testsuite/gdb.threads/pending-fork-event.c @@ -0,0 +1,82 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2021 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 + +static volatile int release_forking_thread = 0; +static int x; +static pthread_barrier_t barrier; + +static void +break_here (void) +{ + x++; +} + +static void +do_fork (void) +{ + pthread_barrier_wait (&barrier); + + while (!release_forking_thread); + + if (FORK_FUNCTION () == 0) + _exit (0); + +} + +static void * +thread_func (void *p) +{ +#if defined(MAIN_THREAD_FORKS) + break_here (); +#elif defined(OTHER_THREAD_FORKS) + do_fork (); +#else +# error "MAIN_THREAD_FORKS or OTHER_THREAD_FORKS must be defined" +#endif +} + + +int +main (void) +{ + pthread_t thread; + int ret; + + pthread_barrier_init (&barrier, NULL, 2); + + alarm (30); + ret = pthread_create (&thread, NULL, thread_func, NULL); + assert (ret == 0); + + pthread_barrier_wait (&barrier); + +#if defined(MAIN_THREAD_FORKS) + do_fork (); +#elif defined(OTHER_THREAD_FORKS) + break_here (); +#else +# error "MAIN_THREAD_FORKS or OTHER_THREAD_FORKS must be defined" +#endif + + pthread_join (thread, NULL); + + return 0; +} diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp new file mode 100644 index 000000000000..6574b9abf5b7 --- /dev/null +++ b/gdb/testsuite/gdb.threads/pending-fork-event.exp @@ -0,0 +1,86 @@ +# Copyright (C) 2021 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 . + +# Test that we handle well, in all-stop, a fork happening in a thread B while +# doing a step-like command in a thread A. +# +# The scenario we want to test for is: +# +# - the step of thread A completes, we handle the event and stop all threads +# as a result +# - while stopping all threads, thread B reports a fork event which stays +# pending +# +# A buggy GDB / GDBserver combo would notice the thread of the child process +# of the (still unprocessed) fork event, and erroneously create a new inferior +# for it. Once fixed, the child process' thread is hidden by whoever holds the +# pending fork event. + +standard_testfile + +proc do_test { target-non-stop who_forks fork_function } { + + set opts [list debug "additional_flags=-DFORK_FUNCTION=$fork_function"] + + # WHO_FORKS says which of the main or other thread calls (v)fork. The + # thread that does not call (v)fork is the one who tries to step. + if { $who_forks == "main" } { + lappend opts "additional_flags=-DMAIN_THREAD_FORKS" + set this_binfile ${::binfile}-main-${fork_function} + } elseif { $who_forks == "other" } { + lappend opts "additional_flags=-DOTHER_THREAD_FORKS" + set this_binfile ${::binfile}-other-${fork_function} + } else { + error "invalid who_forks value: $who_forks" + } + + if { [gdb_compile_pthreads "$::srcdir/$::subdir/$::srcfile" $this_binfile executable $opts] != "" } { + return + } + + save_vars { ::GDBFLAGS } { + append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\"" + clean_restart $this_binfile + } + + if {![runto_main]} { + fail "could not run to main" + return + } + + # Run until breakpoint in the second thread. + gdb_test "break break_here" "Breakpoint $::decimal.*" + gdb_continue_to_breakpoint "thread started" + + # Delete the breakpoint so the thread doesn't do a step-over. + delete_breakpoints + + # Let the forking thread make progress during the step. + gdb_test "p release_forking_thread = 1" " = 1" + + # stepi the non-forking thread. + gdb_test "stepi" + + # Make sure there's still a single inferior. + gdb_test "info inferior" {\* 1 [^\r\n]+} +} + +foreach_with_prefix target-non-stop { auto on off } { + foreach_with_prefix who_forks { main other } { + foreach_with_prefix fork_function { fork vfork } { + do_test ${target-non-stop} $who_forks $fork_function + } + } +} diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h index 7c293b1f89d2..9f9f69eb1ddd 100644 --- a/gdbserver/gdbthread.h +++ b/gdbserver/gdbthread.h @@ -80,6 +80,15 @@ struct thread_info /* Branch trace target information for this thread. */ struct btrace_target_info *btrace = nullptr; + + /* A pointer to this thread's fork child or fork parent. Valid only while + the parent fork or vfork event is not reported to GDB. + + Used to avoid wildcard vCont actions resuming a (v)fork child before GDB is + notified about the parent's (v)fork event. Also used to avoid including the + (v)fork child in thread list packet responses (e.g. qfThreadInfo). */ + thread_info *fork_child = nullptr; + thread_info *fork_parent = nullptr; }; extern std::list all_threads; diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc index d214aff7051e..7e87bdd8643c 100644 --- a/gdbserver/linux-low.cc +++ b/gdbserver/linux-low.cc @@ -564,10 +564,10 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp, event_lwp->status_pending_p = 1; event_lwp->status_pending = wstat; - /* Link the threads until the parent event is passed on to - higher layers. */ - event_lwp->fork_relative = child_lwp; - child_lwp->fork_relative = event_lwp; + /* Link the threads until the parent's event is passed on to + GDB. */ + event_lwp->thread->fork_child = child_lwp->thread; + child_lwp->thread->fork_parent = event_lwp->thread; /* If the parent thread is doing step-over with single-step breakpoints, the list of single-step breakpoints are cloned @@ -3590,8 +3590,10 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus, if (event_child->waitstatus.kind () == TARGET_WAITKIND_FORKED || event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED) { - event_child->fork_relative->fork_relative = NULL; - event_child->fork_relative = NULL; + gdb_assert (event_child->thread->fork_child != nullptr); + gdb_assert (event_child->thread->fork_child->fork_parent != nullptr); + event_child->thread->fork_child->fork_parent = nullptr; + event_child->thread->fork_child = nullptr; } *ourstatus = event_child->waitstatus; @@ -4371,19 +4373,19 @@ linux_set_resume_request (thread_info *thread, thread_resume *resume, size_t n) /* Don't let wildcard resumes resume fork children that GDB does not yet know are new fork children. */ - if (lwp->fork_relative != NULL) + if (lwp->thread->fork_parent != nullptr) { - struct lwp_info *rel = lwp->fork_relative; + lwp_info *parent = get_thread_lwp (lwp->thread->fork_parent); + target_waitkind kind = parent->waitstatus.kind (); - if (rel->status_pending_p - && (rel->waitstatus.kind () == TARGET_WAITKIND_FORKED - || rel->waitstatus.kind () == TARGET_WAITKIND_VFORKED)) - { - if (debug_threads) - debug_printf ("not resuming LWP %ld: has queued stop reply\n", - lwpid_of (thread)); - continue; - } + gdb_assert (parent->status_pending_p); + gdb_assert (kind == TARGET_WAITKIND_FORKED + || kind == TARGET_WAITKIND_VFORKED); + + if (debug_threads) + debug_printf ("not resuming LWP %ld: is a fork child not know to GDB\n", + lwpid_of (thread)); + continue; } /* If the thread has a pending event that has already been diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h index 05067ffc6e6f..46aff0a29524 100644 --- a/gdbserver/linux-low.h +++ b/gdbserver/linux-low.h @@ -756,12 +756,6 @@ struct lwp_info information or exit status until it can be reported to GDB. */ struct target_waitstatus waitstatus; - /* A pointer to the fork child/parent relative. Valid only while - the parent fork event is not reported to higher layers. Used to - avoid wildcard vCont actions resuming a fork child before GDB is - notified about the parent's fork event. */ - struct lwp_info *fork_relative = nullptr; - /* When stopped is set, this is where the lwp last stopped, with decr_pc_after_break already accounted for. If the LWP is running, this is the address at which the lwp was resumed. */ diff --git a/gdbserver/server.cc b/gdbserver/server.cc index b1d4c92b3d97..7963f2faf267 100644 --- a/gdbserver/server.cc +++ b/gdbserver/server.cc @@ -1656,6 +1656,12 @@ handle_qxfer_threads_worker (thread_info *thread, struct buffer *buffer) gdb_byte *handle; bool handle_status = target_thread_handle (ptid, &handle, &handle_len); + /* If this is a fork or vfork child (has a fork parent), GDB does not yet + know about this process, and must not know about it until it gets the + corresponding (v)fork event. Exclude this thread from the list. */ + if (thread->fork_parent != nullptr) + return; + write_ptid (ptid_s, ptid); buffer_xml_printf (buffer, "