From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 007BD3858413 for ; Fri, 12 Nov 2021 20:54:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 007BD3858413 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 1ACKsM0K017321 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 12 Nov 2021 15:54:27 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 1ACKsM0K017321 Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 61E391ECEB for ; Fri, 12 Nov 2021 15:54:22 -0500 (EST) Subject: Re: [PATCH 1/2] gdbserver: hide fork child threads from GDB (ping) To: gdb-patches@sourceware.org References: <20211029203332.69894-1-simon.marchi@polymtl.ca> From: Simon Marchi Message-ID: <35710f09-2033-9ebf-d545-1f210166c235@polymtl.ca> Date: Fri, 12 Nov 2021 15:54:22 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20211029203332.69894-1-simon.marchi@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 12 Nov 2021 20:54:22 +0000 X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, 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: Fri, 12 Nov 2021 20:54:30 -0000 Ping. On 2021-10-29 4:33 p.m., Simon Marchi wrote: > 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 34ede238d19f..ff001475a9f0 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 > @@ -3594,8 +3594,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; > @@ -4375,19 +4377,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 c58f7e01d8da..9890d6cc9798 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, " -- > 2.33.1 >