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

[Pedro Alves <palves@redhat.com>]

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?

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

>  /* 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...

>  }
>  
>  /* 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
> +   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;
>  }
>  \f
>  

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

> +    }
> +

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

> +	error (_("Attaching to %s failed%s%s"),
> +	       target_pid_to_str (ptid_t (pid)).c_str (),
> +	       !errmsg.empty () ? "\n" : "",
> +	       errmsg.c_str ());
> +      }
>      }
>  
>    set_current_inferior (remote_add_inferior (false, pid, 1, 0));
> 

Thanks,
Pedro Alves