Re: [PATCH v2] Improve ptrace-error detection on Linux targets

Sergio Durigan Junior
2019-09-04T19:21:56Z

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 <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

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible