[PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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