PR gdb/16188: Verify PTRACE_TRACEME succeeded
Commit Message
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(-)
Comments
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
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,
@@ -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). */
@@ -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
@@ -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
@@ -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
@@ -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 *));
@@ -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