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

Sergio Durigan Junior
2017-02-18T04:55:57Z

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