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

Sergio Durigan Junior
2019-09-26T18:23:34Z

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;
>> }
>> \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.

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