From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118705 invoked by alias); 18 Feb 2017 04:55:57 -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 118667 invoked by uid 89); 18 Feb 2017 04:55:56 -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=insights, H*i:sk:f38a33a, H*f:sk:f38a33a 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 04:55:46 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 83F0A81226; Sat, 18 Feb 2017 04:55:46 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1I4tjKY017693 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 17 Feb 2017 23:55:46 -0500 From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , dje@google.com Subject: Re: [PATCH] PR gdb/16188: Verify PTRACE_TRACEME succeeded References: <20170216201931.5843-1-sergiodj@redhat.com> X-URL: http://blog.sergiodj.net Date: Sat, 18 Feb 2017 04:55:00 -0000 In-Reply-To: (Pedro Alves's message of "Fri, 17 Feb 2017 10:48:25 +0000") Message-ID: <87y3x4m71r.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00500.txt.bz2 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 http://sergiodj.net/