From patchwork Thu Feb 16 20:19:31 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 19275 Received: (qmail 97244 invoked by alias); 16 Feb 2017 20:19:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 96628 invoked by uid 89); 16 Feb 2017 20:19:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=honour, inferiorh, Dummy, inferior.h X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 16 Feb 2017 20:19:43 +0000 Received: from smtp.corp.redhat.com (int-mx16.intmail.prod.int.phx2.redhat.com [10.5.11.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5EA3E61BA8; Thu, 16 Feb 2017 20:19:43 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTP id AA0DC3066B; Thu, 16 Feb 2017 20:19:42 +0000 (UTC) From: Sergio Durigan Junior To: GDB Patches Cc: dje@google.com, palves@redhat.com, Sergio Durigan Junior Subject: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded Date: Thu, 16 Feb 2017 15:19:31 -0500 Message-Id: <20170216201931.5843-1-sergiodj@redhat.com> X-IsSubscribed: yes 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 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