[pushed] Fix race exposed by gdb.threads/killed.exp (Re: [PATCH 2/6] Introduce throw_ptrace_error)

Pedro Alves
2015-03-19T17:33:37Z

On 03/11/2015 03:44 PM, Pedro Alves wrote:

> If from local context at the ptrace call site the error can be handled
> better, sure, but then we wouldn't be calling perror_with_name. But
> the situation is the reverse here. The context is higher up on the
> call chain, not around the ptrace call site. It's because errors can't
> be handled locally that we're _already_ throwing today. The fix was for all
> those ptrace calls deep down in the call chain that can't tell why they're
> being called, and so don't know whether they should ignore the error
> or not. For example, say you're poking a register manually in the command
> line and that errors out with ESRCH. Should we ignore that error? Probably not.
> It should probably propagate to the command line. OTOH, if GDB decides to
> flush registers just while it is resuming a thread, and the thread/process
> becomes zombie. We should probably ignore the ESRCH, cancel whatever other
> setup we were doing for the resume / skip the remainder of the ptrace calls,
> and go straight to collecting the wait status. In both cases, the registers
> write ptrace call is the same. So now what?

Turns out that the killed.exp test was still racy, exactly because
of a code path in the backend like that.

...
LLW: waitpid 4212 received Trace/breakpoint trap (stopped)
LLW: Handling extended status 0x03057f
LHEW: Got clone event from LWP 4212, new child is LWP 4216
[New Thread 0x7ffff7fc0700 (LWP 4216)]
LNW: waitpid(-1, ...) returned 0, ERRNO-OK
RSRL: resuming stopped-resumed LWP Thread 0x7ffff7fc0700 (LWP 4216) at 0x3615ef4ce1: step=0
RSRL: resuming stopped-resumed LWP Thread 0x7ffff7fc1740 (LWP 4212) at 0x3615ef4ce1: step=0
sigchld
ptrace: No such process.
(gdb) linux_nat_wait: [process -1], [TARGET_WNOHANG]
LLW: enter
sigchld
...

This time the error was thrown while GDB was trying to read the
PC of the LWP that is about to be resumed, in resume_stopped_resumed_lwps.
Fixed that with another TRY/CATCH.

And, it turns out that version of the fix that reads the status
from /proc/pid/status was still racy as is too. I was still seeing
"ptrace: No such process" sometimes. That was because there's a
tiny race window where ptrace fails with ESRCH, but /proc/pid/status
shows a state different from zombie. I hacked gdb to print the
state, and I saw:

RSRL: resuming stopped-resumed LWP Thread 0x7ffff7fc0700 (LWP 9947) at 0x3615ef4ce1: step=0
RSRL: resuming stopped-resumed LWP Thread 0x7ffff7fc1740 (LWP 9943) at 0x3615ef4ce1: step=0
infrun: state line of pid=9943 is "State: R (running)" (have_state=1)
sigchld
sigchld
ptrace: No such process (inferior_ptid=[Thread 0x7ffff7fc1740 (LWP 9943)]).

This is the same as mentioned in the ptrace(2) man page:

The tracer cannot assume that the ptrace-stopped tracee exists.
There are many scenarios when the tracee may die while stopped
(such as SIGKILL). Therefore, the tracer must be prepared to handle an
ESRCH error on any ptrace operation. Unfortunately, the same error is
returned if the tracee exists but is not ptrace-stopped (for commands
which require a stopped tracee), or if it is not traced by the process
which issued the ptrace call. The tracer needs to keep track of
the stopped/running state of the tracee, and interpret ESRCH as "tracee died
unexpectedly" only if it knows that the tracee has been observed to
enter ptrace-stop. Note that there is no guarantee that waitpid(WNOHANG)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
will reliably report the tracee's death status if a ptrace operation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
returned ESRCH. waitpid(WNOHANG) may return 0 instead. In other words,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
the tracee may be "not yet fully dead", but already refusing ptrace requests.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We already keep track of stopped/running state. So instead of checking whether
/proc/pid/status shows "Z (Zombie)" state, I'm checking the opposite -- whether
the lwp is NOT in "T (tracing stop)" state (that is, no longer ptrace-stopped).

Here's what I pushed.

---
From 23f238d3456531db33456918f004dcc5ce151363 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 19 Mar 2015 15:12:33 +0000
Subject: [PATCH] Fix race exposed by gdb.threads/killed.exp

On GNU/Linux, this test sometimes FAILs like this:

(gdb) run
Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.threads/killed
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
ptrace: No such process.
(gdb)
Program terminated with signal SIGKILL, Killed.
The program no longer exists.
FAIL: gdb.threads/killed.exp: run program to completion (timeout)

Note the suspicious "No such process" line (that's errno==ESRCH).
Adding debug output we see:

linux_nat_wait: [process -1], [TARGET_WNOHANG]
LLW: enter
LNW: waitpid(-1, ...) returned 18465, ERRNO-OK
LLW: waitpid 18465 received Stopped (signal) (stopped)
LNW: waitpid(-1, ...) returned 18461, ERRNO-OK
LLW: waitpid 18461 received Trace/breakpoint trap (stopped)
LLW: Handling extended status 0x03057f
LHEW: Got clone event from LWP 18461, new child is LWP 18465
LNW: waitpid(-1, ...) returned 0, ERRNO-OK
RSRL: resuming stopped-resumed LWP LWP 18465 at 0x3b36af4b51: step=0
RSRL: resuming stopped-resumed LWP LWP 18461 at 0x3b36af4b51: step=0
sigchld
ptrace: No such process.
(gdb) linux_nat_wait: [process -1], [TARGET_WNOHANG]
LLW: enter
LNW: waitpid(-1, ...) returned 18465, ERRNO-OK
LLW: waitpid 18465 received Killed (terminated)
LLW: LWP 18465 exited.
LNW: waitpid(-1, ...) returned 18461, No child processes
LLW: waitpid 18461 received Killed (terminated)
Process 18461 exited
LNW: waitpid(-1, ...) returned -1, No child processes
LLW: exit
sigchld
infrun: target_wait (-1, status) =
infrun: 18461 [process 18461],
infrun: status->kind = signalled, signal = GDB_SIGNAL_KILL
infrun: TARGET_WAITKIND_SIGNALLED

Program terminated with signal SIGKILL, Killed.
The program no longer exists.
infrun: stop_waiting
FAIL: gdb.threads/killed.exp: run program to completion (timeout)

The issue is that here:

RSRL: resuming stopped-resumed LWP LWP 18465 at 0x3b36af4b51: step=0
RSRL: resuming stopped-resumed LWP LWP 18461 at 0x3b36af4b51: step=0

The first line shows we had just resumed LWP 18465, which does:

void *
child_func (void *dummy)
{
kill (pid, SIGKILL);
exit (1);
}

So if the kernel manages to schedule that thread fast enough, the
process may be killed before GDB has a chance to resume LWP 18461.

GDBserver has code at the tail end of linux_resume_one_lwp to cope
with this:

~~~
ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (thread),
(PTRACE_TYPE_ARG3) 0,
/* Coerce to a uintptr_t first to avoid potential gcc warning
of coercing an 8 byte integer to a 4 byte pointer. */
(PTRACE_TYPE_ARG4) (uintptr_t) signal);

current_thread = saved_thread;
if (errno)
{
/* ESRCH from ptrace either means that the thread was already
running (an error) or that it is gone (a race condition). If
it's gone, we will get a notification the next time we wait,
so we can ignore the error. We could differentiate these
two, but it's tricky without waiting; the thread still exists
as a zombie, so sending it signal 0 would succeed. So just
ignore ESRCH. */
if (errno == ESRCH)
return;

perror_with_name ("ptrace");
}
~~~

However, that's not a complete fix, because between starting to handle
the resume request and getting that PTRACE_CONTINUE, we run other
ptrace calls that can also fail with ESRCH, and that end up throwing
an error (with perror_with_name).

In the case above, I indeed sometimes see resume_stopped_resumed_lwps
fail in the registers read:

resume_stopped_resumed_lwps (struct lwp_info *lp, void *data)
{
...
CORE_ADDR pc = regcache_read_pc (regcache);

Or e.g., in 32-bit mode, i386_linux_resume has several calls that can
throw too.

Whether to ignore ptrace errors or not depends on context that is only
available somewhere up the call chain. So the fix is to let ptrace
errors throw as they do today, and wrap the resume request in a
TRY/CATCH that swallows it iff the lwp that we were trying to resume
is no longer ptrace-stopped.

gdb/gdbserver/ChangeLog:
2015-03-19 Pedro Alves <palves@redhat.com>

* linux-low.c (linux_resume_one_lwp): Rename to ...
(linux_resume_one_lwp_throw): ... this. Don't handle ESRCH here,
instead call perror_with_name.
(check_ptrace_stopped_lwp_gone): New function.
(linux_resume_one_lwp): Reimplement as wrapper around
linux_resume_one_lwp_throw that swallows errors if the LWP is
gone.

gdb/ChangeLog:
2015-03-19 Pedro Alves <palves@redhat.com>

* linux-nat.c (linux_resume_one_lwp): Rename to ...
(linux_resume_one_lwp_throw): ... this. Don't handle ESRCH here,
instead call perror_with_name.
(check_ptrace_stopped_lwp_gone): New function.
(linux_resume_one_lwp): Reimplement as wrapper around
linux_resume_one_lwp_throw that swallows errors if the LWP is
gone.
(resume_stopped_resumed_lwps): Try register reads in TRY/CATCH and
swallows errors if the LWP is gone. Use
linux_resume_one_lwp_throw instead of linux_resume_one_lwp.
---
gdb/ChangeLog | 13 ++++++
gdb/gdbserver/ChangeLog | 10 +++++
gdb/gdbserver/linux-low.c | 80 +++++++++++++++++++++++++++--------
gdb/linux-nat.c | 105 +++++++++++++++++++++++++++++++++++++++-------
gdb/nat/linux-procfs.c | 9 ++++
gdb/nat/linux-procfs.h | 2 +
6 files changed, 186 insertions(+), 33 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7ae3c58..a8b8850 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,18 @@
2015-03-19 Pedro Alves <palves@redhat.com>

+ * linux-nat.c (linux_resume_one_lwp): Rename to ...
+ (linux_resume_one_lwp_throw): ... this. Don't handle ESRCH here,
+ instead call perror_with_name.
+ (check_ptrace_stopped_lwp_gone): New function.
+ (linux_resume_one_lwp): Reimplement as wrapper around
+ linux_resume_one_lwp_throw that swallows errors if the LWP is
+ gone.
+ (resume_stopped_resumed_lwps): Try register reads in TRY/CATCH and
+ swallows errors if the LWP is gone. Use
+ linux_resume_one_lwp_throw instead of linux_resume_one_lwp.
+
+2015-03-19 Pedro Alves <palves@redhat.com>
+
* linux-nat.c (status_callback): Return early if the LWP has no
status pending.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 0383e67..cbd199b 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,15 @@
2015-03-19 Pedro Alves <palves@redhat.com>

+ * linux-low.c (linux_resume_one_lwp): Rename to ...
+ (linux_resume_one_lwp_throw): ... this. Don't handle ESRCH here,
+ instead call perror_with_name.
+ (check_ptrace_stopped_lwp_gone): New function.
+ (linux_resume_one_lwp): Reimplement as wrapper around
+ linux_resume_one_lwp_throw that swallows errors if the LWP is
+ gone.
+
+2015-03-19 Pedro Alves <palves@redhat.com>
+
* linux-low.c (count_events_callback, select_event_lwp_callback):
No longer check whether the thread has resume_stop as last resume
kind.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 2b988ec..0c54115 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3379,13 +3379,12 @@ stop_all_lwps (int suspend, struct lwp_info *except)
}
}

-/* Resume execution of the inferior process.
- If STEP is nonzero, single-step it.
- If SIGNAL is nonzero, give it that signal. */
+/* Resume execution of LWP. If STEP is nonzero, single-step it. If
+ SIGNAL is nonzero, give it that signal. */

static void
-linux_resume_one_lwp (struct lwp_info *lwp,
- int step, int signal, siginfo_t *info)
+linux_resume_one_lwp_throw (struct lwp_info *lwp,
+ int step, int signal, siginfo_t *info)
{
struct thread_info *thread = get_lwp_thread (lwp);
struct thread_info *saved_thread;
@@ -3566,8 +3565,6 @@ linux_resume_one_lwp (struct lwp_info *lwp,

regcache_invalidate_thread (thread);
errno = 0;
- lwp->stopped = 0;
- lwp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
lwp->stepping = step;
ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (thread),
(PTRACE_TYPE_ARG3) 0,
@@ -3577,19 +3574,68 @@ linux_resume_one_lwp (struct lwp_info *lwp,

current_thread = saved_thread;
if (errno)
+ perror_with_name ("resuming thread");
+
+ /* Successfully resumed. Clear state that no longer makes sense,
+ and mark the LWP as running. Must not do this before resuming
+ otherwise if that fails other code will be confused. E.g., we'd
+ later try to stop the LWP and hang forever waiting for a stop
+ status. Note that we must not throw after this is cleared,
+ otherwise handle_zombie_lwp_error would get confused. */
+ lwp->stopped = 0;
+ lwp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
+}
+
+/* Called when we try to resume a stopped LWP and that errors out. If
+ the LWP is no longer in ptrace-stopped state (meaning it's zombie,
+ or about to become), discard the error, clear any pending status
+ the LWP may have, and return true (we'll collect the exit status
+ soon enough). Otherwise, return false. */
+
+static int
+check_ptrace_stopped_lwp_gone (struct lwp_info *lp)
+{
+ struct thread_info *thread = get_lwp_thread (lp);
+
+ /* If we get an error after resuming the LWP successfully, we'd
+ confuse !T state for the LWP being gone. */
+ gdb_assert (lp->stopped);
+
+ /* We can't just check whether the LWP is in 'Z (Zombie)' state,
+ because even if ptrace failed with ESRCH, the tracee may be "not
+ yet fully dead", but already refusing ptrace requests. In that
+ case the tracee has 'R (Running)' state for a little bit
+ (observed in Linux 3.18). See also the note on ESRCH in the
+ ptrace(2) man page. Instead, check whether the LWP has any state
+ other than ptrace-stopped. */
+
+ /* Don't assume anything if /proc/PID/status can't be read. */
+ if (linux_proc_pid_is_trace_stopped_nowarn (lwpid_of (thread)) == 0)
{
- /* ESRCH from ptrace either means that the thread was already
- running (an error) or that it is gone (a race condition). If
- it's gone, we will get a notification the next time we wait,
- so we can ignore the error. We could differentiate these
- two, but it's tricky without waiting; the thread still exists
- as a zombie, so sending it signal 0 would succeed. So just
- ignore ESRCH. */
- if (errno == ESRCH)
- return;
+ lp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
+ lp->status_pending_p = 0;
+ return 1;
+ }
+ return 0;
+}
+
+/* Like linux_resume_one_lwp_throw, but no error is thrown if the LWP
+ disappears while we try to resume it. */

- perror_with_name ("ptrace");
+static void
+linux_resume_one_lwp (struct lwp_info *lwp,
+ int step, int signal, siginfo_t *info)
+{
+ TRY
+ {
+ linux_resume_one_lwp_throw (lwp, step, signal, info);
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ if (!check_ptrace_stopped_lwp_gone (lwp))
+ throw_exception (ex);
}
+ END_CATCH
}

struct thread_resume_array
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 40f1e1f..8b62041 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1503,7 +1503,8 @@ linux_nat_detach (struct target_ops *ops, const char *args, int from_tty)
single-step it. If SIGNAL is nonzero, give it that signal. */

static void
-linux_resume_one_lwp (struct lwp_info *lp, int step, enum gdb_signal signo)
+linux_resume_one_lwp_throw (struct lwp_info *lp, int step,
+ enum gdb_signal signo)
{
lp->step = step;

@@ -1522,11 +1523,68 @@ linux_resume_one_lwp (struct lwp_info *lp, int step, enum gdb_signal signo)
if (linux_nat_prepare_to_resume != NULL)
linux_nat_prepare_to_resume (lp);
linux_ops->to_resume (linux_ops, lp->ptid, step, signo);
- lp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
+
+ /* Successfully resumed. Clear state that no longer makes sense,
+ and mark the LWP as running. Must not do this before resuming
+ otherwise if that fails other code will be confused. E.g., we'd
+ later try to stop the LWP and hang forever waiting for a stop
+ status. Note that we must not throw after this is cleared,
+ otherwise handle_zombie_lwp_error would get confused. */
lp->stopped = 0;
+ lp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
registers_changed_ptid (lp->ptid);
}

+/* Called when we try to resume a stopped LWP and that errors out. If
+ the LWP is no longer in ptrace-stopped state (meaning it's zombie,
+ or about to become), discard the error, clear any pending status
+ the LWP may have, and return true (we'll collect the exit status
+ soon enough). Otherwise, return false. */
+
+static int
+check_ptrace_stopped_lwp_gone (struct lwp_info *lp)
+{
+ /* If we get an error after resuming the LWP successfully, we'd
+ confuse !T state for the LWP being gone. */
+ gdb_assert (lp->stopped);
+
+ /* We can't just check whether the LWP is in 'Z (Zombie)' state,
+ because even if ptrace failed with ESRCH, the tracee may be "not
+ yet fully dead", but already refusing ptrace requests. In that
+ case the tracee has 'R (Running)' state for a little bit
+ (observed in Linux 3.18). See also the note on ESRCH in the
+ ptrace(2) man page. Instead, check whether the LWP has any state
+ other than ptrace-stopped. */
+
+ /* Don't assume anything if /proc/PID/status can't be read. */
+ if (linux_proc_pid_is_trace_stopped_nowarn (ptid_get_lwp (lp->ptid)) == 0)
+ {
+ lp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
+ lp->status = 0;
+ lp->waitstatus.kind = TARGET_WAITKIND_IGNORE;
+ return 1;
+ }
+ return 0;
+}
+
+/* Like linux_resume_one_lwp_throw, but no error is thrown if the LWP
+ disappears while we try to resume it. */
+
+static void
+linux_resume_one_lwp (struct lwp_info *lp, int step, enum gdb_signal signo)
+{
+ TRY
+ {
+ linux_resume_one_lwp_throw (lp, step, signo);
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ if (!check_ptrace_stopped_lwp_gone (lp))
+ throw_exception (ex);
+ }
+ END_CATCH
+}
+
/* Resume LP. */

static void
@@ -3542,24 +3600,39 @@ resume_stopped_resumed_lwps (struct lwp_info *lp, void *data)
{
struct regcache *regcache = get_thread_regcache (lp->ptid);
struct gdbarch *gdbarch = get_regcache_arch (regcache);
- CORE_ADDR pc = regcache_read_pc (regcache);

- /* Don't bother if there's a breakpoint at PC that we'd hit
- immediately, and we're not waiting for this LWP. */
- if (!ptid_match (lp->ptid, *wait_ptid_p))
+ TRY
{
- if (breakpoint_inserted_here_p (get_regcache_aspace (regcache), pc))
- return 0;
- }
+ CORE_ADDR pc = regcache_read_pc (regcache);
+ int leave_stopped = 0;

- if (debug_linux_nat)
- fprintf_unfiltered (gdb_stdlog,
- "RSRL: resuming stopped-resumed LWP %s at %s: step=%d\n",
- target_pid_to_str (lp->ptid),
- paddress (gdbarch, pc),
- lp->step);
+ /* Don't bother if there's a breakpoint at PC that we'd hit
+ immediately, and we're not waiting for this LWP. */
+ if (!ptid_match (lp->ptid, *wait_ptid_p))
+ {
+ if (breakpoint_inserted_here_p (get_regcache_aspace (regcache), pc))
+ leave_stopped = 1;
+ }

- linux_resume_one_lwp (lp, lp->step, GDB_SIGNAL_0);
+ if (!leave_stopped)
+ {
+ if (debug_linux_nat)
+ fprintf_unfiltered (gdb_stdlog,
+ "RSRL: resuming stopped-resumed LWP %s at "
+ "%s: step=%d\n",
+ target_pid_to_str (lp->ptid),
+ paddress (gdbarch, pc),
+ lp->step);
+
+ linux_resume_one_lwp_throw (lp, lp->step, GDB_SIGNAL_0);
+ }
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ if (!check_ptrace_stopped_lwp_gone (lp))
+ throw_exception (ex);
+ }
+ END_CATCH
}

return 0;
diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
index f149383..7599b32 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -151,6 +151,15 @@ linux_proc_pid_is_stopped (pid_t pid)
return linux_proc_pid_has_state (pid, "T (stopped)", 1);
}

+/* Detect `T (tracing stop)' in `/proc/PID/status'.
+ Other states including `T (stopped)' are reported as false. */
+
+int
+linux_proc_pid_is_trace_stopped_nowarn (pid_t pid)
+{
+ return linux_proc_pid_has_state (pid, "T (tracing stop)", 1);
+}
+
/* Return non-zero if PID is a zombie. If WARN, warn on failure to
open the /proc file. */

diff --git a/gdb/nat/linux-procfs.h b/gdb/nat/linux-procfs.h
index 979ae0d..c4f5788 100644
--- a/gdb/nat/linux-procfs.h
+++ b/gdb/nat/linux-procfs.h
@@ -36,6 +36,8 @@ extern pid_t linux_proc_get_tracerpid_nowarn (pid_t lwpid);

extern int linux_proc_pid_is_stopped (pid_t pid);

+extern int linux_proc_pid_is_trace_stopped_nowarn (pid_t pid);
+
/* Return non-zero if PID is a zombie. Failure to open the
/proc/pid/status file results in a warning. */

--
1.9.3