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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Thursday, September 26 2019, Pedro Alves wrote:

> On 9/26/19 5:21 AM, Sergio Durigan Junior wrote:
>
>> +
>> +@smallexample
>> +$ gdb program
>> +...
>> +Starting program: program
>> +warning: Could not trace the inferior process.
>> +Error:
>> +warning: ptrace: Permission denied
>
> This "Error:" above is stale, right?

In the documentation, yes.  This is not being output anymore in the
code.  I removed it from the docs now.

>> +There might be restrictions preventing ptrace from working.  Please see
>> +the appendix "Linux kernel ptrace restrictions" in the GDB documentation
>> +for more details.
>> +During startup program exited with code 127.
>> +(@value{GDBP})
>> +@end smallexample
>> +
>
>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index d64c3641ff..c0e15c122f 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -967,7 +967,8 @@ linux_ptrace_fun ()
>>  {
>>    if (ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
>>  	      (PTRACE_TYPE_ARG4) 0) < 0)
>> -    trace_start_error_with_name ("ptrace");
>> +    trace_start_error_with_name ("ptrace",
>> +				 linux_ptrace_me_fail_reason (errno).c_str ());
>
> This code path is also run by a fork child.  So it needs the same pipe
> treatment pointed out in v2 review, due to linux_ptrace_me_fail_reason not
> being async-signal-safe.  It does not make sense to do the treatment in one
> place and not on others.

OK, I will do that.

>>  /* Prepare to be traced.  */
>>  
>>  static void
>> @@ -101,7 +115,8 @@ inf_ptrace_me (void)
>>  {
>>    /* "Trace me, Dr. Memory!"  */
>>    if (ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3) 0, 0) < 0)
>> -    trace_start_error_with_name ("ptrace");
>> +    trace_start_error_with_name ("ptrace",
>> +				 inf_ptrace_me_fail_reason (errno).c_str ());
>
> Same here...

Likewise.

>>  }
>>  
>>  /* Start a new inferior Unix child process.  EXEC_FILE is the file to
>> diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
>> index 98b5d2e09e..7cdab9af89 100644
>> --- a/gdb/inf-ptrace.h
>> +++ b/gdb/inf-ptrace.h
>> @@ -83,4 +83,14 @@ protected:
>>  
>>  extern pid_t get_ptrace_pid (ptid_t);
>>  
>> +/* Pointer to "inf_ptrace_me_fail_reason", 
>
> As pointed out before, this part of the comment does not make sense.
> "inf_ptrace_me_fail_reason" is the name of the variable!
>
> It's the same as saying this:
>
>  /* Pointer to "ptr".  */
>  int *ptr;
>
> which is of course bogus.
>
> Just say something like:
>
>   /* Pointer to function that can be called by "inf_ptrace_me" (...)
>
>
> which implements a function

Done.

>> +   that can be called by "inf_ptrace_me" in order to obtain the reason
>> +   for a ptrace failure.  ERR is the ERRNO value set by the failing
>> +   ptrace call.
>> +
>> +   This pointer can be overriden by targets that want to personalize
>> +   the error message printed when ptrace fails (see linux-nat.c, for
>> +   example).  */
>> +extern std::string (*inf_ptrace_me_fail_reason) (int err);
>> +
>>  #endif
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index cd5cf1830d..2c7ded7043 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -1132,7 +1132,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 ());
>> @@ -1187,8 +1187,9 @@ linux_nat_target::attach (const char *args, int from_tty)
>>      }
>>    catch (const gdb_exception_error &ex)
>>      {
>> +      int saved_errno = errno;
>>        pid_t pid = parse_pid_to_attach (args);
>> -      std::string reason = linux_ptrace_attach_fail_reason (pid);
>> +      std::string reason = linux_ptrace_attach_fail_reason (pid, saved_errno);
>>  
>>        if (!reason.empty ())
>>  	throw_error (ex.error, "warning: %s\n%s", reason.c_str (),
>> @@ -4567,6 +4568,10 @@ Enables printf debugging output."),
>>    sigemptyset (&blocked_mask);
>>  
>>    lwp_lwpid_htab_create ();
>> +
>> +  /* Set the proper function to generate a message when ptrace
>> +     fails.  */
>> +  inf_ptrace_me_fail_reason = linux_ptrace_me_fail_reason;
>>  }
>>  
>>  
>
>> +static std::string
>> +linux_ptrace_restricted_fail_reason (int err)
>> +{
>> +  if (err != EACCES && err != EPERM)
>> +    {
>> +      /* It just makes sense to perform the checks below if errno was
>> +	 either EACCES or EPERM.  */
>> +      return {};
>> +    }
>> +
>> +  std::string ret;
>> +  gdb_dlhandle_up handle;
>> +
>> +  try
>> +    {
>> +      handle = gdb_dlopen ("libselinux.so.1");
>> +    }
>> +  catch (const gdb_exception_error &e)
>> +    {
>> +      handle.reset (nullptr);
>
> Nit, this line is unnecessary.  If an exception was thrown,
> then handle was not writen to by the try block.

Ah, I wasn't sure of it, so decided to be safe.  Thanks.

>> +    }
>> +
>
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -5825,8 +5825,20 @@ extended_remote_target::attach (const char *args, int from_tty)
>>      case PACKET_UNKNOWN:
>>        error (_("This target does not support attaching to a process"));
>>      default:
>> -      error (_("Attaching to %s failed"),
>> -	     target_pid_to_str (ptid_t (pid)).c_str ());
>> +      {
>> +	std::string errmsg = rs->buf.data ();
>> +
>> +	if (!errmsg.empty ())
>> +	  {
>> +	    /* Get rid of the "E." prefix.  */
>> +	    errmsg.erase (0, 2);
>> +	  }
>> +
>
> What if the server just sends a regular numeric error, like "E01"?

Maybe I looked at wrong places, but I couldn't find a standard way to
deal with these error codes.  This doesn't seem to be documented.

I can extend the code to just remove the "E." prefix if the string
starts with "E.", but that seems hacky.  Is there a more recommended way
to do this?

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/