[PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

[PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

[Sergio Durigan Junior]

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

Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

[Pedro Alves]

On 02/16/2017 08:19 PM, Sergio Durigan Junior wrote:

> 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.

Off hand, all that comes up is to write a LD_PRELOAD wrapper around
ptrace that always fails, similar to testsuite/lib/read1.c.
But that'd only work for that specific call and only for ptrace targets.
And it'd probably conflict with the ptrace calls that we do early on
gdb startup to probe for level of ptrace support.  Likely not work the
trouble.

>  
> -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;


Hmm, seeing this makes me wonder about the interface
of returning the errno.  It loses detail on context of what
exactly fails.

Throwing an error/exception and catching it from fork_inferior instead would
be the "obvious" choice, but we're in an async-signal-safe context (we're
in a fork child, before calling exec), and we already do things that we
shouldn't, and I wouldn't want to make it worse.

But, all we do when we "catch" the error is print something and _exit.
So I'm wondering whether a couple functions similar to "error"
and "perror_with_name" but that print the error and _exit themselves
wouldn't be a better interface.  I think it'd result in less boilerplate.
Something like these exported from fork-child.c:

extern void trace_start_error (const char *fmt, ...)
  ATTRIBUTE_NORETURN;
extern void trace_start_error_with_name (const char *string)
  ATTRIBUTE_NORETURN;

/* There was an error when trying to initiate the trace in
   the fork child.  Report the error to the user and bail out.  */

void
trace_start_error (const char *fmt, ...)
{
  va_list ap;

  va_start (ap, fmt);
  fprintf_unfiltered (gdb_stderr, "Could not trace the inferior "
		                  "process.\nError: ");
  vfprintf_unfiltered (gdb_stderr, fmt, ap);
  va_end (args);

  gdb_flush (gdb_stderr);
  _exit (0177);
}

/* Like "trace_start_error", but the error message is constructed
   by combining STRING with the system error message for errno.
   This function does not return.  */

void
trace_start_error_with_name (const char *string)
{
  fatal_trace_error ("%s", safe_strerror (trace_errno));
}


and then in darwin_ptrace_me you'd do:

   if (res != 0)
-    error (_("unable to read from pipe, read returned: %d"), res);
+     trace_start_error (_("unable to read from pipe, read returned: %d"), res);

> +    }
> +  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;
> +

And these gotos would be replaced with calls
to trace_start_error_with_name, etc.

Thanks,
Pedro Alves

Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

[Sergio Durigan Junior]

Thanks for the review.

On Friday, February 17 2017, Pedro Alves wrote:

> On 02/16/2017 08:19 PM, Sergio Durigan Junior wrote:
>
>> 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.
>
> Off hand, all that comes up is to write a LD_PRELOAD wrapper around
> ptrace that always fails, similar to testsuite/lib/read1.c.
> But that'd only work for that specific call and only for ptrace targets.
> And it'd probably conflict with the ptrace calls that we do early on
> gdb startup to probe for level of ptrace support.  Likely not work the
> trouble.

Yeah.  I mean, even though I really like testcases for all the bugs
fixed (and yeah, sorry again for not including the testcase on my last
patch to fix the 'maint print symbols' thing), I think this code is
simple enough *and* the testcase is hard enough that the cost-benefit is
not very good.

>>  
>> -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;
>
>
> Hmm, seeing this makes me wonder about the interface
> of returning the errno.  It loses detail on context of what
> exactly fails.
>
> Throwing an error/exception and catching it from fork_inferior instead would
> be the "obvious" choice, but we're in an async-signal-safe context (we're
> in a fork child, before calling exec), and we already do things that we
> shouldn't, and I wouldn't want to make it worse.

I've noticed a few places were calling error on these ptrace_me
functions, which is clearly incorrect IIUC.  Anyway, maybe my next patch
(cleanup fork_inferior and related) can address some of these issues.

> But, all we do when we "catch" the error is print something and _exit.
> So I'm wondering whether a couple functions similar to "error"
> and "perror_with_name" but that print the error and _exit themselves
> wouldn't be a better interface.  I think it'd result in less boilerplate.
> Something like these exported from fork-child.c:
>
> extern void trace_start_error (const char *fmt, ...)
>   ATTRIBUTE_NORETURN;
> extern void trace_start_error_with_name (const char *string)
>   ATTRIBUTE_NORETURN;
>
> /* There was an error when trying to initiate the trace in
>    the fork child.  Report the error to the user and bail out.  */
>
> void
> trace_start_error (const char *fmt, ...)
> {
>   va_list ap;
>
>   va_start (ap, fmt);
>   fprintf_unfiltered (gdb_stderr, "Could not trace the inferior "
> 		                  "process.\nError: ");
>   vfprintf_unfiltered (gdb_stderr, fmt, ap);
>   va_end (args);
>
>   gdb_flush (gdb_stderr);
>   _exit (0177);
> }
>
> /* Like "trace_start_error", but the error message is constructed
>    by combining STRING with the system error message for errno.
>    This function does not return.  */
>
> void
> trace_start_error_with_name (const char *string)
> {
>   fatal_trace_error ("%s", safe_strerror (trace_errno));
> }
>
>
> and then in darwin_ptrace_me you'd do:
>
>    if (res != 0)
> -    error (_("unable to read from pipe, read returned: %d"), res);
> +     trace_start_error (_("unable to read from pipe, read returned: %d"), res);
>
>> +    }
>> +  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;
>> +
>
> And these gotos would be replaced with calls
> to trace_start_error_with_name, etc.

Wow, thanks for the insights.  I'll make sure to include your name in
the ChangeLog entry.

I'll send v2 soon.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

[Sergio Durigan Junior]

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.

Thanks to Pedro, this second version of the patch is simpler and more
more logical.  Basically, two helper functions are added:
trace_start_error and trace_start_error_with_name.  The former can be
used when there is a customized error message to be printed to the
user.  The latter works like perror_with_name, so you just need to
pass the function that error'd.

Both helper functions mentioned above do basically the same thing:
print the error message to stderr and call _exit, properly terminating
the forked inferior.

Most of the patch takes care of guarding the necessary system calls
against errors on the "traceme_fun" callbacks.  It is not right to
call error on these situations, so I've replaced these calls with the
proper helper function call.

Regression-tested on BuildBot.

Thanks,

gdb/ChangeLog:
2017-02-17  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	PR gdb/16188
	* darwin-nat.c (darwin_ptrace_me): Check if calls to system
	calls succeeded.
	* fork-child.c (trace_start_error): New function.
	(trace_start_error_with_name): Likewise.
	* gnu-nat.c (gnu_ptrace_me): Check if call to PTRACE succeeded.
	* inf-ptrace.c (inf_ptrace_me): Likewise.
	* inferior.h (trace_start_error): New prototype.
	(trace_start_error_with_name): Likewise.
---
 gdb/darwin-nat.c | 20 +++++++++++++-------
 gdb/fork-child.c | 25 +++++++++++++++++++++++++
 gdb/gnu-nat.c    |  2 +-
 gdb/inf-ptrace.c |  3 ++-
 gdb/inferior.h   | 14 ++++++++++++++
 5 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 8c5e8a0..e02e51d 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -254,7 +254,6 @@ darwin_ptrace (const char *name,
 {
   int ret;
 
-  errno = 0;
   ret = ptrace (request, pid, arg3, arg4);
   if (ret == -1 && errno == 0)
     ret = 0;
@@ -1728,23 +1727,30 @@ darwin_ptrace_me (void)
   int res;
   char c;
 
+  errno = 0;
   /* Close write end point.  */
-  close (ptrace_fds[1]);
+  if (close (ptrace_fds[1]) < 0)
+    trace_start_error_with_name ("close");
 
   /* 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]);
+    trace_start_error (_("unable to read from pipe, read returned: %d"), res);
+
+  if (close (ptrace_fds[0]) < 0)
+    trace_start_error_with_name ("close");
 
   /* Get rid of privileges.  */
-  setegid (getgid ());
+  if (setegid (getgid ()) < 0)
+    trace_start_error_with_name ("setegid");
 
   /* Set TRACEME.  */
-  PTRACE (PT_TRACE_ME, 0, 0, 0);
+  if (PTRACE (PT_TRACE_ME, 0, 0, 0) < 0)
+    trace_start_error_with_name ("PTRACE");
 
   /* Redirect signals to exception port.  */
-  PTRACE (PT_SIGEXC, 0, 0, 0);
+  if (PTRACE (PT_SIGEXC, 0, 0, 0) < 0)
+    trace_start_error_with_name ("PTRACE");
 }
 
 /* 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..f6256fb 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -109,6 +109,31 @@ escape_bang_in_quoted_argument (const char *shell_file)
   return 0;
 }
 
+/* See inferior.h.  */
+
+void
+trace_start_error (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  fprintf_unfiltered (gdb_stderr, "Could not trace the inferior "
+		                  "process.\nError: ");
+  vfprintf_unfiltered (gdb_stderr, fmt, ap);
+  va_end (args);
+
+  gdb_flush (gdb_stderr);
+  _exit (0177);
+}
+
+/* See inferior.h.  */
+
+void
+trace_start_error_with_name (const char *string)
+{
+  trace_start_error ("%s: %s", string, safe_strerror (errno));
+}
+
 /* Start an inferior Unix child process and sets inferior_ptid to its
    pid.  EXEC_FILE is the file to run.  ALLARGS is a string containing
    the arguments to the program.  ENV is the environment vector to
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 9935dcb..7efb3c1 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2126,7 +2126,7 @@ gnu_ptrace_me (void)
   struct inf *inf = cur_inf ();
   inf_debug (inf, "tracing self");
   if (ptrace (PTRACE_TRACEME) != 0)
-    error (_("ptrace (PTRACE_TRACEME) failed!"));
+    trace_start_error_with_name ("ptrace");
 }
 
 static void
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index f61bfe7..21578742 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -79,7 +79,8 @@ static void
 inf_ptrace_me (void)
 {
   /* "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)
+    trace_start_error_with_name ("ptrace");
 }
 
 /* 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..7c0ddf3 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -132,6 +132,20 @@ extern void child_terminal_init_with_pgrp (int pgrp);
 
 /* From fork-child.c */
 
+/* Report an error that happened when starting to trace the inferior
+   (i.e., when the "traceme_fun" callback is called on fork_inferior)
+   and bail out.  This function does not return.  */
+
+extern void trace_start_error (const char *fmt, ...)
+  ATTRIBUTE_NORETURN;
+
+/* Like "trace_start_error", but the error message is constructed by
+   combining STRING with the system error message for errno.  This
+   function does not return.  */
+
+extern void trace_start_error_with_name (const char *string)
+  ATTRIBUTE_NORETURN;
+
 extern int fork_inferior (char *, char *, char **,
 			  void (*)(void),
 			  void (*)(int), void (*)(void), char *,
-- 
2.9.3

Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

[Pedro Alves]

Hi Sergio,

This LGTM, save for the errno handling in Darwin bits:

On 02/18/2017 05:09 AM, Sergio Durigan Junior wrote:
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 8c5e8a0..e02e51d 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -254,7 +254,6 @@ darwin_ptrace (const char *name,
>  {
>    int ret;
>  
> -  errno = 0;
>    ret = ptrace (request, pid, arg3, arg4);
>    if (ret == -1 && errno == 0)
>      ret = 0;

Removing "errno = 0" here is incorrect.  There are ptrace calls where a -1
return is not an error, thus that check for "errno==0" after the
ptrace call.  Since system calls are not required to clear errno on
success, that errno=0 is required.

This is Darwin, but the Linux man pages, in "man ptrace" say:

 On error, all requests return -1, and errno is set appropriately.  Since the
 value returned by a successful PTRACE_PEEK* request may be -1, the caller
 must clear errno before the call, and then check it afterward to determine whether
 or not an error occurred.

And actually, the comment just above darwin_ptrace talks
about clearning errno.  So it's really incorrect.

> @@ -1728,23 +1727,30 @@ darwin_ptrace_me (void)
>    int res;
>    char c;
>  
> +  errno = 0;

OTOH, I don't see the need to clear it here.  Below,
errno will only be used when a syscall fails, and in
failure case, the syscall must set errno.

>    /* Close write end point.  */
> -  close (ptrace_fds[1]);
> +  if (close (ptrace_fds[1]) < 0)
> +    trace_start_error_with_name ("close");
>  
>    /* 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]);
> +    trace_start_error (_("unable to read from pipe, read returned: %d"), res);
> +
> +  if (close (ptrace_fds[0]) < 0)
> +    trace_start_error_with_name ("close");
>  
>    /* Get rid of privileges.  */
> -  setegid (getgid ());
> +  if (setegid (getgid ()) < 0)
> +    trace_start_error_with_name ("setegid");
>  
>    /* Set TRACEME.  */
> -  PTRACE (PT_TRACE_ME, 0, 0, 0);
> +  if (PTRACE (PT_TRACE_ME, 0, 0, 0) < 0)
> +    trace_start_error_with_name ("PTRACE");
>  
>    /* Redirect signals to exception port.  */
> -  PTRACE (PT_SIGEXC, 0, 0, 0);
> +  if (PTRACE (PT_SIGEXC, 0, 0, 0) < 0)
> +    trace_start_error_with_name ("PTRACE");
>  }

Thanks,
Pedro Alves

Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded

[Sergio Durigan Junior]

On Monday, February 20 2017, Pedro Alves wrote:

> Hi Sergio,
>
> This LGTM, save for the errno handling in Darwin bits:
>
> On 02/18/2017 05:09 AM, Sergio Durigan Junior wrote:
>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index 8c5e8a0..e02e51d 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -254,7 +254,6 @@ darwin_ptrace (const char *name,
>>  {
>>    int ret;
>>  
>> -  errno = 0;
>>    ret = ptrace (request, pid, arg3, arg4);
>>    if (ret == -1 && errno == 0)
>>      ret = 0;
>
> Removing "errno = 0" here is incorrect.  There are ptrace calls where a -1
> return is not an error, thus that check for "errno==0" after the
> ptrace call.  Since system calls are not required to clear errno on
> success, that errno=0 is required.
>
> This is Darwin, but the Linux man pages, in "man ptrace" say:
>
>  On error, all requests return -1, and errno is set appropriately.  Since the
>  value returned by a successful PTRACE_PEEK* request may be -1, the caller
>  must clear errno before the call, and then check it afterward to determine whether
>  or not an error occurred.
>
> And actually, the comment just above darwin_ptrace talks
> about clearning errno.  So it's really incorrect.

Oh, I'm really sorry, this was actually a mistake on the patch.  I meant
to delete the 'errno = 0;' on darwin_ptrace_me, not on darwin_ptrace.
Of course, I understand that errno must be cleared before the ptrace
call and I had read the exact same paragraph on the manpage.  Anyway,
sorry for wasting your time on this.

>> @@ -1728,23 +1727,30 @@ darwin_ptrace_me (void)
>>    int res;
>>    char c;
>>  
>> +  errno = 0;
>
> OTOH, I don't see the need to clear it here.  Below,
> errno will only be used when a syscall fails, and in
> failure case, the syscall must set errno.

Yeah.

I fixed the mistake and pushed the patch.  Thanks.

  0db8980cc0ee05727c11f8b7c6674137a4d5de4e

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] Fix thinko on last commit

[Sergio Durigan Junior]

On fork-child.c:trace_start_error, va_end should refer to 'ap', not
'args.  This fixes it.

Sorry about the breakage.

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

	PR gdb/16188
	* fork-child.c (trace_start_error): Fix thinko.  va_end should
	refer to 'ap', not 'args'.
---
 gdb/ChangeLog    | 6 ++++++
 gdb/fork-child.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index cf68c7e..610755c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,4 +1,10 @@
 2017-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	PR gdb/16188
+	* fork-child.c (trace_start_error): Fix thinko.  va_end should
+	refer to 'ap', not 'args'.
+
+2017-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
 	    Pedro Alves  <palves@redhat.com>
 
 	PR gdb/16188
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index f6256fb..dc2a314 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -120,7 +120,7 @@ trace_start_error (const char *fmt, ...)
   fprintf_unfiltered (gdb_stderr, "Could not trace the inferior "
 		                  "process.\nError: ");
   vfprintf_unfiltered (gdb_stderr, fmt, ap);
-  va_end (args);
+  va_end (ap);
 
   gdb_flush (gdb_stderr);
   _exit (0177);
-- 
2.9.3

Re: [PATCH] Fix thinko on last commit

[Sergio Durigan Junior]

On Monday, February 20 2017, I wrote:

> On fork-child.c:trace_start_error, va_end should refer to 'ap', not
> 'args.  This fixes it.

I've pushed this as obvious.

> Sorry about the breakage.
>
> gdb/ChangeLog:
> 2017-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	PR gdb/16188
> 	* fork-child.c (trace_start_error): Fix thinko.  va_end should
> 	refer to 'ap', not 'args'.
> ---
>  gdb/ChangeLog    | 6 ++++++
>  gdb/fork-child.c | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index cf68c7e..610755c 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,4 +1,10 @@
>  2017-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
> +
> +	PR gdb/16188
> +	* fork-child.c (trace_start_error): Fix thinko.  va_end should
> +	refer to 'ap', not 'args'.
> +
> +2017-02-20  Sergio Durigan Junior  <sergiodj@redhat.com>
>  	    Pedro Alves  <palves@redhat.com>
>  
>  	PR gdb/16188
> diff --git a/gdb/fork-child.c b/gdb/fork-child.c
> index f6256fb..dc2a314 100644
> --- a/gdb/fork-child.c
> +++ b/gdb/fork-child.c
> @@ -120,7 +120,7 @@ trace_start_error (const char *fmt, ...)
>    fprintf_unfiltered (gdb_stderr, "Could not trace the inferior "
>  		                  "process.\nError: ");
>    vfprintf_unfiltered (gdb_stderr, fmt, ap);
> -  va_end (args);
> +  va_end (ap);
>  
>    gdb_flush (gdb_stderr);
>    _exit (0177);
> -- 
> 2.9.3

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/