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

Pedro Alves
2019-09-26T18:13:34Z

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