This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Improve ptrace-error detection on Linux targets
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.
>From a3e8744c1ed7fa8c702009fe3caf8578bb1785ea Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
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