From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10650 invoked by alias); 17 Feb 2017 10:48:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 9932 invoked by uid 89); 17 Feb 2017 10:48:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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; Fri, 17 Feb 2017 10:48:29 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (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 4875561BA6; Fri, 17 Feb 2017 10:48:29 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1HAmRYK016920; Fri, 17 Feb 2017 05:48:27 -0500 Subject: Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded To: Sergio Durigan Junior , GDB Patches References: <20170216201931.5843-1-sergiodj@redhat.com> Cc: dje@google.com From: Pedro Alves Message-ID: Date: Fri, 17 Feb 2017 10:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170216201931.5843-1-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-02/txt/msg00468.txt.bz2 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