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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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
http://sergiodj.net/