Re: [PATCH 11/16 v3] Extended-remote Linux exit events

Pedro Alves
2014-11-13T19:18:52Z

On 10/31/2014 11:28 PM, Don Breazeal wrote:

>
> Now for the long answer: in my testing I ran into a race condition in
> check_zombie_leaders, which detects when a thread group leader has exited
> and other threads still exist. On the Linux kernel, ptrace/waitpid don't
> allow reaping the leader thread until all other threads in the group are
> reaped. When the leader exits, it goes zombie, but waitpid will not return
> an exit status until the other threads are gone. When a non-leader thread
> calls exec, all other non-leader threads are destroyed, the leader becomes
> a zombie, and once the "other" threads have been reaped, the execing thread
> takes over the leader's pid (tgid) and appears to vanish. In order to
> handle this situation in the current implementation, check_zombie_leaders
> polls the process state in /proc and deletes thread group leaders that are
> in a zombie state. The replacement is added to the lwp list when the exec
> event is reported.
>
> See https://sourceware.org/ml/gdb-patches/2011-10/msg00704.html for a more
> detailed explanation of how this works.
>
> Here is the relevant part of check_zombie_leaders:
>
> if (leader_lp != NULL
> /* Check if there are other threads in the group, as we may
> have raced with the inferior simply exiting. */
> && !last_thread_of_process_p (leader_pid)
> && linux_proc_pid_is_zombie (leader_pid))
> {
> /* ...large informative comment block... */
> delete_lwp (leader_lp);
>
> The race occurred when there were two threads in the program, and the
> non-leader thread called exec. In this case the leader thread passed
> through a very brief zombie state before being replaced by the exec'ing
> thread as the thread group leader. This state transition was asynchronous,
> with no dependency on anything gdbserver did. Because there were no other
> threads, there were no thread exit events, and thus there was no
> synchronization with the leader passing through the zombie state and the
> exec completing. If there had been more threads, the leader would remain
> in the zombie state until they were waited for. In the two-thread case,
> sometimes the leader exit was detected and sometimes it wasn't. (Recall
> that check_zombie_leaders is polling the state, via
> linux_proc_pid_is_zombie. The race is between the leader thread passing
> through the zombie state and check_zombie_leaders testing for zombie
> state.) If leader exit wasn't detected, gdbserver would end up with a
> dangling lwp entry that didn't correspond to any real lwp, and would hang
> waiting for that lwp to stop. Using PTRACE_EVENT_EXIT guarantees that the
> leader exit will be detected.
>
> Note that check_zombie_leaders works just fine for the scenarios where the
> leader thread exits and the other threads continue to run, with no exec
> calls. It is required for systems that don't support the extended ptrace
> events.
>
> The sequence of events resulting in the race condition was this:
>
> 1) In the program, a CLONE event for a new thread occurs.
>
> 2) In the program, both threads are resumed once gdbserver has
> completed the new thread processing.
>
> 3) In gdbserver, the function linux_wait_for_event_filtered loops until
> waitpid returns "no more events" for the SIGCHLD generated by the
> CLONE event. Then linux_wait_for_event_filtered calls
> check_zombie_leaders.
>
> 4) In the program, the new thread is doing the exec. During the exec
> the leader thread will pass through a transitory zombie state. If
> there were more than two threads, the leader thread would remain a
> zombie until all the non-leader, non-exec'ing threads were reaped by
> gdbserver. Since there are no such threads to reap, the leader just
> becomes a zombie and is replaced by the exec'ing thread on-the-fly.
> (Note that it appears that the leader thread is a zombie just for a
> very brief instant.)
>
> 5) In gdbserver, check_zombie_leaders checks whether an lwp entry
> corresponds to a zombie leader thread, and if so, deletes it. Here
> is the race: in (4) above, the leader may or may not be in the
> transitory zombie state. In the case where a zombie isn't detected,
> delete_lwp is not called.
>
> 6) In gdbserver, an EXEC event is detected and processed. When it gets
> ready to report the event to GDB, it calls stop_all_lwps, which sends
> a SIGSTOP to each lwp in the list and the waits until all the lwps in
> the list have reported a stop event. If the zombie leader wasn't
> detected and processed in step (5), gdbserver blocks forever in
> linux_wait_for_event_filtered, waiting for the undeleted lwp to be
> stopped, which will never happen.

This is easy to see with thread-execl.exp, if we revert the PTRACE_EVENT_EXIT
parts:

linux_low_filter_event: pc is 0x3b37209237
pc is 0x3b37209237
stop pc is 0x3b37209237
HEW: Got exec event from LWP 2721
Hit a non-gdbserver trap event.
>>>> entering stop_all_lwps
stop_all_lwps (stop, except=none)
Sending sigstop to lwp 23192
wait_for_sigstop: pulling events
my_waitpid (-1, 0x40000001)
my_waitpid (-1, 0x80000001): status(0), -1
LWFE: waitpid(-1, ...) returned -1, No child processes
leader_pid=2721, leader_lp!=NULL=1, num_lwps=2, zombie=0
sigsuspend'ing
*** hang ***

But I'm having trouble convincing myself that we do need
the PTRACE_EVENT_EXIT handling.

This seems to work for me just as well. The fix is in
making check_zombie_leaders ignore stopped threads.

@@ -1629,7 +1597,7 @@ check_zombie_leaders (void)
leader_pid, leader_lp!= NULL, num_lwps (leader_pid),
linux_proc_pid_is_zombie (leader_pid));

- if (leader_lp != NULL
+ if (leader_lp != NULL && !leader_lp->stopped

But I'm in a hurry to leave now. Maybe I'm missing something else.

From ab07ee8afc3c2358cd6b23474655fde087e2b36e Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 13 Nov 2014 17:53:43 +0000
Subject: [PATCH] revert PTRACE_EVENT_EXIT bits

---
gdb/gdbserver/linux-low.c | 56 +++--------------------------------------------
1 file changed, 3 insertions(+), 53 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index f308333..316b302 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -538,38 +538,6 @@ handle_extended_wait (struct lwp_info *event_child, int *wstatp)
event_child->waitstatus.kind = TARGET_WAITKIND_VFORK_DONE;
return 0;
}
- else if (event == PTRACE_EVENT_EXIT)
- {
- unsigned long exit_status;
- unsigned long lwpid = lwpid_of (event_thr);
- int ret;
-
- if (debug_threads)
- debug_printf ("HEW: Got exit event from LWP %ld\n", lwpid );
-
- ptrace (PTRACE_GETEVENTMSG, lwpid, (PTRACE_TYPE_ARG3) 0, &exit_status);
-
- if (num_lwps (pid_of (event_thr)) > 1)
- {
- /* If there is at least one more LWP, then the program still
- exists and the exit should not be reported to GDB. */
- delete_lwp (event_child);
- ret = 1;
- }
- else
- {
- /* Set the exit status to the actual exit status, so normal
- WIFEXITED/WIFSIGNALLED processing and reporting for the
- last lwp in the process can proceed from here. */
- *wstatp = exit_status;
- ret = 0;
- }
-
- /* Resume the thread so that it actually exits. Subsequent exit
- events for LWPs that were deleted above will be ignored. */
- ptrace (PTRACE_CONT, lwpid, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0);
- return ret;
- }
else if (event == PTRACE_EVENT_EXEC)
{
if (debug_threads)
@@ -1629,7 +1597,7 @@ check_zombie_leaders (void)
leader_pid, leader_lp!= NULL, num_lwps (leader_pid),
linux_proc_pid_is_zombie (leader_pid));

- if (leader_lp != NULL
+ if (leader_lp != NULL && !leader_lp->stopped
/* Check if there are other threads in the group, as we may
have raced with the inferior simply exiting. */
&& !last_thread_of_process_p (leader_pid)
@@ -2107,17 +2075,6 @@ linux_low_filter_event (ptid_t filter_ptid, int lwpid, int *wstatp)
child = add_lwp (child_ptid);
child->stopped = 1;
current_thread = child->thread;
-
- if (non_stop && stopping_threads == NOT_STOPPING_THREADS)
- {
- /* Make sure we delete the lwp entry for the exec'ing thread,
- which will have vanished. We do this by sending a signal
- to all the other threads in the lwp list, deleting any
- that are not found. Note that in all-stop mode this will
- happen before reporting the event. */
- stop_all_lwps (0, child);
- unstop_all_lwps (0, child);
- }
}

/* If we didn't find a process, one of two things presumably happened:
@@ -2166,8 +2123,7 @@ linux_low_filter_event (ptid_t filter_ptid, int lwpid, int *wstatp)
architecture being defined already (so that CHILD has a valid
regcache), and on LAST_STATUS being set (to check for SIGTRAP or
not). */
- if (WIFSTOPPED (wstat)
- && (linux_ptrace_get_extended_event (wstat) != PTRACE_EVENT_EXIT))
+ if (WIFSTOPPED (wstat))
{
if (debug_threads
&& the_low_target.get_pc != NULL)
@@ -3486,8 +3442,6 @@ send_sigstop (struct lwp_info *lwp)
calling exec. See comments in linux_low_filter_event regarding
PTRACE_EVENT_EXEC. */
delete_lwp (lwp);
- set_desired_thread (0);
-
if (debug_threads)
debug_printf ("send_sigstop: lwp %d has vanished\n", pid);
}
@@ -5565,10 +5519,7 @@ linux_supports_vfork_events (void)
static int
linux_supports_exec_events (void)
{
- /* Check for PTRACE_O_TRACEEXIT, since our implementation of follow
- exec depends on this option, which was implemented in a later
- kernel version than PTRACE_O_TRACEFORK et al. */
- return linux_supports_traceexit ();
+ return linux_supports_tracefork ();
}

static int
@@ -6588,7 +6539,6 @@ initialize_low (void)
linux_ptrace_set_requested_options (PTRACE_O_TRACEFORK
| PTRACE_O_TRACEVFORK
| PTRACE_O_TRACEVFORKDONE
- | PTRACE_O_TRACEEXIT
| PTRACE_O_TRACEEXEC);
linux_ptrace_check_options ();
}
--
1.9.3