[PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

Sergio Durigan Junior
2017-02-16T20:19:45Z

This patch fixes PR gdb/16188, which is about the fact that
fork_inferior doesn't verify the return value of the "traceme_fun"
callback. On most targets, this callback is actually a wrapper to a
ptrace call that does a PTRACE_TRACEME on the forked GDB process that
will eventually become the inferior.

My approach to this bug was to expand the declaration of "traceme_fun"
and make it (a) return a boolean value indicating whether the function
succeeded or not, and (b) receive a pointer to an int which represents
the errno value of the failure, if it occurred.

Then, obviously I had to update all the providers of this callback and
make them honour these new things. On gdb/inf-ptrace.c, where we have
the generic "inf_ptrace_me", this was just a matter of checking the
return value of ptrace. On gdb/darwin-nat.c, I decided to take a
conservative approach and verify if every system call actually
succeded. On gdb/procfs.c, it seems clear to me that the function
itself takes care of error handling and does the right thing if
something wrong happens (i.e., it prints warnings and calls _exit).
On some other cases (e.g., gdb/gnu-nat.c) the callback was actually
calling error, which is not right since we're in the forked GDB. So I
removed these calls to error and replaced them by the necessary logic
to make the function return false to fork_inferior.

Last, but not least, I expanded a bit the comments on fork_inferior
and on the declaration of the callback.

I confess I have no idea how to make a testcase for this bug, but I've
run the patch through BuildBot and no regressions were found
whatsoever. I could not actively test some targets (gdb/darwin-nat.c,
gdb/procfs.c, gdb/gnu-nat.c), so I'd appreciate if others could look
at the patch.

Thanks,

gdb/ChangeLog:
2017-02-16 Sergio Durigan Junior <sergiodj@redhat.com>

PR gdb/16188
* darwin-nat.c (darwin_ptrace_me): Update prototype. Return a
bool and take an int pointer as parameter. Check if calls to
system calls succeeded. Fill TRACE_ERRNO if needed.
* fork-child.c (fork_inferior): Update declaration; declare
TRACEME_FUN as returning a boolean value and taking an int
pointer. Check if the call to TRACEME_FUN succeeded.
* gnu-nat.c (gnu_ptrace_me): Update declaration. Check if call to
PTRACE succeeded.
* inf-ptrace.c (inf_ptrace_me): Likewise.
* inferior.h (fork_inferior): Update declaration of TRACEME_FUN.
* procfs.c (procfs_set_exec_trap): Update declaration. Return
true.
---
gdb/darwin-nat.c | 39 ++++++++++++++++++++++++++++++---------
gdb/fork-child.c | 25 ++++++++++++++++++++++---
gdb/gnu-nat.c | 11 ++++++++---
gdb/inf-ptrace.c | 14 +++++++++++---
gdb/inferior.h | 2 +-
gdb/procfs.c | 6 ++++--
6 files changed, 76 insertions(+), 21 deletions(-)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 8c5e8a0..a6f5969 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -98,7 +98,7 @@ static void darwin_mourn_inferior (struct target_ops *ops);

static void darwin_kill_inferior (struct target_ops *ops);

-static void darwin_ptrace_me (void);
+static bool darwin_ptrace_me (int *trace_errno);

static void darwin_ptrace_him (int pid);

@@ -1722,29 +1722,50 @@ darwin_init_thread_list (struct inferior *inf)
FIXME: is there a lighter way ? */
static int ptrace_fds[2];

-static void
-darwin_ptrace_me (void)
+static bool
+darwin_ptrace_me (int *trace_errno)
{
int res;
char c;

+ errno = 0;
/* Close write end point. */
- close (ptrace_fds[1]);
+ if (close (ptrace_fds[1]) < 0)
+ goto fail;

/* Wait until gdb is ready. */
res = read (ptrace_fds[0], &c, 1);
if (res != 0)
- error (_("unable to read from pipe, read returned: %d"), res);
- close (ptrace_fds[0]);
+ {
+ int saved_errno = errno;
+
+ warning (_("unable to read from pipe, read returned: %d"), res);
+ errno = saved_errno;
+ goto fail;
+ }
+ if (close (ptrace_fds[0]) < 0)
+ goto fail;

/* Get rid of privileges. */
- setegid (getgid ());
+ if (setegid (getgid ()) < 0)
+ goto fail;

/* Set TRACEME. */
- PTRACE (PT_TRACE_ME, 0, 0, 0);
+ if (PTRACE (PT_TRACE_ME, 0, 0, 0) < 0)
+ goto fail;

/* Redirect signals to exception port. */
- PTRACE (PT_SIGEXC, 0, 0, 0);
+ if (PTRACE (PT_SIGEXC, 0, 0, 0) < 0)
+ goto fail;
+
+ return true;
+
+fail:
+ /* Fill trace_errno as necessary. */
+ if (trace_errno != NULL)
+ *trace_errno = errno;
+
+ return false;
}

/* Dummy function to be sure fork_inferior uses fork(2) and not vfork(2). */
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index eaa8cb5..edeb708 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -114,14 +114,21 @@ escape_bang_in_quoted_argument (const char *shell_file)
the arguments to the program. ENV is the environment vector to
pass. SHELL_FILE is the shell file, or NULL if we should pick
one. EXEC_FUN is the exec(2) function to use, or NULL for the default
- one. */
+ one.
+
+ TRACEME_FUN is the function that will be called to start tracing
+ the inferior; it needs to return true iff the tracing started
+ correctly, or false otherwise. If there was any error, the
+ function shall fill TRACE_ERRRNO appropriately with the errno
+ associated with the failure. */

/* This function is NOT reentrant. Some of the variables have been
made static to ensure that they survive the vfork call. */

int
fork_inferior (char *exec_file_arg, char *allargs, char **env,
- void (*traceme_fun) (void), void (*init_trace_fun) (int),
+ bool (*traceme_fun) (int *trace_errno),
+ void (*init_trace_fun) (int),
void (*pre_trace_fun) (void), char *shell_file_arg,
void (*exec_fun)(const char *file, char * const *argv,
char * const *env))
@@ -317,6 +324,8 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,

if (pid == 0)
{
+ int trace_errno;
+
/* Switch to the main UI, so that gdb_std{in/out/err} in the
child are mapped to std{in/out/err}. This makes it possible
to use fprintf_unfiltered/warning/error/etc. in the child
@@ -355,7 +364,17 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
for the inferior. */

/* "Trace me, Dr. Memory!" */
- (*traceme_fun) ();
+ if (!(*traceme_fun) (&trace_errno))
+ {
+ /* There was an error when trying to initiate the trace.
+ Let's report that to the user and bail out. */
+ fprintf_unfiltered (gdb_stderr, "Could not trace the inferior "
+ "process.\n");
+ fprintf_unfiltered (gdb_stderr, "Error: %s\n",
+ safe_strerror (trace_errno));
+ gdb_flush (gdb_stderr);
+ _exit (0177);
+ }

/* The call above set this process (the "child") as debuggable
by the original gdb process (the "parent"). Since processes
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 9935dcb..f3b7bb7 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2119,14 +2119,19 @@ cur_inf (void)
return gnu_current_inf;
}

-static void
-gnu_ptrace_me (void)
+static bool
+gnu_ptrace_me (int *trace_errno)
{
/* We're in the child; make this process stop as soon as it execs. */
struct inf *inf = cur_inf ();
inf_debug (inf, "tracing self");
if (ptrace (PTRACE_TRACEME) != 0)
- error (_("ptrace (PTRACE_TRACEME) failed!"));
+ {
+ if (trace_errno != NULL)
+ *trace_errno = errno;
+ return false;
+ }
+ return true;
}

static void
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index f61bfe7..7d41842 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -75,11 +75,19 @@ inf_ptrace_remove_fork_catchpoint (struct target_ops *self, int pid)

/* Prepare to be traced. */

-static void
-inf_ptrace_me (void)
+static bool
+inf_ptrace_me (int *trace_errno)
{
+ bool ret = true;
+
/* "Trace me, Dr. Memory!" */
- ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3)0, 0);
+ if (ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3) 0, 0) < 0)
+ {
+ if (trace_errno != NULL)
+ *trace_errno = errno;
+ ret = false;
+ }
+ return ret;
}

/* Start a new inferior Unix child process. EXEC_FILE is the file to
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 258cc29..6c6ec15 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -133,7 +133,7 @@ extern void child_terminal_init_with_pgrp (int pgrp);
/* From fork-child.c */

extern int fork_inferior (char *, char *, char **,
- void (*)(void),
+ bool (*)(int *trace_errno),
void (*)(int), void (*)(void), char *,
void (*)(const char *,
char * const *, char * const *));
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 2269016..6000202 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -4411,8 +4411,8 @@ procfs_init_inferior (struct target_ops *ops, int pid)
synchronize with the parent process. The parent process should
take care of the details. */

-static void
-procfs_set_exec_trap (void)
+static bool
+procfs_set_exec_trap (int *trace_errno)
{
/* This routine called on the child side (inferior side)
after GDB forks the inferior. It must use only local variables,
@@ -4510,6 +4510,8 @@ procfs_set_exec_trap (void)
/* FIXME: No need to destroy the procinfo --
we have our own address space, and we're about to do an exec! */
/*destroy_procinfo (pi);*/
+
+ return true;
}

/* This function is called BEFORE gdb forks the inferior process. Its
--
2.9.3