From patchwork Sat Feb 18 05:09:00 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: 19293 Received: (qmail 11721 invoked by alias); 18 Feb 2017 05:09:09 -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 11697 invoked by uid 89); 18 Feb 2017 05:09:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=printed 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; Sat, 18 Feb 2017 05:09:06 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9C097C057FA7; Sat, 18 Feb 2017 05:09:06 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1I595M1022680; Sat, 18 Feb 2017 00:09:06 -0500 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: Sat, 18 Feb 2017 00:09:00 -0500 Message-Id: <20170218050900.31399-1-sergiodj@redhat.com> In-Reply-To: <20170216201931.5843-1-sergiodj@redhat.com> References: <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. Thanks to Pedro, this second version of the patch is simpler and more more logical. Basically, two helper functions are added: trace_start_error and trace_start_error_with_name. The former can be used when there is a customized error message to be printed to the user. The latter works like perror_with_name, so you just need to pass the function that error'd. Both helper functions mentioned above do basically the same thing: print the error message to stderr and call _exit, properly terminating the forked inferior. Most of the patch takes care of guarding the necessary system calls against errors on the "traceme_fun" callbacks. It is not right to call error on these situations, so I've replaced these calls with the proper helper function call. Regression-tested on BuildBot. Thanks, gdb/ChangeLog: 2017-02-17 Sergio Durigan Junior Pedro Alves PR gdb/16188 * darwin-nat.c (darwin_ptrace_me): Check if calls to system calls succeeded. * fork-child.c (trace_start_error): New function. (trace_start_error_with_name): Likewise. * gnu-nat.c (gnu_ptrace_me): Check if call to PTRACE succeeded. * inf-ptrace.c (inf_ptrace_me): Likewise. * inferior.h (trace_start_error): New prototype. (trace_start_error_with_name): Likewise. --- gdb/darwin-nat.c | 20 +++++++++++++------- gdb/fork-child.c | 25 +++++++++++++++++++++++++ gdb/gnu-nat.c | 2 +- gdb/inf-ptrace.c | 3 ++- gdb/inferior.h | 14 ++++++++++++++ 5 files changed, 55 insertions(+), 9 deletions(-) diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c index 8c5e8a0..e02e51d 100644 --- a/gdb/darwin-nat.c +++ b/gdb/darwin-nat.c @@ -254,7 +254,6 @@ darwin_ptrace (const char *name, { int ret; - errno = 0; ret = ptrace (request, pid, arg3, arg4); if (ret == -1 && errno == 0) ret = 0; @@ -1728,23 +1727,30 @@ darwin_ptrace_me (void) int res; char c; + errno = 0; /* Close write end point. */ - close (ptrace_fds[1]); + if (close (ptrace_fds[1]) < 0) + trace_start_error_with_name ("close"); /* 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]); + trace_start_error (_("unable to read from pipe, read returned: %d"), res); + + if (close (ptrace_fds[0]) < 0) + trace_start_error_with_name ("close"); /* Get rid of privileges. */ - setegid (getgid ()); + if (setegid (getgid ()) < 0) + trace_start_error_with_name ("setegid"); /* Set TRACEME. */ - PTRACE (PT_TRACE_ME, 0, 0, 0); + if (PTRACE (PT_TRACE_ME, 0, 0, 0) < 0) + trace_start_error_with_name ("PTRACE"); /* Redirect signals to exception port. */ - PTRACE (PT_SIGEXC, 0, 0, 0); + if (PTRACE (PT_SIGEXC, 0, 0, 0) < 0) + trace_start_error_with_name ("PTRACE"); } /* 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..f6256fb 100644 --- a/gdb/fork-child.c +++ b/gdb/fork-child.c @@ -109,6 +109,31 @@ escape_bang_in_quoted_argument (const char *shell_file) return 0; } +/* See inferior.h. */ + +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); +} + +/* See inferior.h. */ + +void +trace_start_error_with_name (const char *string) +{ + trace_start_error ("%s: %s", string, safe_strerror (errno)); +} + /* Start an inferior Unix child process and sets inferior_ptid to its pid. EXEC_FILE is the file to run. ALLARGS is a string containing the arguments to the program. ENV is the environment vector to diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c index 9935dcb..7efb3c1 100644 --- a/gdb/gnu-nat.c +++ b/gdb/gnu-nat.c @@ -2126,7 +2126,7 @@ gnu_ptrace_me (void) struct inf *inf = cur_inf (); inf_debug (inf, "tracing self"); if (ptrace (PTRACE_TRACEME) != 0) - error (_("ptrace (PTRACE_TRACEME) failed!")); + trace_start_error_with_name ("ptrace"); } static void diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c index f61bfe7..21578742 100644 --- a/gdb/inf-ptrace.c +++ b/gdb/inf-ptrace.c @@ -79,7 +79,8 @@ static void inf_ptrace_me (void) { /* "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) + trace_start_error_with_name ("ptrace"); } /* 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..7c0ddf3 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -132,6 +132,20 @@ extern void child_terminal_init_with_pgrp (int pgrp); /* From fork-child.c */ +/* Report an error that happened when starting to trace the inferior + (i.e., when the "traceme_fun" callback is called on fork_inferior) + and bail out. This function does not return. */ + +extern void trace_start_error (const char *fmt, ...) + ATTRIBUTE_NORETURN; + +/* 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. */ + +extern void trace_start_error_with_name (const char *string) + ATTRIBUTE_NORETURN; + extern int fork_inferior (char *, char *, char **, void (*)(void), void (*)(int), void (*)(void), char *,