From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1520 invoked by alias); 4 Sep 2019 19:21:56 -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 1508 invoked by uid 89); 4 Sep 2019 19:21:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=HTo:U*palves 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; Wed, 04 Sep 2019 19:21:52 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 70AE3302C07F; Wed, 4 Sep 2019 19:21:51 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1DD7360C18; Wed, 4 Sep 2019 19:21:51 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Eli Zaretskii , Ruslan Kabatsayev Subject: Re: [PATCH v2] Improve ptrace-error detection on Linux targets References: <20190819032918.3536-1-sergiodj@redhat.com> <20190826183205.19008-1-sergiodj@redhat.com> <28c4f743-91f1-59c3-83ff-3f791811f996@redhat.com> <87mufrai1z.fsf@redhat.com> Date: Wed, 04 Sep 2019 19:21:00 -0000 In-Reply-To: (Pedro Alves's message of "Fri, 30 Aug 2019 13:45:37 +0100") Message-ID: <877e6nsw8h.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00033.txt.bz2 On Friday, August 30 2019, Pedro Alves wrote: > On 8/29/19 8:27 PM, Sergio Durigan Junior wrote: >> On Thursday, August 29 2019, Pedro Alves wrote: > >>>> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h >>>> index fd2f12a342..04ada53bf6 100644 >>>> --- a/gdb/nat/linux-ptrace.h >>>> +++ b/gdb/nat/linux-ptrace.h >>>> @@ -176,13 +176,26 @@ struct buffer; >>>> # define TRAP_HWBKPT 4 >>>> #endif >>>> >>>> -extern std::string linux_ptrace_attach_fail_reason (pid_t pid); >>>> +/* Find all possible reasons we could fail to attach PID and return >>>> + these as a string. An empty string is returned if we didn't find >>>> + any reason. If ERR is EACCES or EPERM, we also add a warning about >>>> + possible restrictions to use ptrace. */ >>>> +extern std::string linux_ptrace_attach_fail_reason (pid_t pid, int err = -1); >>> >>> If ERR is an errno number, then it's a bit odd to use -1 for default, >>> since errno == 0 is the traditional "no error" number. Pedantically, I believe >>> there's no garantee that a valid error number must be a positive integer. >>> >>> But, why the default argument in the first place? What calls this >>> without passing an error? >> >> So, the only place that calls linux_ptrace_attach_fail_reason without >> passing the ERR argument is linux_ptrace_attach_fail_reason_string: >> >> std::string >> linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err) >> { >> long lwpid = ptid.lwp (); >> std::string reason = linux_ptrace_attach_fail_reason (lwpid); >> >> if (!reason.empty ()) >> return string_printf ("%s (%d), %s", safe_strerror (err), err, >> reason.c_str ()); >> else >> return string_printf ("%s (%d)", safe_strerror (err), err); >> } >> >> In this case, I opted to keep it as is because the function will compose >> a string contaning like: >> >> A (B)[: C] >> >> Where: >> >> A = safe_strerror >> B = errno >> C = fail reason (optional) >> >> This function (linux_ptrace_attach_fail_reason_string) is called in >> three places: >> >> gdb/linux-nat.c: >> >> std::string reason >> = linux_ptrace_attach_fail_reason_string (ptid, err); >> >> warning (_("Cannot attach to lwp %d: %s"), >> lwpid, reason.c_str ()); >> >> gdb/gdbserver/linux-low.c: >> >> std::string reason >> = linux_ptrace_attach_fail_reason_string (ptid, err); >> >> warning (_("Cannot attach to lwp %d: %s"), lwpid, reason.c_str ()); >> >> and >> >> std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err); >> error ("Cannot attach to process %ld: %s", pid, reason.c_str ()); >> >> >> It seems to me like these error messages are expecting a short string to >> just append to their existing strings, so I didn't think it made much >> sense to extend the ptrace error checking here as well. That's why I >> didn't extend linux_ptrace_attach_fail_reason_string to pass ERR down to >> linux_ptrace_attach_fail_reason. > > OK, I think that's just a bit too messy. Let's take a fresh look at the > result: > > /* Find all possible reasons we could fail to attach PID and return > these as a string. An empty string is returned if we didn't find > any reason. If ERR is EACCES or EPERM, we also add a warning about > possible restrictions to use ptrace. */ > extern std::string linux_ptrace_attach_fail_reason (pid_t pid, int err = -1); > > /* Find all possible reasons we could have failed to attach to PTID > and return them as a string. ERR is the error PTRACE_ATTACH failed > with (an errno). */ > extern std::string linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err); > > Both the functions have the exact same prototype (same parameters, same return). > And since they both return std::string, why is linux_ptrace_attach_fail_reason_string > called "xxx_string"? From the function names alone, I'd think > linux_ptrace_attach_fail_reason_string returned a string, while > linux_ptrace_attach_fail_reason returned something else, or printed > directly. But that's not the case. > > So linux_ptrace_attach_fail_reason_string is used when we're attaching > to an lwp, other than the thread group leader (the main thread). > IMO, it'd be better to rename that function accordingly, and update > its comments accordingly too. > > Then, it becomes clearer that linux_ptrace_attach_fail_reason is to > be used in the initial process attach, in which case we want to > check the ptrace permissions. > > Also, we can factor out things such that we don't need the > default parameter. I tend to prefer avoiding mode-setting default > parameters, preferring differently named functions, because default > parameters make it harder to grep around the sources, distinguishing the > cases where you passed an argument or not. In this case, > the linux_ptrace_attach_fail_reason / linux_ptrace_attach_fail_reason_string > functions serve different purposes, so decoupling them via not using > default parameters is better, IMO, it avoids missing some conversion > in some spot. > > Which is exactly what happened. Here's a patch implementing > the idea. Notice how gdbserver/linux-low.c:linux_attach > has a spot where it is currently using linux_ptrace_attach_fail_reason_string > after attaching to the main thread fails. That spot should be including > the new ptrace restrictions fail reasons, but it wasn't due to use of > linux_ptrace_attach_fail_reason_string: > > @@ -1202,7 +1202,7 @@ linux_attach (unsigned long pid) > { > remove_process (proc); > > - std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err); > + std::string reason = linux_ptrace_attach_fail_reason (pid, err); > error ("Cannot attach to process %ld: %s", pid, reason.c_str ()); > } > > I haven't checked whether the resulting error message looks good as is, > or whether we need to tweak that error call in addition. Can you take it > from here? > > Here's the patch on top of yours. Sure, thanks for the patch. I've tried it here and it seems fine. I'll submit it integrated into v3 soon. > > From a3e8744c1ed7fa8c702009fe3caf8578bb1785ea Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Fri, 30 Aug 2019 13:15:12 +0100 > Subject: [PATCH] linux_ptrace_attach_fail_reason > > --- > gdb/gdbserver/linux-low.c | 4 ++-- > gdb/gdbserver/thread-db.c | 2 +- > gdb/linux-nat.c | 2 +- > gdb/nat/linux-ptrace.c | 24 ++++++++++++++++++------ > gdb/nat/linux-ptrace.h | 10 ++++++---- > 5 files changed, 28 insertions(+), 14 deletions(-) > > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c > index 1e0a5cbf54d..45cf43ad9ed 100644 > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c > @@ -1170,7 +1170,7 @@ attach_proc_task_lwp_callback (ptid_t ptid) > else if (err != 0) > { > std::string reason > - = linux_ptrace_attach_fail_reason_string (ptid, err); > + = linux_ptrace_attach_fail_reason_lwp (ptid, err); > > warning (_("Cannot attach to lwp %d: %s"), lwpid, reason.c_str ()); > } > @@ -1202,7 +1202,7 @@ linux_attach (unsigned long pid) > { > remove_process (proc); > > - std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err); > + std::string reason = linux_ptrace_attach_fail_reason (pid, err); > error ("Cannot attach to process %ld: %s", pid, reason.c_str ()); > } > > diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c > index b2791b0513a..cfba05977e6 100644 > --- a/gdb/gdbserver/thread-db.c > +++ b/gdb/gdbserver/thread-db.c > @@ -225,7 +225,7 @@ attach_thread (const td_thrhandle_t *th_p, td_thrinfo_t *ti_p) > err = linux_attach_lwp (ptid); > if (err != 0) > { > - std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err); > + std::string reason = linux_ptrace_attach_fail_reason_lwp (ptid, err); > > warning ("Could not attach to thread %ld (LWP %d): %s", > (unsigned long) ti_p->ti_tid, ti_p->ti_lid, reason.c_str ()); > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c > index b5a9eaf72e3..22cb55e6dcc 100644 > --- a/gdb/linux-nat.c > +++ b/gdb/linux-nat.c > @@ -1136,7 +1136,7 @@ attach_proc_task_lwp_callback (ptid_t ptid) > else > { > std::string reason > - = linux_ptrace_attach_fail_reason_string (ptid, err); > + = linux_ptrace_attach_fail_reason_lwp (ptid, err); > > warning (_("Cannot attach to lwp %d: %s"), > lwpid, reason.c_str ()); > diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c > index 599d9cfb550..2201a24d812 100644 > --- a/gdb/nat/linux-ptrace.c > +++ b/gdb/nat/linux-ptrace.c > @@ -92,10 +92,13 @@ for more details.\n"); > return ret; > } > > -/* See declaration in linux-ptrace.h. */ > +/* Find all possible reasons we could fail to attach PID and return > + these as a string. An empty string is returned if we didn't find > + any reason. Helper for linux_ptrace_attach_fail_reason and > + linux_ptrace_attach_fail_reason_lwp. */ > > -std::string > -linux_ptrace_attach_fail_reason (pid_t pid, int err) > +static std::string > +linux_ptrace_attach_fail_reason_1 (pid_t pid) > { > pid_t tracerpid = linux_proc_get_tracerpid_nowarn (pid); > std::string result; > @@ -111,8 +114,17 @@ linux_ptrace_attach_fail_reason (pid_t pid, int err) > "terminated"), > (int) pid); > > - std::string ptrace_restrict = linux_ptrace_restricted_fail_reason (err); > + return result; > +} > > +/* See declaration in linux-ptrace.h. */ > + > +std::string > +linux_ptrace_attach_fail_reason (pid_t pid, int err) > +{ > + std::string result = linux_ptrace_attach_fail_reason_1 (pid); > + > + std::string ptrace_restrict = linux_ptrace_restricted_fail_reason (err); > if (!ptrace_restrict.empty ()) > result += "\n" + ptrace_restrict; > > @@ -122,10 +134,10 @@ linux_ptrace_attach_fail_reason (pid_t pid, int err) > /* See linux-ptrace.h. */ > > std::string > -linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err) > +linux_ptrace_attach_fail_reason_lwp (ptid_t ptid, int err) > { > long lwpid = ptid.lwp (); > - std::string reason = linux_ptrace_attach_fail_reason (lwpid); > + std::string reason = linux_ptrace_attach_fail_reason_1 (lwpid); > > if (!reason.empty ()) > return string_printf ("%s (%d), %s", safe_strerror (err), err, > diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h > index 04ada53bf69..94c9ba48ba5 100644 > --- a/gdb/nat/linux-ptrace.h > +++ b/gdb/nat/linux-ptrace.h > @@ -180,12 +180,14 @@ struct buffer; > these as a string. An empty string is returned if we didn't find > any reason. If ERR is EACCES or EPERM, we also add a warning about > possible restrictions to use ptrace. */ > -extern std::string linux_ptrace_attach_fail_reason (pid_t pid, int err = -1); > +extern std::string linux_ptrace_attach_fail_reason (pid_t pid, int err); > > -/* Find all possible reasons we could have failed to attach to PTID > +/* Find all possible reasons we could have failed to attach to LWPID > and return them as a string. ERR is the error PTRACE_ATTACH failed > - with (an errno). */ > -extern std::string linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err); > + with (an errno). Unlike linux_ptrace_attach_fail_reason, this > + function should be used when attaching to an LWP other than the > + leader; it does not warn about ptrace restrictions. */ > +extern std::string linux_ptrace_attach_fail_reason_lwp (ptid_t lwpid, int err); > > /* When the call to 'ptrace (PTRACE_TRACEME...' fails, and we have > already forked, this function can be called in order to try to > -- > 2.14.5 -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/