Re: [PATCH 2/2] gdb: handle main thread exiting during detach

Andrew Burgess via Gdb-patches
2023-07-18T14:40:45Z

Andrew Burgess <aburgess@redhat.com> writes:

> Overview
> ========
>
> In non-stop mode, the main thread is running while a second thread is
> stopped. The user has the second thread selected as the current
> thread and asks GDB to detach. At the exact moment of detach the main
> thread exits. This situation causes crashes, assertion failures, and
> unexpected errors to be reported from GDB for both native and remote
> targets.
>
> This commit addresses this situation for native and remote targets,
> but the fixes are all different.
>
> Native Linux Target
> ===================
>
> This is caught in linux_nat_target::detach and the stop_wait_callback
> notices that the thread has exited and cleans up.
>
> GDB then detaches from everything except the main thread by calling
> detach_callback.
>
> After this the first problem is this assert:
>
> /* Only the initial process should be left right now. */
> gdb_assert (num_lwps (pid) == 1);
>
> The num_lwps call will return 0 as the main thread has exited and all
> of the other threads have been detached. I fix this by changing the
> assert too:
>
> gdb_assert (num_lwps (pid) <= 1);
>
> and improving the comment.
>
> The next problem is that we do:
>
> main_lwp = find_lwp_pid (ptid_t (pid));
>
> and then proceed assuming that main_lwp is not nullptr, however, as
> the main thread has exited (num_lwps(pid) == 0), main_lwp is nullptr,
> and so we crash trying to dereference the nullptr. I fix this by
> adding a nullptr check for main_lwp. I think this is fine because the
> actions we are skipping all relate to detaching from the main thread,
> but we'll not be doing this as the thread has already exited!
>
> For Remote Targets
> ==================
>
> On remote targets there are two problems. The first is that when the
> exit occurs during the early phase of the detach, we see the stop
> notification arrive while GDB is removing the breakpoints ahead of the
> detach. The 'set debug remote on' trace looks like this:
>
> [remote] Sending packet: $z0,7f1648fe0241,1#35
> [remote] Notification received: Stop:W0;process:2a0ac8
> # At this point an unpatched gdbserver segfaults, and the connection
> # is broken. A patched gdbserver continues as below...
> [remote] Packet received: E01
> [remote] Sending packet: $z0,7f1648ff00a8,1#68
> [remote] Packet received: E01
> [remote] Sending packet: $z0,7f1648ff132f,1#6b
> [remote] Packet received: E01
> [remote] Sending packet: $D;2a0ac8#3e
> [remote] Packet received: E01
>
> I was originally running into Segmentation Faults, from within
> gdbserver/mem-break.cc, in the function find_gdb_breakpoint. This
> function calls current_process() and then dereferences the result to
> find the breakpoint list.
>
> However, in our case, the current process has already exited, and so
> the current_process() call returns nullptr.
>
> It seems reasonable that in this case gdbserver should report a
> failure to remove the breakpoint, and this is what I've done -- by
> adding a nullptr check within find_gdb_breakpoint.
>
> The second problem I ran into was on the gdb side, as the process has
> already exited, but GDB has not yet acknowledged the exit event, the
> detach -- the 'D' packet in the above trace -- fails. This was being
> reported to the user with a 'Can't detach process' error. As the test
> actually calls detach from Python code, this error was then becoming a
> Python exception.
>
> Though clearly the detach has returned an error, and so, maybe, having
> GDB throw an error would be fine, I think in this case, there's a good
> argument that the remote error should be ignored -- if GDB tries to
> detach and gets back an error, and if there's a pending exit event for
> the pid we tried to detach, then just ignore the error and pretend the
> detach worked fine.
>
> Testing
> =======
>
> In order to test this issue I needed to ensure that the exit event
> arrives at the same time as the detach call. The window of
> opportunity for getting the exit to arrive is so small I've never
> managed to trigger this in real use.
>
> However, if we trigger both the exit and the detach from a single
> Python function then we never return to GDB's event loop, as such GDB
> never processes the exit event, and so the first time GDB gets a
> chance to see the exit is during the detach call. And so that is the
> approach I've taken for testing this patch.
> ---
> gdb/linux-nat.c | 19 ++-
> gdb/remote.c | 19 ++-
> .../main-thread-exit-during-detach.c | 59 ++++++++
> .../main-thread-exit-during-detach.exp | 143 ++++++++++++++++++
> gdbserver/mem-break.cc | 7 +
> 5 files changed, 239 insertions(+), 8 deletions(-)
> create mode 100644 gdb/testsuite/gdb.threads/main-thread-exit-during-detach.c
> create mode 100644 gdb/testsuite/gdb.threads/main-thread-exit-during-detach.exp
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 73008a313c0..c3d62e0f85e 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1426,10 +1426,11 @@ linux_nat_target::detach (inferior *inf, int from_tty)
>
> iterate_over_lwps (ptid_t (pid), detach_callback);
>
> - /* Only the initial process should be left right now. */
> - gdb_assert (num_lwps (pid) == 1);
> -
> - main_lwp = find_lwp_pid (ptid_t (pid));
> + /* We have detached from everything except the main thread now, so
> + should only have one thread left. However, in non-stop mode the
> + main thread might have exited, in which case we'll have no threads
> + left. */
> + gdb_assert (num_lwps (pid) <= 1);
>
> if (forks_exist_p ())
> {
> @@ -1443,10 +1444,14 @@ linux_nat_target::detach (inferior *inf, int from_tty)
> {
> target_announce_detach (from_tty);
>
> - /* Pass on any pending signal for the last LWP. */
> - int signo = get_detach_signal (main_lwp);
> + main_lwp = find_lwp_pid (ptid_t (pid));
> + if (main_lwp != nullptr)
> + {
> + /* Pass on any pending signal for the last LWP. */
> + int signo = get_detach_signal (main_lwp);
>
> - detach_one_lwp (main_lwp, &signo);
> + detach_one_lwp (main_lwp, &signo);
> + }
>
> detach_success (inf);
> }
> diff --git a/gdb/remote.c b/gdb/remote.c
> index ff3d7e5cd32..f61035a26b0 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6126,7 +6126,24 @@ remote_target::remote_detach_pid (int pid)
> else if (rs->buf[0] == '\0')
> error (_("Remote doesn't know how to detach"));
> else
> - error (_("Can't detach process."));
> + {
> + /* It is possible that we have an unprocessed exit event for this
> + pid. If this is the case then we can ignore the failure to detach
> + and just pretend that the detach worked, as far as the user is
> + concerned, the process exited immediately after the detach. */
> + bool process_has_already_exited = false;
> + struct remote_notif_state *rns = rs->notif_state;
> + auto reply
> + = (struct stop_reply *) rns->pending_event[notif_client_stop.id];
> + if (reply != nullptr && reply->ptid.pid () == pid)
> + {
> + if (reply->ws.kind () == TARGET_WAITKIND_EXITED)
> + process_has_already_exited = true;
> + }
> +
> + if (!process_has_already_exited)
> + error (_("Can't detach process: %s"), (char *) rs->buf.data ());
> + }
> }
>
> /* This detaches a program to which we previously attached, using
> diff --git a/gdb/testsuite/gdb.threads/main-thread-exit-during-detach.c b/gdb/testsuite/gdb.threads/main-thread-exit-during-detach.c
> new file mode 100644
> index 00000000000..ecd0138c354
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/main-thread-exit-during-detach.c
> @@ -0,0 +1,59 @@
> +/* 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 <http://www.gnu.org/licenses/>. */
> +
> +#include <pthread.h>
> +#include <unistd.h>
> +
> +/* This is set from GDB to allow the main thread to exit. */
> +
> +volatile int dont_exit_just_yet = 1;
> +
> +/* Somewhere to place a breakpoint. */
> +
> +void
> +breakpt ()
> +{
> + /* Just spin. When the test is started under GDB we never enter the spin
> + loop, but when we attach, the worker thread will be spinning here. */
> + while (dont_exit_just_yet)
> + sleep (1);
> +}
> +
> +/* Thread function, doesn't do anything but hit a breakpoint. */
> +
> +void *
> +thread_worker (void *arg)
> +{
> + breakpt ();
> + return NULL;
> +}
> +
> +int
> +main ()
> +{
> + pthread_t thr;
> +

This test program is missing an 'alarm (300);' call, which I've now
added just here. Apologies for missing this before posting.

Thanks,
Andrew

> + /* Create a thread. */
> + pthread_create (&thr, NULL, thread_worker, NULL);
> + pthread_detach (thr);
> +
> + /* Spin until GDB releases us. */
> + while (dont_exit_just_yet)
> + sleep (1);
> +
> + _exit (0);
> +}
> diff --git a/gdb/testsuite/gdb.threads/main-thread-exit-during-detach.exp b/gdb/testsuite/gdb.threads/main-thread-exit-during-detach.exp
> new file mode 100644
> index 00000000000..97df3bd2783
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/main-thread-exit-during-detach.exp
> @@ -0,0 +1,143 @@
> +# 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 <http://www.gnu.org/licenses/>.
> +
> +# Check for a race condition where in non-stop mode, the user might
> +# have a thread other than the main (original) thread selected and use
> +# the 'detach' command.
> +#
> +# As GDB tries to detach it is possible that the main thread might
> +# exit, the main thread is still running due to non-stop mode.
> +#
> +# GDB used to assume that the main thread would always exist when
> +# processing the detach, clearly this isn't the case, and this
> +# assumption would lead to assertion failures and segfaults.
> +#
> +# Triggering the precise timing is pretty hard, we need the main
> +# thread to exit after the user has entered the 'detach' command, but
> +# before GDB enters the detach implementation and stops all threads,
> +# the window of opportunity for this bug is actually tiny.
> +#
> +# However, we can trigger this bug 100% from Python, as GDB's
> +# event-loop only kicks in once we return from a Python function.
> +# Thus, if we have a single Python function that causes the main
> +# thread to exit, and then calls detach GDB will not have a chance to
> +# handle the main thread exiting before entering the detach code.
> +
> +standard_testfile
> +
> +require allow_python_tests
> +
> +if [build_executable "failed to prepare" $testfile $srcfile {debug pthreads}] {
> + return -1
> +}
> +
> +# Run the test. When SPAWN_INFERIOR is true the inferior is started
> +# as a separate process which GDB then attaches too. When
> +# SPAWN_INFERIOR is false the inferior is started directly within GDB.
> +
> +proc run_test { spawn_inferior } {
> + global srcfile
> + global gdb_prompt
> + global GDBFLAGS
> +
> + save_vars { GDBFLAGS } {
> + append GDBFLAGS " -ex \"set non-stop on\""
> + clean_restart $::binfile
> + }
> +
> + # Setup the inferior. When complete the main thread (#1) will
> + # still be running (due to non-stop mode), while the worker thread
> + # (#2) will be stopped.
> + #
> + # There are two setup modes, when SPAWN_INFERIOR is true we span a
> + # separate process and attach to it, after the attach both threads
> + # are stopped, so it is necessary to resume thread #1.
> + #
> + # When SPAWN_INFERIOR is false we just start the inferior within
> + # GDB, in this case we place a breakpoint that will be hit by
> + # thread #2. When the breakpoint is hit thread #1 will remain
> + # running.
> + if {$spawn_inferior} {
> + set test_spawn_id [spawn_wait_for_attach $::binfile]
> + set testpid [spawn_id_get_pid $test_spawn_id]
> +
> + set escapedbinfile [string_to_regexp $::binfile]
> + gdb_test -no-prompt-anchor "attach $testpid" \
> + "Attaching to program.*`?$escapedbinfile'?, process $testpid.*" \
> + "attach to the inferior"
> +
> + # Attaching to a multi-threaded application in non-stop mode
> + # can result in thread stops being reported after the prompt
> + # is displayed.
> + #
> + # Send a simple command now just to resync the command prompt.
> + gdb_test "p 1 + 2" " = 3"
> +
> + # Set thread 1 (the current thread) running again.
> + gdb_test "continue&"
> + } else {
> + if {![runto_main]} {
> + return -1
> + }
> +
> + gdb_breakpoint "breakpt"
> + gdb_continue_to_breakpoint "run to breakpoint"
> + }
> +
> + # Switch to thread 2.
> + gdb_test "thread 2" \
> + [multi_line \
> + "Switching to thread 2\[^\r\n\]*" \
> + "#0\\s+.*"]
> +
> + # Create a Python function that sets a variable in the inferior and
> + # then detaches. Setting the variable in the inferior will allow the
> + # main thread to exit, we even sleep for a short while in order to
> + # give the inferior a chance to exit.
> + #
> + # However, we don't want GDB to notice the exit before we call detach,
> + # which is why we perform both these actions from a Python function.
> + gdb_test_multiline "Create worker function" \
> + "python" "" \
> + "import time" "" \
> + "def set_and_detach():" "" \
> + " gdb.execute(\"set variable dont_exit_just_yet=0\")" "" \
> + " time.sleep(1)" "" \
> + " gdb.execute(\"detach\")" "" \
> + "end" ""
> +
> + # The Python function performs two actions, the first causes the
> + # main thread to exit, while the second detaches from the inferior.
> + #
> + # In both cases the stop arrives while GDB is processing the
> + # detach, however, for remote targets GDB doesn't report the stop,
> + # while for local targets GDB does report the stop.
> + if {![gdb_is_target_remote]} {
> + set stop_re "\\\[Thread.*exited\\\]\r\n"
> + } else {
> + set stop_re ""
> + }
> + gdb_test "python set_and_detach()" \
> + "${stop_re}\\\[Inferior.*detached\\\]"
> +}
> +
> +foreach_with_prefix spawn_inferior { true false } {
> + if {$spawn_inferior && ![can_spawn_for_attach]} {
> + # If spawning (and attaching too) a separate inferior is not
> + # supported for the current board, then lets not try too.
> + continue
> + }
> +
> + run_test $spawn_inferior
> +}
> diff --git a/gdbserver/mem-break.cc b/gdbserver/mem-break.cc
> index 897b9a2273c..12143caf104 100644
> --- a/gdbserver/mem-break.cc
> +++ b/gdbserver/mem-break.cc
> @@ -976,6 +976,13 @@ static struct gdb_breakpoint *
> find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
> {
> struct process_info *proc = current_process ();
> +
> + /* GDB might not yet be aware that the current process has exited and so
> + still requests that we modify (delete) a breakpoint. Just claim we
> + can't find the breakpoint. */
> + if (proc == nullptr)
> + return nullptr;
> +
> struct breakpoint *bp;
> enum bkpt_type type = Z_packet_to_bkpt_type (z_type);
>
> --
> 2.25.4