[PATCH 3/6] Fix race exposed by gdb.threads/killed.exp

Pedro Alves
2015-03-06T19:58:16Z

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 ptrace call that fails was a PTRACE_CONTINUE.

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.

So instead of ignoring ESRCH locally at each ptrace call, let ptrace
errors throw a new THREAD_NOT_FOUND_ERROR, and wrap the resume request
in a TRY_CATCH that swallows it.

For now, nothing in the core ever sees or handles this error, but I
envision that we'll find uses for it there too.

gdb/gdbserver/ChangeLog:
2015-03-06 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 throw_ptrace_error.
(linux_resume_one_lwp): Reimplement as wrapper around
linux_resume_one_lwp_throw that swallows THREAD_NOT_FOUND_ERROR.

gdb/ChangeLog:
2015-03-06 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 throw_ptrace_error.
(linux_resume_one_lwp): Reimplement as wrapper around
linux_resume_one_lwp_throw that swallows THREAD_NOT_FOUND_ERROR.
---
gdb/common/common-exceptions.h | 3 +++
gdb/gdbserver/linux-low.c | 54 +++++++++++++++++++++++++++++-------------
gdb/linux-nat.c | 32 ++++++++++++++++++++++++-
gdb/nat/ptrace-utils.c | 22 ++++++++++++++++-
4 files changed, 93 insertions(+), 18 deletions(-)

diff --git a/gdb/common/common-exceptions.h b/gdb/common/common-exceptions.h
index a32e6f9..1a13f18 100644
--- a/gdb/common/common-exceptions.h
+++ b/gdb/common/common-exceptions.h
@@ -93,6 +93,9 @@ enum errors {
aborted as the inferior state is no longer valid. */
TARGET_CLOSE_ERROR,

+ /* The thread is gone. */
+ THREAD_NOT_FOUND_ERROR,
+
/* An undefined command was executed. */
UNDEFINED_COMMAND_ERROR,

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 48d905b..f9c4079 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -28,6 +28,7 @@
#include "nat/linux-ptrace.h"
#include "nat/linux-procfs.h"
#include "nat/linux-personality.h"
+#include "nat/ptrace-utils.h"
#include <signal.h>
#include <sys/ioctl.h>
#include <fcntl.h>
@@ -3378,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. */
+/* Like linux_resume_one_lwp but throws THREAD_NOT_FOUND_ERROR if the
+ LWP has disappeared (ptrace returns ESRCH). */

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;
@@ -3576,18 +3576,40 @@ linux_resume_one_lwp (struct lwp_info *lwp,

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;
+ throw_ptrace_error ("resuming thread");
+}

- perror_with_name ("ptrace");
+/* Resume execution of LWP. If STEP is nonzero, single-step it. If
+ SIGNAL is nonzero, give it that signal. No error is throw if the
+ LWP disappears while we try to resume it, no error is thrown. */
+
+static void
+linux_resume_one_lwp (struct lwp_info *lwp,
+ int step, int signal, siginfo_t *info)
+{
+ struct gdb_exception ex;
+
+ TRY_CATCH (ex, RETURN_MASK_ERROR)
+ {
+ linux_resume_one_lwp_throw (lwp, step, signal, info);
+ }
+ if (ex.reason < 0)
+ {
+ if (ex.error == THREAD_NOT_FOUND_ERROR)
+ {
+ /* We got an ESRCH while resuming the LWP. ESRCH from
+ ptrace either means that the thread was already running
+ (an error/bug) or that it is gone (a race condition, e.g,
+ another thread exited the whole process.), or it was
+ killed by SIGKILL. If it's gone, we will get a
+ notification the next time we wait, so we can ignore the
+ error. We could differentiate these, but it's tricky
+ without waiting; the thread still exists as a zombie, so
+ sending it signal 0 would succeed. So just ignore
+ ESRCH. */
+ }
+ else
+ throw_exception (ex);
}
}

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 20fe533..6bb62fd 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1504,7 +1504,7 @@ 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;

@@ -1528,6 +1528,36 @@ linux_resume_one_lwp (struct lwp_info *lp, int step, enum gdb_signal signo)
registers_changed_ptid (lp->ptid);
}

+static void
+linux_resume_one_lwp (struct lwp_info *lp, int step, enum gdb_signal signo)
+{
+ struct gdb_exception ex;
+
+ TRY_CATCH (ex, RETURN_MASK_ERROR)
+ {
+ linux_resume_one_lwp_throw (lp, step, signo);
+ }
+ if (ex.reason < 0)
+ {
+ if (ex.error == THREAD_NOT_FOUND_ERROR)
+ {
+ /* We got an ESRCH while resuming the LWP. ESRCH from
+ ptrace either means that the thread was already running
+ (an error/bug) or that it is gone (a race condition, e.g,
+ another thread exited the whole process.), or it was
+ killed by SIGKILL. If it's gone, we will get a
+ notification the next time we wait, so we can ignore the
+ error. We could differentiate these, but it's tricky
+ without waiting; the thread still exists as a zombie, so
+ sending it signal 0 would succeed. So just ignore
+ ESRCH. */
+ }
+ else
+ throw_exception (ex);
+ }
+}
+
+
/* Resume LP. */

static void
diff --git a/gdb/nat/ptrace-utils.c b/gdb/nat/ptrace-utils.c
index a3dc9b5..ca59454 100644
--- a/gdb/nat/ptrace-utils.c
+++ b/gdb/nat/ptrace-utils.c
@@ -25,5 +25,25 @@
void
throw_ptrace_error (const char *string)
{
- perror_with_name (string);
+ enum errors errcode;
+
+ switch (errno)
+ {
+ case ESRCH:
+ /* This either means that:
+
+ - the thread was already running (a GDB error/bug).
+
+ - the thread is gone, a race condition. E.g, another thread
+ was set running and it exited the whole process, or the
+ process was killed by a SIGKILL.
+
+ Assume the latter. */
+ errcode = THREAD_NOT_FOUND_ERROR;
+ break;
+ default:
+ errcode = GENERIC_ERROR;
+ break;
+ }
+ throw_perror_with_name (errcode, string);
}
--
1.9.3