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

[Pedro Alves <palves@redhat.com>]

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