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

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

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/