[PATCH] Improve ptrace-error detection on Linux targets

classic Classic list List threaded Threaded
99 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:

Sergio> Changes from v3:

Thanks for doing this.  I like this change quite a bit.  For one thing I
think it would have saved me some time over the years, as I randomly
forget about ptrace security measures on new machines.

Sergio> +++ b/gdb/gdbserver/server.c
Sergio> @@ -2913,9 +2913,21 @@ handle_v_attach (char *own_buf)
Sergio>  {
Sergio>    client_state &cs = get_client_state ();
Sergio>    int pid;
Sergio> +  int ret;
 
Sergio>    pid = strtol (own_buf + 8, NULL, 16);
Sergio> -  if (pid != 0 && attach_inferior (pid) == 0)
Sergio> +
Sergio> +  try
Sergio> +    {
Sergio> +      ret = attach_inferior (pid);
Sergio> +    }
Sergio> +  catch (const gdb_exception_error &e)
Sergio> +    {
Sergio> +      sprintf (own_buf, "E.%s", e.what ());

Unrestricted sprintf gives me pause.  Do we know own_buf is large
enough?  Or can/should we truncate the text instead?

Sergio> -gdb_dlopen (const char *filename)
Sergio> +gdb_dlopen (const char *filename, bool dont_throw)

This parameter is only used in one spot, and it's on an error path that
is computing the string to show to the user.  So, I think it's better to
just remove the parameter and use try/catch in that one caller.

Sergio> +static std::string
Sergio> +linux_ptrace_restricted_fail_reason (int err)
Sergio> +{
...
Sergio> +  std::string ret;
Sergio> +  gdb_dlhandle_up handle = gdb_dlopen ("libselinux.so.1", true);
Sergio> +
Sergio> +  if (handle != nullptr)
Sergio> +    {
Sergio> +      selinux_ftype selinux_get_bool
Sergio> + = (selinux_ftype) gdb_dlsym (handle, "security_get_boolean_active");
Sergio> +
Sergio> +      if (selinux_get_bool != NULL
Sergio> +  && (*selinux_get_bool) ("deny_ptrace") == 1)
Sergio> + string_appendf (ret,
Sergio> + _("\n\
Sergio> +The SELinux 'deny_ptrace' option is enabled and preventing GDB\n\
Sergio> +from using 'ptrace'.  You can disable it by executing (as root):\n\
Sergio> +\n\
Sergio> +  setsebool deny_ptrace off\n"));
Sergio> +    }
Sergio> +
Sergio> +  gdb_file_up yama_ptrace_scope
Sergio> +    = gdb_fopen_cloexec ("/proc/sys/kernel/yama/ptrace_scope", "r");
...

If SELinux was the problem, is it also possible that ptrace_scope is the
problem?

I was wondering if each case should just return, or if checking each one
is the correct thing to do here.

thanks,
Tom
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
On Tuesday, September 24 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>
> Sergio> Changes from v3:
>
> Thanks for doing this.  I like this change quite a bit.  For one thing I
> think it would have saved me some time over the years, as I randomly
> forget about ptrace security measures on new machines.

:-)

Thanks for the review.

> Sergio> +++ b/gdb/gdbserver/server.c
> Sergio> @@ -2913,9 +2913,21 @@ handle_v_attach (char *own_buf)
> Sergio>  {
> Sergio>    client_state &cs = get_client_state ();
> Sergio>    int pid;
> Sergio> +  int ret;
>  
> Sergio>    pid = strtol (own_buf + 8, NULL, 16);
> Sergio> -  if (pid != 0 && attach_inferior (pid) == 0)
> Sergio> +
> Sergio> +  try
> Sergio> +    {
> Sergio> +      ret = attach_inferior (pid);
> Sergio> +    }
> Sergio> +  catch (const gdb_exception_error &e)
> Sergio> +    {
> Sergio> +      sprintf (own_buf, "E.%s", e.what ());
>
> Unrestricted sprintf gives me pause.  Do we know own_buf is large
> enough?  Or can/should we truncate the text instead?

I was also worried about this.  I found other cases using sprintf, but
their strings are not as big.  I know own_buf comes from struct
client_state, and its size of PBUFSIZ + 1.  One option would be to use
snprintf with this value.  WDYT?

> Sergio> -gdb_dlopen (const char *filename)
> Sergio> +gdb_dlopen (const char *filename, bool dont_throw)
>
> This parameter is only used in one spot, and it's on an error path that
> is computing the string to show to the user.  So, I think it's better to
> just remove the parameter and use try/catch in that one caller.

OK, I will do that.

> Sergio> +static std::string
> Sergio> +linux_ptrace_restricted_fail_reason (int err)
> Sergio> +{
> ...
> Sergio> +  std::string ret;
> Sergio> +  gdb_dlhandle_up handle = gdb_dlopen ("libselinux.so.1", true);
> Sergio> +
> Sergio> +  if (handle != nullptr)
> Sergio> +    {
> Sergio> +      selinux_ftype selinux_get_bool
> Sergio> + = (selinux_ftype) gdb_dlsym (handle, "security_get_boolean_active");
> Sergio> +
> Sergio> +      if (selinux_get_bool != NULL
> Sergio> +  && (*selinux_get_bool) ("deny_ptrace") == 1)
> Sergio> + string_appendf (ret,
> Sergio> + _("\n\
> Sergio> +The SELinux 'deny_ptrace' option is enabled and preventing GDB\n\
> Sergio> +from using 'ptrace'.  You can disable it by executing (as root):\n\
> Sergio> +\n\
> Sergio> +  setsebool deny_ptrace off\n"));
> Sergio> +    }
> Sergio> +
> Sergio> +  gdb_file_up yama_ptrace_scope
> Sergio> +    = gdb_fopen_cloexec ("/proc/sys/kernel/yama/ptrace_scope", "r");
> ...
>
> If SELinux was the problem, is it also possible that ptrace_scope is the
> problem?

Not at the same time, but if both restrictions are active, we can be
sure that both will have an impact when using GDB.

> I was wondering if each case should just return, or if checking each one
> is the correct thing to do here.

I don't have a strong opinion here, but I think it's more useful for the
user if we print everything wrong in the first pass.  I'm thinking of
cases when starting GDB may take a while, for example: if we tell the
user a list of problems that need to be solved, then she won't need to
keep retrying.

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/
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
>> Unrestricted sprintf gives me pause.  Do we know own_buf is large
>> enough?  Or can/should we truncate the text instead?

Sergio> I was also worried about this.  I found other cases using sprintf, but
Sergio> their strings are not as big.  I know own_buf comes from struct
Sergio> client_state, and its size of PBUFSIZ + 1.  One option would be to use
Sergio> snprintf with this value.  WDYT?

I think that would be fine.

>> I was wondering if each case should just return, or if checking each one
>> is the correct thing to do here.

Sergio> I don't have a strong opinion here, but I think it's more useful for the
Sergio> user if we print everything wrong in the first pass.  I'm thinking of
Sergio> cases when starting GDB may take a while, for example: if we tell the
Sergio> user a list of problems that need to be solved, then she won't need to
Sergio> keep retrying.

Makes sense to me, thanks.

Tom
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Sergio Durigan Junior
Changes from v4:

- Don't add 'dont_throw' optional argument to gdb_dlopen; use
  conventional try..catch.

- Use snprintf when printing to 'own_buf' and make sure we don't
  overflow the buffer.



In Fedora GDB, we carry the following patch:

  https://src.fedoraproject.org/rpms/gdb/blob/8ac06474ff1e2aa4920d14e0666b083eeaca8952/f/gdb-attach-fail-reasons-5of5.patch

Its purpose is to try to detect a specific scenario where SELinux's
'deny_ptrace' option is enabled, which prevents GDB from ptrace'ing in
order to debug the inferior (PTRACE_ATTACH and PTRACE_TRACEME will
fail with EACCES in this case).

I like the idea of improving error detection and providing more
information to the user (a simple "Permission denied" can be really
frustrating), but I don't fully agree with the way the patch was
implemented: it makes GDB link against libselinux only for the sake of
consulting the 'deny_ptrace' setting, and then prints a warning if
ptrace failed and this setting is on.

My first thought (and attempt) was to make GDB print a generic warning
when a ptrace error happened; this message would just point the user
to our documentation, where she could find more information about
possible causes for the error (and try to diagnose/fix the problem).
This proved to be too simple, and I was convinced that it is actually
a good idea to go the extra kilometre and try to pinpoint the specific
problem (or problems) preventing ptrace from working, as well as
provide useful suggestions on how the user can fix things.

Here is the patch I came up with.  It implements a new function,
'linux_ptrace_restricted_fail_reason', which does a few things to
check what's wrong with ptrace:

  - It dlopen's "libselinux.so.1" and checks if the "deny_ptrace"
    option is enabled.

  - It reads the contents of "/proc/sys/kernel/yama/ptrace_scope" and
    checks if it's different than 0.

For each of these checks, if it succeeds, the user will see a message
informing about the restriction in place, and how it can be disabled.
For example, if "deny_ptrace" is enabled, the user will see:

  # gdb /usr/bin/true
  ...
  Starting program: /usr/bin/true
  warning: Could not trace the inferior process.
  warning: ptrace: Permission denied
  The SELinux 'deny_ptrace' option is enabled and preventing GDB
  from using 'ptrace'.  You can disable it by executing (as root):

    setsebool deny_ptrace off

  If you are debugging the inferior remotely, the ptrace restriction(s) need
  to be disabled in the target system (e.g., where GDBserver is running).
  During startup program exited with code 127.
  (gdb)

In case "/proc/sys/kernel/yama/ptrace_scope" is > 0:

  # gdb /usr/bin/true
  ...
  Starting program: /usr/bin/true
  warning: Could not trace the inferior process.
  warning: ptrace: Operation not permitted
  The Linux kernel's Yama ptrace scope is in effect, which can prevent
  GDB from using 'ptrace'.  You can disable it by executing (as root):

    echo 0 > /proc/sys/kernel/yama/ptrace_scope

  If you are debugging the inferior remotely, the ptrace restriction(s) need
  to be disabled in the target system (e.g., where GDBserver is running).
  During startup program exited with code 127.
  (gdb)

If both restrictions are enabled, both messages will show up.

This works for gdbserver as well, and actually fixes a latent bug I
found: when ptrace is restricted, gdbserver would hang due to an
unchecked ptrace call:

  # gdbserver :9988 /usr/bin/true
  gdbserver: linux_ptrace_test_ret_to_nx: Cannot PTRACE_TRACEME: Operation not permitted
  gdbserver: linux_ptrace_test_ret_to_nx: status 256 is not WIFSTOPPED!
  gdbserver: linux_ptrace_test_ret_to_nx: failed to kill child pid 2668100 No such process
  [ Here you would have to issue a C-c ]

Now, you will see:

  # gdbserver :9988 /usr/bin/true
  gdbserver: linux_ptrace_test_ret_to_nx: Cannot PTRACE_TRACEME: Permission denied
  gdbserver: linux_ptrace_test_ret_to_nx: status 256 is not WIFSTOPPED!
  gdbserver: linux_ptrace_test_ret_to_nx: failed to kill child pid 2766868 No such process
  gdbserver: Could not trace the inferior process.
  gdbserver: ptrace: Permission denied
  The SELinux 'deny_ptrace' option is enabled and preventing GDB
  from using 'ptrace'.  You can disable it by executing (as root):

    setsebool deny_ptrace off

  If you are debugging the inferior remotely, the ptrace restriction(s) need
  to be disabled in the target system (e.g., where GDBserver is running).
  #

(I decided to keep all the other messages, even though I find them a
bit distracting).

If GDB can't determine the cause for the failure, it will still print
the generic error message which tells the user to check our
documentation:

  There might be restrictions preventing ptrace from working.  Please see
  the appendix "Linux kernel ptrace restrictions" in the GDB documentation
  for more details.
  If you are debugging the inferior remotely, the ptrace restriction(s) need
  to be disabled in the target system (e.g., where GDBserver is running).

This means that the patch expands our documentation and creates a new
appendix section named "Linux kernel ptrace restrictions", with
sub-sections for each possible restriction that might be in place.

Notice how, on every message, we instruct the user to "do the right
thing" if gdbserver is being used.  This is because if the user
started gdbserver *before* any ptrace restriction was in place, and
then, for some reason, one or more restrictions get enabled, then the
error message will be displayed both on gdbserver *and* on the
connected GDB.  Since the user will be piloting GDB, it's important to
explicitly say that the ptrace restrictions are enabled in the target,
where gdbserver is running.

The current list of possible restrictions is:

  - SELinux's 'deny_ptrace' option (detected).

  - YAMA's /proc/sys/kernel/yama/ptrace_scope setting (detected).

  - seccomp on Docker containers (I couldn't find how to detect).

It's important to mention that all of this is Linux-specific; as far
as I know, SELinux, YAMA and seccomp are Linux-only features.

I tested this patch locally, on my Fedora 30 machine (actually, a
Fedora Rawhide VM), but I'm not proposing a testcase for it because of
the difficulty of writing one.

WDYT?

gdb/doc/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * gdb.texinfo (Linux kernel ptrace restrictions): New appendix
        section.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>
            Jan Kratochvil  <[hidden email]>
            Pedro Alves  <[hidden email]>

        * gdbsupport/gdb-dlfcn.h (gdb_dlopen): Update comment and
        mention that the function throws an error.
        * inf-ptrace.c (default_inf_ptrace_me_fail_reason): New
        function.
        (inf_ptrace_me_fail_reason): New variable.
        (inf_ptrace_me): Update call to 'trace_start_error_with_name'.
        * inf-ptrace.h (inf_ptrace_me_fail_reason): New variable.
        * linux-nat.c (attach_proc_task_lwp_callback): Call
        'linux_ptrace_attach_fail_reason_lwp'.
        (linux_nat_target::attach): Update call to
        'linux_ptrace_attach_fail_reason'.
        (_initialize_linux_nat): Set 'inf_ptrace_me_fail_reason'.
        * nat/fork-inferior.c (trace_start_error_with_name): Add
        optional 'append' argument.
        * nat/fork-inferior.h (trace_start_error_with_name): Update
        prototype.
        * nat/linux-ptrace.c: Include "gdbsupport/gdb-dlfcn.h",
        "gdbsupport/filestuff.h" and "nat/fork-inferior.h".
        (selinux_ftype): New typedef.
        (linux_ptrace_restricted_fail_reason): New function.
        (linux_ptrace_attach_fail_reason_1): New function.
        (linux_ptrace_attach_fail_reason): Change first argument type
        from 'ptid_t' to 'pid_t'.  Call
        'linux_ptrace_attach_fail_reason_1' and
        'linux_ptrace_restricted_fail_reason'.
        (linux_ptrace_attach_fail_reason_lwp): New function.
        (linux_ptrace_me_fail_reason): New function.
        (errno_pipe): New variable.
        (linux_fork_to_function): Initialize pipe before forking.
        (linux_child_function): Deal with errno-passing from child.
        Handle ptrace error.
        (linux_check_child_ptrace_errno): New function.
        (linux_check_child_ptrace_errno): Call
        'linux_check_child_ptrace_errno'.
        * nat/linux-ptrace.h (linux_ptrace_attach_fail_reason): Update
        prototype.
        (linux_ptrace_attach_fail_reason_lwp): New prototype.
        (linux_ptrace_me_fail_reason): New prototype.
        * remote.c (extended_remote_target::attach): Handle error
        message passed by the server when attach fails.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>
            Pedro Alves  <[hidden email]>

        * linux-low.c (linux_ptrace_fun): Call
        'linux_ptrace_me_fail_reason'.
        (attach_proc_task_lwp_callback): Call
        'linux_ptrace_attach_fail_reason_lwp'.
        (linux_attach): Call 'linux_ptrace_attach_fail_reason'.
        * server.c (handle_v_attach): Use try..catch when calling
        'attach_inferior', and send an error message to the client
        when needed.
        * thread-db.c (attach_thread): Call
        'linux_ptrace_attach_fail_reason_lwp'.
---
 gdb/doc/gdb.texinfo        | 143 +++++++++++++++++++++++++++
 gdb/gdbserver/linux-low.c  |   9 +-
 gdb/gdbserver/server.c     |  14 ++-
 gdb/gdbserver/thread-db.c  |   2 +-
 gdb/gdbsupport/gdb-dlfcn.h |   4 +-
 gdb/inf-ptrace.c           |  17 +++-
 gdb/inf-ptrace.h           |  10 ++
 gdb/linux-nat.c            |   9 +-
 gdb/nat/fork-inferior.c    |   4 +-
 gdb/nat/fork-inferior.h    |   7 +-
 gdb/nat/linux-ptrace.c     | 192 +++++++++++++++++++++++++++++++++++--
 gdb/nat/linux-ptrace.h     |  27 ++++--
 gdb/remote.c               |  16 +++-
 13 files changed, 423 insertions(+), 31 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f2713c0396..e7b5b18f2b 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -182,6 +182,9 @@ software in general.  We will miss him.
                                 @value{GDBN}
 * Operating System Information:: Getting additional information from
                                  the operating system
+* Linux kernel ptrace restrictions::        Restrictions sometimes
+                                            imposed by the Linux
+                                            kernel on @code{ptrace}
 * Trace File Format:: GDB trace file format
 * Index Section Format::        .gdb_index section format
 * Man Pages:: Manual pages
@@ -44665,6 +44668,146 @@ should contain a comma-separated list of cores that this process
 is running on.  Target may provide additional columns,
 which @value{GDBN} currently ignores.
 
+@node Linux kernel ptrace restrictions
+@appendix Linux kernel @code{ptrace} restrictions
+@cindex linux kernel ptrace restrictions, attach
+
+The @code{ptrace} system call is used by @value{GDBN} and
+@code{gdbserver} on GNU/Linux to, among other things, attach to a new
+or existing inferior in order to start debugging it.  Due to security
+concerns, some distributions and vendors disable or severely restrict
+the ability to perform these operations, which can make @value{GDBN}
+or @code{gdbserver} malfunction.  In this section, we will expand on
+how this malfunction can manifest itself, and how to modify the
+system's settings in order to be able to use @value{GDBN} and
+@code{gdbserver} properly.
+
+@menu
+* The error message::                   The error message displayed when the
+                                        system prevents @value{GDBN}
+                                        or @code{gdbserver} from using
+                                        @code{ptrace}
+* SELinux's deny_ptrace::               SELinux and the @code{deny_ptrace} option
+* Yama's ptrace_scope::                 Yama and the @code{ptrace_scope} setting
+* Docker and seccomp::                  Docker and the @code{seccomp}
+                                        infrastructure
+@end menu
+
+@node The error message
+@appendixsection The error message
+
+When the system prevents @value{GDBN} or @code{gdbserver} from using
+the @code{ptrace} system call, you will likely see a descriptive error
+message explaining what is wrong and how to attempt to fix the
+problem.  For example, when SELinux's @code{deny_ptrace} option is
+enabled, you can see:
+
+@smallexample
+$ gdb program
+...
+(@value{GDBP}) run
+Starting program: program
+warning: Could not trace the inferior process.
+Error:
+warning: ptrace: Permission denied
+The SELinux 'deny_ptrace' option is enabled and preventing @value{GDBN}
+from using 'ptrace'.  You can disable it by executing (as root):
+
+  setsebool deny_ptrace off
+
+If you are debugging the inferior remotely, the instruction(s) above must
+be performed in the target system (e.g., where GDBserver is running).
+During startup program exited with code 127.
+(@value{GDBP})
+@end smallexample
+
+Sometimes, it may not be possible to acquire the necessary data to
+determine the root cause of the failure.  In this case, you will see a
+generic error message pointing you to this section:
+
+@smallexample
+$ gdb program
+...
+Starting program: program
+warning: Could not trace the inferior process.
+Error:
+warning: ptrace: Permission denied
+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
+
+@node SELinux's deny_ptrace
+@appendixsection SELinux's @code{deny_ptrace}
+@cindex SELinux
+@cindex deny_ptrace
+
+If you are using SELinux, you might want to check whether the
+@code{deny_ptrace} option is enabled by doing:
+
+@smallexample
+$ getsebool deny_ptrace
+deny_ptrace --> on
+@end smallexample
+
+If the option is enabled, you can disable it by doing, as root:
+
+@smallexample
+# setsebool deny_ptrace off
+@end smallexample
+
+The option will be disabled until the next reboot.  If you would like
+to disable it permanently, you can do (as root):
+
+@smallexample
+# setsebool -P deny_ptrace off
+@end smallexample
+
+@node Yama's ptrace_scope
+@appendixsection Yama's @code{ptrace_scope}
+@cindex yama, ptrace_scope
+
+If your system has Yama enabled, you might want to check whether the
+@code{ptrace_scope} setting is enabled by checking the value of
+@file{/proc/sys/kernel/yama/ptrace_scope}:
+
+@smallexample
+$ cat /proc/sys/kernel/yama/ptrace_scope
+0
+@end smallexample
+
+If you see anything other than @code{0}, @value{GDBN} or
+@code{gdbserver} can be affected by it.  You can temporarily disable
+the feature by doing, as root:
+
+@smallexample
+# sysctl kernel.yama.ptrace_scope=0
+kernel.yama.ptrace_scope = 0
+@end smallexample
+
+You can make this permanent by doing, as root:
+
+@smallexample
+# sysctl -w kernel.yama.ptrace_scope=0
+kernel.yama.ptrace_scope = 0
+@end smallexample
+
+@node Docker and seccomp
+@appendixsection Docker and @code{seccomp}
+@cindex docker, seccomp
+
+If you are using Docker (@uref{https://www.docker.com/}) containers,
+you will probably have to disable its @code{seccomp} protections in
+order to be able to use @value{GDBN} or @code{gdbserver}.  To do that,
+you can use the options @code{--cap-add=SYS_PTRACE --security-opt
+seccomp=unconfined} when invoking Docker:
+
+@smallexample
+$ docker run --cap-add=SYS_PTRACE --security-opt seccomp=unconfined
+@end smallexample
+
 @node Trace File Format
 @appendix Trace File Format
 @cindex trace file format
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 ());
 
   if (setpgid (0, 0) < 0)
     trace_start_error_with_name ("setpgid");
@@ -1165,7 +1166,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 ());
  }
@@ -1197,8 +1198,8 @@ linux_attach (unsigned long pid)
     {
       remove_process (proc);
 
-      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
-      error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
+      std::string reason = linux_ptrace_attach_fail_reason (pid, err);
+      error (_("Cannot attach to process %ld: %s"), pid, reason.c_str ());
     }
 
   /* Don't ignore the initial SIGSTOP if we just attached to this
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 67e8e3e54d..976ecbd2df 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2893,9 +2893,21 @@ handle_v_attach (char *own_buf)
 {
   client_state &cs = get_client_state ();
   int pid;
+  int ret;
 
   pid = strtol (own_buf + 8, NULL, 16);
-  if (pid != 0 && attach_inferior (pid) == 0)
+
+  try
+    {
+      ret = attach_inferior (pid);
+    }
+  catch (const gdb_exception_error &e)
+    {
+      snprintf (own_buf, PBUFSIZ, "E.%s", e.what ());
+      return 0;
+    }
+
+  if (pid != 0 && ret == 0)
     {
       /* Don't report shared library events after attaching, even if
  some libraries are preloaded.  GDB will always poll the
diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c
index c6b43a06cc..e3acf83850 100644
--- a/gdb/gdbserver/thread-db.c
+++ b/gdb/gdbserver/thread-db.c
@@ -224,7 +224,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/gdbsupport/gdb-dlfcn.h b/gdb/gdbsupport/gdb-dlfcn.h
index 6a39d38941..e933b7a473 100644
--- a/gdb/gdbsupport/gdb-dlfcn.h
+++ b/gdb/gdbsupport/gdb-dlfcn.h
@@ -32,8 +32,8 @@ struct dlclose_deleter
 typedef std::unique_ptr<void, dlclose_deleter> gdb_dlhandle_up;
 
 /* Load the dynamic library file named FILENAME, and return a handle
-   for that dynamic library.  Return NULL if the loading fails for any
-   reason.  */
+   for that dynamic library.  Throw an error if the loading fails for
+   any reason.  */
 
 gdb_dlhandle_up gdb_dlopen (const char *filename);
 
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 4a8e732373..b792af00d1 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -94,6 +94,20 @@ inf_ptrace_target::remove_fork_catchpoint (int pid)
 #endif /* PT_GET_PROCESS_STATE */
 
 
+/* Default method for "inf_ptrace_me_fail_reason", which returns an
+   empty string.  */
+
+static std::string
+default_inf_ptrace_me_fail_reason (int err)
+{
+  return {};
+}
+
+/* See inf-ptrace.h.  */
+
+std::string (*inf_ptrace_me_fail_reason) (int err)
+  = default_inf_ptrace_me_fail_reason;
+
 /* 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 ());
 }
 
 /* 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", 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;
 }
 
 
diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 355e9bef43..2ead4a4858 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -591,7 +591,7 @@ trace_start_error (const char *fmt, ...)
 /* See nat/fork-inferior.h.  */
 
 void
-trace_start_error_with_name (const char *string)
+trace_start_error_with_name (const char *string, const char *append)
 {
-  trace_start_error ("%s: %s", string, safe_strerror (errno));
+  trace_start_error ("%s: %s%s", string, safe_strerror (errno), append);
 }
diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
index 065496c382..405f2cf548 100644
--- a/gdb/nat/fork-inferior.h
+++ b/gdb/nat/fork-inferior.h
@@ -98,9 +98,10 @@ extern void trace_start_error (const char *fmt, ...)
   ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 
 /* Like "trace_start_error", but the error message is constructed by
-   combining STRING with the system error message for errno.  This
-   function does not return.  */
-extern void trace_start_error_with_name (const char *string)
+   combining STRING with the system error message for errno, and
+   (optionally) with APPEND.  This function does not return.  */
+extern void trace_start_error_with_name (const char *string,
+ const char *append = "")
   ATTRIBUTE_NORETURN;
 
 #endif /* NAT_FORK_INFERIOR_H */
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index c1ebc0a032..8a048d2ec9 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -21,6 +21,9 @@
 #include "linux-procfs.h"
 #include "linux-waitpid.h"
 #include "gdbsupport/buffer.h"
+#include "gdbsupport/gdb-dlfcn.h"
+#include "nat/fork-inferior.h"
+#include "gdbsupport/filestuff.h"
 #ifdef HAVE_SYS_PROCFS_H
 #include <sys/procfs.h>
 #endif
@@ -30,11 +33,94 @@
    of 0 means there are no supported features.  */
 static int supported_ptrace_options = -1;
 
-/* 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.  */
+typedef int (*selinux_ftype) (const char *);
 
-std::string
-linux_ptrace_attach_fail_reason (pid_t pid)
+/* Helper function which checks if ptrace is probably restricted
+   (i.e., if ERR is either EACCES or EPERM), and returns a string with
+   possible workarounds.  */
+
+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);
+    }
+
+  if (handle != nullptr)
+    {
+      selinux_ftype selinux_get_bool
+ = (selinux_ftype) gdb_dlsym (handle, "security_get_boolean_active");
+
+      if (selinux_get_bool != NULL
+  && (*selinux_get_bool) ("deny_ptrace") == 1)
+ string_appendf (ret,
+ _("\n\
+The SELinux 'deny_ptrace' option is enabled and preventing GDB\n\
+from using 'ptrace'.  You can disable it by executing (as root):\n\
+\n\
+  setsebool deny_ptrace off\n"));
+    }
+
+  gdb_file_up yama_ptrace_scope
+    = gdb_fopen_cloexec ("/proc/sys/kernel/yama/ptrace_scope", "r");
+
+  if (yama_ptrace_scope != nullptr)
+    {
+      char yama_scope = fgetc (yama_ptrace_scope.get ());
+
+      if (yama_scope != '0')
+ string_appendf (ret,
+ _("\n\
+The Linux kernel's Yama ptrace scope is in effect, which can prevent\n\
+GDB from using 'ptrace'.  You can disable it by executing (as root):\n\
+\n\
+  echo 0 > /proc/sys/kernel/yama/ptrace_scope\n"));
+    }
+
+  if (ret.empty ())
+    {
+      /* It wasn't possible to determine the exact reason for the
+ ptrace error.  Let's just emit a generic error message
+ pointing the user to our documentation, where she can find
+ instructions on how to try to diagnose the problem.  */
+      ret = _("\n\
+There might be restrictions preventing ptrace from working.  Please see\n\
+the appendix \"Linux kernel ptrace restrictions\" in the GDB documentation\n\
+for more details.");
+    }
+
+  /* The user may be debugging remotely, so we have to warn that
+     the instructions above should be performed in the target.  */
+  string_appendf (ret,
+  _("\n\
+If you are debugging the inferior remotely, the ptrace restriction(s) must\n\
+be disabled in the target system (e.g., where GDBserver is running)."));
+
+  return ret;
+}
+
+/* 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.  */
+
+static std::string
+linux_ptrace_attach_fail_reason_1 (pid_t pid)
 {
   pid_t tracerpid = linux_proc_get_tracerpid_nowarn (pid);
   std::string result;
@@ -56,10 +142,24 @@ linux_ptrace_attach_fail_reason (pid_t pid)
 /* See linux-ptrace.h.  */
 
 std::string
-linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err)
+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;
+
+  return result;
+}
+
+/* See linux-ptrace.h.  */
+
+std::string
+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,
@@ -68,6 +168,14 @@ linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err)
     return string_printf ("%s (%d)", safe_strerror (err), err);
 }
 
+/* See linux-ptrace.h.  */
+
+std::string
+linux_ptrace_me_fail_reason (int err)
+{
+  return linux_ptrace_restricted_fail_reason (err);
+}
+
 #if defined __i386__ || defined __x86_64__
 
 /* Address of the 'ret' instruction in asm code block below.  */
@@ -257,6 +365,12 @@ linux_ptrace_test_ret_to_nx (void)
 #endif /* defined __i386__ || defined __x86_64__ */
 }
 
+/* If the PTRACE_TRACEME call on linux_child_function errors, we need
+   to be able to send ERRNO back to the parent so that it can check
+   whether there are restrictions in place preventing ptrace from
+   working.  We do that with a pipe.  */
+static int errno_pipe[2];
+
 /* Helper function to fork a process and make the child process call
    the function FUNCTION, passing CHILD_STACK as parameter.
 
@@ -273,6 +387,11 @@ linux_fork_to_function (gdb_byte *child_stack, int (*function) (void *))
   /* Sanity check the function pointer.  */
   gdb_assert (function != NULL);
 
+  /* Create the pipe that will be used by the child to pass ERRNO
+     after the PTRACE_TRACEME call.  */
+  if (pipe (errno_pipe) != 0)
+    trace_start_error_with_name ("pipe");
+
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
 #define STACK_SIZE 4096
 
@@ -321,7 +440,21 @@ linux_grandchild_function (void *child_stack)
 static int
 linux_child_function (void *child_stack)
 {
-  ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0);
+  /* Close read end.  */
+  close (errno_pipe[0]);
+
+  int ret = ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
+    (PTRACE_TYPE_ARG4) 0);
+  int ptrace_errno = errno;
+
+  /* Write ERRNO to the pipe, even if it's zero, and close the writing
+     end of the pipe.  */
+  write (errno_pipe[1], &ptrace_errno, sizeof (ptrace_errno));
+  close (errno_pipe[1]);
+
+  if (ret != 0)
+    _exit (0);
+
   kill (getpid (), SIGSTOP);
 
   /* Fork a grandchild.  */
@@ -336,6 +469,48 @@ static void linux_test_for_tracesysgood (int child_pid);
 static void linux_test_for_tracefork (int child_pid);
 static void linux_test_for_exitkill (int child_pid);
 
+/* Helper function to wait for the child to send us the ptrace ERRNO,
+   and check if it's OK.  */
+
+static void
+linux_check_child_ptrace_errno ()
+{
+  int child_errno;
+  fd_set rset;
+  struct timeval timeout;
+
+  /* Close the writing end of the pipe.  */
+  close (errno_pipe[1]);
+
+  FD_ZERO (&rset);
+  FD_SET (errno_pipe[0], &rset);
+
+  /* One second should be plenty of time to wait for the child's
+     reply.  */
+  timeout.tv_sec = 1;
+  timeout.tv_usec = 0;
+
+  int ret = select (errno_pipe[0] + 1, &rset, NULL, NULL, &timeout);
+
+  if (ret < 0)
+    trace_start_error_with_name ("select");
+  else if (ret == 0)
+    error (_("Timeout while waiting for child's ptrace errno"));
+  else
+    read (errno_pipe[0], &child_errno, sizeof (child_errno));
+
+  if (child_errno != 0)
+    {
+      /* The child can't use PTRACE_TRACEME.  We just bail out.  */
+      std::string reason = linux_ptrace_restricted_fail_reason (child_errno);
+
+      errno = child_errno;
+      trace_start_error_with_name ("ptrace", reason.c_str ());
+    }
+
+  close (errno_pipe[0]);
+}
+
 /* Determine ptrace features available on this target.  */
 
 void
@@ -352,6 +527,9 @@ linux_check_ptrace_features (void)
      reporting.  */
   child_pid = linux_fork_to_function (NULL, linux_child_function);
 
+  /* Check if the child can successfully use ptrace.  */
+  linux_check_child_ptrace_errno ();
+
   ret = my_waitpid (child_pid, &status, 0);
   if (ret == -1)
     perror_with_name (("waitpid"));
diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index fd2f12a342..90afb60f34 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -176,12 +176,27 @@ struct buffer;
 # define TRAP_HWBKPT 4
 #endif
 
-extern std::string linux_ptrace_attach_fail_reason (pid_t pid);
-
-/* 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);
+/* 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);
+
+/* Find all possible reasons we could have failed to attach to PID's
+   LWPID and return them as a string.  ERR is the error PTRACE_ATTACH
+   failed 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 pid, 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
+   obtain the reason why ptrace failed.  ERR should be the ERRNO value
+   returned by ptrace.
+
+   This function will return a 'std::string' containing the fail
+   reason, or an empty string otherwise.  */
+extern std::string linux_ptrace_me_fail_reason (int err);
 
 extern void linux_ptrace_init_warnings (void);
 extern void linux_check_ptrace_features (void);
diff --git a/gdb/remote.c b/gdb/remote.c
index 21160e13ac..efc5084cfe 100644
--- 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);
+  }
+
+ 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));
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Tom Tromey-2
On Wednesday, September 25 2019, Tom Tromey wrote:

>>> Unrestricted sprintf gives me pause.  Do we know own_buf is large
>>> enough?  Or can/should we truncate the text instead?
>
> Sergio> I was also worried about this.  I found other cases using sprintf, but
> Sergio> their strings are not as big.  I know own_buf comes from struct
> Sergio> client_state, and its size of PBUFSIZ + 1.  One option would be to use
> Sergio> snprintf with this value.  WDYT?
>
> I think that would be fine.
>
>>> I was wondering if each case should just return, or if checking each one
>>> is the correct thing to do here.
>
> Sergio> I don't have a strong opinion here, but I think it's more useful for the
> Sergio> user if we print everything wrong in the first pass.  I'm thinking of
> Sergio> cases when starting GDB may take a while, for example: if we tell the
> Sergio> user a list of problems that need to be solved, then she won't need to
> Sergio> keep retrying.
>
> Makes sense to me, thanks.

Thanks; I've just sent v5 now.

Cheers,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Sergio Durigan Junior
>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:

Sergio> Changes from v4:
Sergio> - Don't add 'dont_throw' optional argument to gdb_dlopen; use
Sergio>   conventional try..catch.

Sergio> - Use snprintf when printing to 'own_buf' and make sure we don't
Sergio>   overflow the buffer.

Thank you for doing this.
I think this is ok, please check it in.

Tom
Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
On 9/26/19 6:32 PM, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>
> Sergio> Changes from v4:
> Sergio> - Don't add 'dont_throw' optional argument to gdb_dlopen; use
> Sergio>   conventional try..catch.
>
> Sergio> - Use snprintf when printing to 'own_buf' and make sure we don't
> Sergio>   overflow the buffer.
>
> Thank you for doing this.
> I think this is ok, please check it in.

Please give me a bit to review before merging.  I think I spotted an issue,
but I haven't had a chance to take a look at it since before Cauldron.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Tom Tromey-2
On Thursday, September 26 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>
> Sergio> Changes from v4:
> Sergio> - Don't add 'dont_throw' optional argument to gdb_dlopen; use
> Sergio>   conventional try..catch.
>
> Sergio> - Use snprintf when printing to 'own_buf' and make sure we don't
> Sergio>   overflow the buffer.
>
> Thank you for doing this.
> I think this is ok, please check it in.

Thanks for the extensive reviews, Tom and Pedro.

Pushed: 381beca6146ac68b57edf47d28cdb335fbd11635

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Pedro Alves-7
On Thursday, September 26 2019, Pedro Alves wrote:

> On 9/26/19 6:32 PM, Tom Tromey wrote:
>>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>>
>> Sergio> Changes from v4:
>> Sergio> - Don't add 'dont_throw' optional argument to gdb_dlopen; use
>> Sergio>   conventional try..catch.
>>
>> Sergio> - Use snprintf when printing to 'own_buf' and make sure we don't
>> Sergio>   overflow the buffer.
>>
>> Thank you for doing this.
>> I think this is ok, please check it in.
>
> Please give me a bit to review before merging.  I think I spotted an issue,
> but I haven't had a chance to take a look at it since before Cauldron.

Ops, I just saw this message now, after pushing the patch.  Would you
like me to revert it?

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
In reply to this post by Sergio Durigan Junior
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;
>  }
>  
>  

> +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
Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
In reply to this post by Sergio Durigan Junior
On 9/26/19 6:51 PM, Sergio Durigan Junior wrote:
> On Thursday, September 26 2019, Pedro Alves wrote:

>> Please give me a bit to review before merging.  I think I spotted an issue,
>> but I haven't had a chance to take a look at it since before Cauldron.
>
> Ops, I just saw this message now, after pushing the patch.  Would you
> like me to revert it?

Yes, since it just went in, I think it would be better to just revert
it, and work on a new full patch.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Pedro Alves-7
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/
Reply | Threaded
Open this post in threaded view
|

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

Sergio Durigan Junior
In reply to this post by Pedro Alves-7
On Thursday, September 26 2019, Pedro Alves wrote:

> On 9/26/19 6:51 PM, Sergio Durigan Junior wrote:
>> On Thursday, September 26 2019, Pedro Alves wrote:
>
>>> Please give me a bit to review before merging.  I think I spotted an issue,
>>> but I haven't had a chance to take a look at it since before Cauldron.
>>
>> Ops, I just saw this message now, after pushing the patch.  Would you
>> like me to revert it?
>
> Yes, since it just went in, I think it would be better to just revert
> it, and work on a new full patch.

OK, done.

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/
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/6] Improve ptrace-error detection

Sergio Durigan Junior
In reply to this post by Sergio Durigan Junior
This patch series is the continuation of:

  [PATCH v5] Improve ptrace-error detection on Linux targets
  https://sourceware.org/ml/gdb-patches/2019-09/msg00504.html

I decided to start a new series because this involved a rewrite of
several parts of the patch.  I addressed all of the comments I
received back in September (mostly from Pedro), but I also did some
improvements, especially in the 'fork_inferior' code.

After submitting this series to our Buildbot, no regressions were
found.

Sergio Durigan Junior (6):
  Introduce scoped_pipe.h
  Don't reset errno/bfd_error on 'throw_perror_with_name'
  Expand 'fork_inferior' to check whether 'traceme_fun' succeeded
  Extend GNU/Linux to check for ptrace error
  Document Linux-specific possible ptrace restrictions
  Fix comment for 'gdb_dlopen'

 gdb/Makefile.in                       |   1 +
 gdb/doc/gdb.texinfo                   | 143 ++++++++++++++++
 gdb/inf-ptrace.c                      |  18 +-
 gdb/linux-nat.c                       |  10 +-
 gdb/nat/fork-inferior.c               | 231 ++++++++++++++++++++++++--
 gdb/nat/fork-inferior.h               |  87 ++++++++--
 gdb/nat/linux-ptrace.c                | 178 +++++++++++++++++++-
 gdb/nat/linux-ptrace.h                |  27 ++-
 gdb/remote.c                          |  40 ++++-
 gdb/unittests/scoped_pipe-selftests.c |  96 +++++++++++
 gdb/utils.c                           |   6 -
 gdbserver/linux-low.cc                |  31 +++-
 gdbserver/server.cc                   |  38 ++++-
 gdbserver/thread-db.cc                |   2 +-
 gdbserver/utils.cc                    |   2 +
 gdbsupport/gdb-dlfcn.h                |   4 +-
 gdbsupport/scoped_pipe.h              |  63 +++++++
 17 files changed, 909 insertions(+), 68 deletions(-)
 create mode 100644 gdb/unittests/scoped_pipe-selftests.c
 create mode 100644 gdbsupport/scoped_pipe.h

--
2.24.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/6] Introduce scoped_pipe.h

Sergio Durigan Junior
This simple patch introduces gdbsupport/scoped_pipe.h, which is based
on gdbsupport/scoped_fd.h.  When the object is instantiated, a pipe is
created using 'gdb_pipe_cloexec'.  There are two methods (get_read_end
and get_write_end) that allow the user to obtain the read/write ends
of the pipe (no more messing with [0] and [1]), and when the object is
destroyed, the pipe is closed (both ends).

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * unittests/scoped_pipe-selftests.c: New file.

gdbsupport/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * scoped_pipe.h: New file.
---
 gdb/Makefile.in                       |  1 +
 gdb/unittests/scoped_pipe-selftests.c | 96 +++++++++++++++++++++++++++
 gdbsupport/scoped_pipe.h              | 63 ++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 gdb/unittests/scoped_pipe-selftests.c
 create mode 100644 gdbsupport/scoped_pipe.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index f9606b8fc7..d5f1450035 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -444,6 +444,7 @@ SUBDIR_UNITTESTS_SRCS = \
  unittests/rsp-low-selftests.c \
  unittests/scoped_fd-selftests.c \
  unittests/scoped_mmap-selftests.c \
+ unittests/scoped_pipe-selftests.c \
  unittests/scoped_restore-selftests.c \
  unittests/string_view-selftests.c \
  unittests/style-selftests.c \
diff --git a/gdb/unittests/scoped_pipe-selftests.c b/gdb/unittests/scoped_pipe-selftests.c
new file mode 100644
index 0000000000..dd634bec97
--- /dev/null
+++ b/gdb/unittests/scoped_pipe-selftests.c
@@ -0,0 +1,96 @@
+/* Self tests for scoped_pipe for GDB, the GNU debugger.
+
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+
+#include "gdbsupport/scoped_pipe.h"
+#include "config.h"
+#include "gdbsupport/selftest.h"
+
+namespace selftests {
+namespace scoped_pipe {
+
+/* Test that the pipe is correctly created.  */
+
+static void
+test_create ()
+{
+  ::scoped_pipe spipe;
+
+  SELF_CHECK (spipe.get_read_end () > 0);
+  SELF_CHECK (spipe.get_write_end () > 0);
+}
+
+/* Test that we can write and read from the pipe.  */
+
+static void
+test_transmission ()
+{
+  int foo = 123;
+  ::scoped_pipe spipe;
+
+  /* Write to the pipe.  */
+  {
+    ssize_t writeret;
+
+    do
+      {
+ writeret = write (spipe.get_write_end (), &foo, sizeof (foo));
+      }
+    while (writeret < 0 && (errno == EAGAIN || errno == EINTR));
+
+    SELF_CHECK (writeret > 0);
+  }
+
+  /* Read from the pipe, and check if the value read is the same as
+     the one that was written.  */
+  {
+    ssize_t readret;
+    int read_foo;
+
+    do
+      {
+ readret = read (spipe.get_read_end (), &read_foo, sizeof (read_foo));
+      }
+    while (readret < 0 && (errno == EAGAIN || errno == EINTR));
+
+    SELF_CHECK (readret > 0);
+
+    SELF_CHECK (read_foo == foo);
+  }
+}
+
+/* Run selftests.  */
+static void
+run_tests ()
+{
+  test_create ();
+  test_transmission ();
+}
+
+} /* namespace scoped_pipe */
+} /* namespace selftests */
+
+void _initialize_scoped_pipe_selftests ();
+void
+_initialize_scoped_pipe_selftests ()
+{
+  selftests::register_test ("scoped_pipe",
+    selftests::scoped_pipe::run_tests);
+}
diff --git a/gdbsupport/scoped_pipe.h b/gdbsupport/scoped_pipe.h
new file mode 100644
index 0000000000..8f133b442f
--- /dev/null
+++ b/gdbsupport/scoped_pipe.h
@@ -0,0 +1,63 @@
+/* scoped_pipe, automatically close a pipe.
+
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_SCOPED_PIPE_H
+#define COMMON_SCOPED_PIPE_H
+
+#include <unistd.h>
+#include "filestuff.h"
+
+/* A smart-pointer-like class to automatically close a pipe.  */
+
+class scoped_pipe
+{
+public:
+  explicit scoped_pipe ()
+  {
+    if (gdb_pipe_cloexec (m_pipe) < 0)
+      error (_("gdb_pipe_cloexec: %s"), safe_strerror (errno));
+  }
+
+  ~scoped_pipe ()
+  {
+    if (m_pipe[0] >= 0)
+      close (m_pipe[0]);
+    if (m_pipe[1] >= 0)
+      close (m_pipe[1]);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (scoped_pipe);
+
+  /* Get the read end of the pipe.  */
+  int get_read_end () const noexcept
+  {
+    return m_pipe[0];
+  }
+
+  /* Get the write end of the pipe.  */
+  int get_write_end () const noexcept
+  {
+    return m_pipe[1];
+  }
+
+private:
+  int m_pipe[2];
+};
+
+#endif /* ! COMMON_SCOPED_PIPE_H */
--
2.24.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

Sergio Durigan Junior
In reply to this post by Sergio Durigan Junior
Since this hunk may be a bit controversial, I decided to split it into
a separate patch.  This is going to be needed by the ptrace-error
feature; GDB will need to be able to access the value of errno even
after a call to our 'perror'-like functions.

The idea here is that we actually *want* errno to propagate between
our customized 'perror' calls.  We currently have this code on
'throw_perror_with_name':

  /* I understand setting these is a matter of taste.  Still, some people
     may clear errno but not know about bfd_error.  Doing this here is not
     unreasonable.  */
  bfd_set_error (bfd_error_no_error);
  errno = 0;

git blame tells me that this piece of code is pretty old; the commit
that "introduced" it is:

  commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc
  Author: Stan Shebs <[hidden email]>
  Date:   Fri Apr 16 01:35:26 1999 +0000

      Initial creation of sourceware repository

so yeah...

If we go to the POSIX specification for 'perror', it doesn't really
say anything about whether errno should be preserved or not.  It does,
however, say that 'perror's messages should be the same as those
returned by 'strerror', and 'strerror' is not supposed to alter errno
if the call is successful.

Maybe when our wrapper was written it was OK to modify errno, I don't
know.  But I'd like to propose that we stick to POSIX in this case.

Another small hunk is the one that saves/restores errno on gdbserver's
'perror_with_name', but this one is pretty trivial, I think.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * utils.c (throw_perror_with_name): Don't reset
        errno/bfd_error.

gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * utils.cc (perror_with_name): Save/restore errno.
---
 gdb/utils.c        | 6 ------
 gdbserver/utils.cc | 2 ++
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 0b470120a2..df8add1afa 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -595,12 +595,6 @@ throw_perror_with_name (enum errors errcode, const char *string)
 {
   std::string combined = perror_string (string);
 
-  /* I understand setting these is a matter of taste.  Still, some people
-     may clear errno but not know about bfd_error.  Doing this here is not
-     unreasonable.  */
-  bfd_set_error (bfd_error_no_error);
-  errno = 0;
-
   throw_error (errcode, _("%s."), combined.c_str ());
 }
 
diff --git a/gdbserver/utils.cc b/gdbserver/utils.cc
index d88f4ac5ca..50ebba43ca 100644
--- a/gdbserver/utils.cc
+++ b/gdbserver/utils.cc
@@ -46,6 +46,7 @@ perror_with_name (const char *string)
 {
   const char *err;
   char *combined;
+  int saved_errno = errno;
 
   err = safe_strerror (errno);
   if (err == NULL)
@@ -56,6 +57,7 @@ perror_with_name (const char *string)
   strcat (combined, ": ");
   strcat (combined, err);
 
+  errno = saved_errno;
   error ("%s.", combined);
 }
 
--
2.24.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/6] Expand 'fork_inferior' to check whether 'traceme_fun' succeeded

Sergio Durigan Junior
In reply to this post by Sergio Durigan Junior
This patch is one important piece of the series.  It expands
'fork_inferior' in order to deal with new steps in the process of
initializing the inferior.  We now have to:

- Create a pipe that will be used to communicate with our
  fork (pre-exec), and which the fork will use to pass back to us the
  errno value of the 'traceme_fun' call.

- Close this pipe after it is used.

- Check the errno value passed back from the fork, and report any
  problems in the initialization to the user.

I thought about and implemented a few designs for all of this, but
ended up sticking with the function overload one.  'fork_inferior' is
now two functions: one that will take a traceme function like
'(*traceme_fun) ()' --- i.e., the original 'fork_inferior' behaviour,
and other that will take a function like '(*traceme_fun) (int
trace_pipe_write)'.  Depending on which function it takes, we know
whether the user does not want us to check whether the 'traceme_fun'
call was successful (former) or if she does (latter).

All in all, the patch is not complicated to understand and keeps the
interface clean enough so that we don't have to update every caller of
'fork_inferior' (which was a problem with previous designs I tried).

The subsequent patch will build on top of this one and implement the
errno-passing-via-pipe on the GNU/Linux target.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

         * nat/fork-inferior.c: Include "gdbsupport/scoped_pipe.h".
         (default_trace_me_fail_reason): New function.
         (trace_me_fail_reason): New variable.
         (write_trace_errno_to_pipe): New function.
         (read_trace_errno_from_pipe): Likewise.
         (check_child_trace_me_errno): Likewise.
         (traceme_info): New struct.
         (fork_inferior_1): Renamed from 'fork_inferior'.
         (fork_inferior): New overloads.
         (trace_start_error_with_name): Add "append" parameter.
         * nat/fork-inferior.h (fork_inferior): Expand comment.
         Add overload declaration.
         (trace_start_error_with_name): Add "append" parameter.
         (trace_me_fail_reason): New variable.
         (check_child_trace_me_errno): New function.
         (write_trace_errno_to_pipe): Likewise.
---
 gdb/nat/fork-inferior.c | 231 ++++++++++++++++++++++++++++++++++++----
 gdb/nat/fork-inferior.h |  87 ++++++++++++---
 2 files changed, 288 insertions(+), 30 deletions(-)

diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 1185ef8998..223ff44195 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -27,6 +27,7 @@
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/signals-state-save-restore.h"
 #include "gdbsupport/gdb_tilde_expand.h"
+#include "gdbsupport/scoped_pipe.h"
 #include <vector>
 
 extern char **environ;
@@ -262,16 +263,157 @@ execv_argv::init_for_shell (const char *exec_file,
   m_argv.push_back (NULL);
 }
 
-/* See nat/fork-inferior.h.  */
+/* Default implementation of 'trace_me_fail_reason'.  Always return
+   an empty string.  */
 
-pid_t
-fork_inferior (const char *exec_file_arg, const std::string &allargs,
-       char **env, void (*traceme_fun) (),
-       gdb::function_view<void (int)> init_trace_fun,
-       void (*pre_trace_fun) (),
-       const char *shell_file_arg,
-               void (*exec_fun)(const char *file, char * const *argv,
-                                char * const *env))
+static std::string
+default_trace_me_fail_reason (int err)
+{
+  return {};
+}
+
+/* See fork-inferior.h.  */
+
+std::string (*trace_me_fail_reason) (int err)
+  = default_trace_me_fail_reason;
+
+/* See fork-inferior.h.  */
+
+void
+write_trace_errno_to_pipe (int writepipe, int trace_errno)
+{
+  ssize_t writeret;
+
+  do
+    {
+      writeret = write (writepipe, &trace_errno, sizeof (trace_errno));
+    }
+  while (writeret < 0 && (errno == EAGAIN || errno == EINTR));
+
+  if (writeret < 0)
+    error (_("Could not write trace errno: %s"), safe_strerror (errno));
+}
+
+/* Helper function to read TRACE_ERRNO from READPIPE, which handles
+   EINTR/EAGAIN and throws and exception if there was an error.  */
+
+static int
+read_trace_errno_from_pipe (int readpipe)
+{
+  ssize_t readret;
+  int trace_errno;
+
+  do
+    {
+      readret = read (readpipe, &trace_errno, sizeof (trace_errno));
+    }
+  while (readret < 0 && (errno == EAGAIN || errno == EINTR));
+
+  if (readret < 0)
+    error (_("Could not read trace errno: %s"), safe_strerror (errno));
+
+  return trace_errno;
+}
+
+/* See fork-inferior.h.  */
+
+void
+check_child_trace_me_errno (int readpipe)
+{
+  fd_set rset;
+  struct timeval timeout;
+  int ret;
+
+  /* Make sure we have a valid 'trace_me_fail_reason' function
+     defined.  */
+  gdb_assert (trace_me_fail_reason != nullptr);
+
+  FD_ZERO (&rset);
+  FD_SET (readpipe, &rset);
+
+  /* Five seconds should be plenty of time to wait for the child's
+     reply.  */
+  timeout.tv_sec = 5;
+  timeout.tv_usec = 0;
+
+  do
+    {
+      ret = select (readpipe + 1, &rset, NULL, NULL, &timeout);
+    }
+  while (ret < 0 && (errno == EAGAIN || errno == EINTR));
+
+  if (ret < 0)
+    perror_with_name ("select");
+  else if (ret == 0)
+    error (_("Timeout while waiting for child's trace errno"));
+  else
+    {
+      int child_errno;
+
+      child_errno = read_trace_errno_from_pipe (readpipe);
+
+      if (child_errno != 0)
+ {
+  /* The child can't use TRACE_TRACEME.  We have to check whether
+     we know the reason for the failure, and then error out.  */
+  std::string reason = trace_me_fail_reason (child_errno);
+
+  if (reason.empty ())
+    reason = "Could not determine reason for trace failure.";
+
+  /* The child is supposed to display a warning containing the
+     safe_strerror message before us, so we just display the
+     possible reason for the failure.  */
+  error ("%s", reason.c_str ());
+ }
+    }
+}
+
+/* Helper struct for fork_inferior_1, containing information on
+   whether we should check if TRACEME_FUN was successfully called or
+   not.  */
+
+struct traceme_info
+{
+  /* True if we should check whether the call to 'traceme_fun
+     (TRACE_ME...)' succeeded or not. */
+  bool check_trace_me_fail_reason;
+
+  union
+  {
+    /* The non-check version of TRACEME_FUN.  It will be set if
+       CHECK_TRACEME_FAIL_REASON is false.
+
+       This function will usually just perform the call to whatever
+       trace function needed to start tracing the inferior (e.g.,
+       ptrace).  */
+    void (*traceme_fun_nocheck) ();
+
+    /* The check version of TRACEME_FUN.  It will be set if
+       CHECK_TRACEME_FAIL_REASON is true.
+
+       This function will usually perform the call to whatever trace
+       function needed to start tracing the inferior, but will also
+       write its errno value to TRACE_ERRNO_PIPE, so that
+       fork_inferior_1 can check whether it suceeded.  */
+    void (*traceme_fun_check) (int trace_errno_pipe);
+  } u;
+};
+
+/* Helper function.
+
+   Depending on the value of TRACEME_INFO.CHECK_TRACEME_FAIL_REASON,
+   this function will check whether the call to TRACEME_FUN succeeded
+   or not.  */
+
+static pid_t
+fork_inferior_1 (const char *exec_file_arg, const std::string &allargs,
+ char **env, const struct traceme_info traceme_info,
+ gdb::function_view<void (int)> init_trace_fun,
+ void (*pre_trace_fun) (),
+ const char *shell_file_arg,
+ void (*exec_fun)(const char *file, char * const *argv,
+  char * const *env))
 {
   pid_t pid;
   /* Set debug_fork then attach to the child while it sleeps, to debug.  */
@@ -283,6 +425,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
   int save_errno;
   const char *inferior_cwd;
   std::string expanded_inferior_cwd;
+  scoped_pipe trace_pipe;
 
   /* If no exec file handed to us, get it from the exec-file command
      -- with a good, common error message if none is specified.  */
@@ -365,12 +508,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
 
   if (pid == 0)
     {
-      /* Close all file descriptors except those that gdb inherited
- (usually 0/1/2), so they don't leak to the inferior.  Note
- that this closes the file descriptors of all secondary
- UIs.  */
-      close_most_fds ();
-
       /* Change to the requested working directory if the user
  requested it.  */
       if (inferior_cwd != NULL)
@@ -392,7 +529,10 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
          for the inferior.  */
 
       /* "Trace me, Dr. Memory!"  */
-      (*traceme_fun) ();
+      if (traceme_info.check_trace_me_fail_reason)
+ (*traceme_info.u.traceme_fun_check) (trace_pipe.get_write_end ());
+      else
+ (*traceme_info.u.traceme_fun_nocheck) ();
 
       /* The call above set this process (the "child") as debuggable
         by the original gdb process (the "parent").  Since processes
@@ -403,6 +543,12 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
         saying "not parent".  Sorry; you'll have to use print
         statements!  */
 
+      /* Close all file descriptors except those that gdb inherited
+ (usually 0/1/2), so they don't leak to the inferior.  Note
+ that this closes the file descriptors of all secondary
+ UIs, and the trace errno pipe (if it's open).  */
+      close_most_fds ();
+
       restore_original_signals_state ();
 
       /* There is no execlpe call, so we have to set the environment
@@ -431,6 +577,13 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
       _exit (0177);
     }
 
+  if (traceme_info.check_trace_me_fail_reason)
+    {
+      /* Check the trace errno, and inform the user about the reason
+ of the failure, if there was any.  */
+      check_child_trace_me_errno (trace_pipe.get_read_end ());
+    }
+
   /* Restore our environment in case a vforked child clob'd it.  */
   environ = save_our_env;
 
@@ -448,6 +601,48 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
   return pid;
 }
 
+/* See fork-inferior.h.  */
+
+pid_t
+fork_inferior (const char *exec_file_arg, const std::string &allargs,
+       char **env, void (*traceme_fun) (),
+       gdb::function_view<void (int)> init_trace_fun,
+       void (*pre_trace_fun) (),
+       const char *shell_file_arg,
+               void (*exec_fun)(const char *file, char * const *argv,
+                                char * const *env))
+{
+  struct traceme_info traceme_info;
+
+  traceme_info.check_trace_me_fail_reason = false;
+  traceme_info.u.traceme_fun_nocheck = traceme_fun;
+
+  return fork_inferior_1 (exec_file_arg, allargs, env, traceme_info,
+  init_trace_fun, pre_trace_fun, shell_file_arg,
+  exec_fun);
+}
+
+/* See fork-inferior.h.  */
+
+pid_t
+fork_inferior (const char *exec_file_arg, const std::string &allargs,
+       char **env, void (*traceme_fun) (int trace_errno_pipe),
+       gdb::function_view<void (int)> init_trace_fun,
+       void (*pre_trace_fun) (),
+       const char *shell_file_arg,
+               void (*exec_fun)(const char *file, char * const *argv,
+                                char * const *env))
+{
+  struct traceme_info traceme_info;
+
+  traceme_info.check_trace_me_fail_reason = true;
+  traceme_info.u.traceme_fun_check = traceme_fun;
+
+  return fork_inferior_1 (exec_file_arg, allargs, env, traceme_info,
+  init_trace_fun, pre_trace_fun, shell_file_arg,
+  exec_fun);
+}
+
 /* See nat/fork-inferior.h.  */
 
 ptid_t
@@ -592,7 +787,7 @@ trace_start_error (const char *fmt, ...)
 /* See nat/fork-inferior.h.  */
 
 void
-trace_start_error_with_name (const char *string)
+trace_start_error_with_name (const char *string, const char *append)
 {
-  trace_start_error ("%s: %s", string, safe_strerror (errno));
+  trace_start_error ("%s: %s%s", string, safe_strerror (errno), append);
 }
diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
index cf6f137edd..b67215353f 100644
--- a/gdb/nat/fork-inferior.h
+++ b/gdb/nat/fork-inferior.h
@@ -32,17 +32,41 @@ struct process_stratum_target;
 #define START_INFERIOR_TRAPS_EXPECTED 1
 
 /* Start an inferior Unix child process and sets inferior_ptid to its
-   pid.  EXEC_FILE is the file to run.  ALLARGS is a string containing
-   the arguments to the program.  ENV is the environment vector to
-   pass.  SHELL_FILE is the shell file, or NULL if we should pick
-   one.  EXEC_FUN is the exec(2) function to use, or NULL for the default
-   one.  */
-
-/* This function is NOT reentrant.  Some of the variables have been
-   made static to ensure that they survive the vfork call.  */
+   pid.
+
+   EXEC_FILE is the file to run.
+
+   ALLARGS is a string containing the arguments to the program.
+
+   ENV is the environment vector to pass.
+
+   SHELL_FILE is the shell file, or NULL if we should pick one.
+
+   EXEC_FUN is the exec(2) function to use, or NULL for the default
+   one.
+
+   This function is NOT reentrant.  Some of the variables have been
+   made static to ensure that they survive the vfork call.
+
+   This function does not check whether the call to TRACEME_FUN
+   succeeded or not.  */
 extern pid_t fork_inferior (const char *exec_file_arg,
     const std::string &allargs,
-    char **env, void (*traceme_fun) (),
+    char **env,
+    void (*traceme_fun) (),
+    gdb::function_view<void (int)> init_trace_fun,
+    void (*pre_trace_fun) (),
+    const char *shell_file_arg,
+    void (*exec_fun) (const char *file,
+      char * const *argv,
+      char * const *env));
+
+/* Like fork_inferior above, but check whether the call to TRACEME_FUN
+   succeeded or not.  */
+extern pid_t fork_inferior (const char *exec_file_arg,
+    const std::string &allargs,
+    char **env,
+    void (*traceme_fun) (int trace_errno_pipe),
     gdb::function_view<void (int)> init_trace_fun,
     void (*pre_trace_fun) (),
     const char *shell_file_arg,
@@ -82,9 +106,48 @@ extern void trace_start_error (const char *fmt, ...)
   ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 
 /* Like "trace_start_error", but the error message is constructed by
-   combining STRING with the system error message for errno.  This
-   function does not return.  */
-extern void trace_start_error_with_name (const char *string)
+   combining STRING with the system error message for errno, and
+   (optionally) with APPEND.  This function does not return.  */
+extern void trace_start_error_with_name (const char *string,
+ const char *append = "")
   ATTRIBUTE_NORETURN;
 
+/* Pointer to function which can be called by
+   'check_child_trace_me_errno' when we need to determine the reason
+   of a e.g. 'ptrace (PTRACE_ME, ...)' failure.  ERR is the ERRNO
+   value set by the failing ptrace call.
+
+   By default, the function returns an empty string (see
+   fork-inferior.c).
+
+   This pointer can be overriden by targets that want to personalize
+   the error message printed when trace fails (see linux-nat.c or
+   gdbserver/linux-low.c, for example).  */
+extern std::string (*trace_me_fail_reason) (int err);
+
+/* Check the "trace me" errno (generated when executing e.g. 'ptrace
+   (PTRACE_ME, ...)') of the child process that was created by
+   GDB/GDBserver when creating an inferior.  The errno value will be
+   passed via a pipe (see 'fork_inferior'), and READPIPE is the read
+   end of the pipe.
+
+   If possible (i.e., if 'trace_me_fail_reason' is defined by the
+   target), then we also try to determine the possible reason for a
+   failure.
+
+   The idea is to wait a few seconds (via 'select') until something is
+   written on READPIPE.  When that happens, we check if the child's
+   trace errno is different than 0.  If it is, we call the function
+   'trace_me_fail_reason' and try to obtain the reason for the
+   failure, and then throw an exception (with the reason as the
+   exception's message).
+
+   If nothing is written on the pipe, or if 'select' fails, we also
+   throw exceptions.  */
+extern void check_child_trace_me_errno (int readpipe);
+
+/* Helper function to write TRACE_ERRNO to WRITEPIPE, which handles
+   EINTR/EAGAIN and throws an exception if there was an error.  */
+extern void write_trace_errno_to_pipe (int writepipe, int trace_errno);
+
 #endif /* NAT_FORK_INFERIOR_H */
--
2.24.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 4/6] Extend GNU/Linux to check for ptrace error

Sergio Durigan Junior
In reply to this post by Sergio Durigan Junior
This patch implements the ptrace-errno-checking on the GNU/Linux
target (both native and remote).  It builds on top of the previous
'fork_inferior' extension patch.

The idea is to provide a new 'traceme_fun' for each ptrace backend,
which will accept a new integer argument representing the write end of
the ptrace status pipe (that was created in 'fork_inferior').  This
function will invoke the actual tracing syscall (which is 'ptrace' in
this case), get its errno value and write it back via the pipe.  You
can see examples of this new approach by looking at
'inf_ptrace_me' (GDB) or 'linux_ptrace_fun' (gdbserver).

The rest of the patch implements the necessary machinery to do
something useful with the errno information that we received from
'ptrace'.

In Fedora GDB, we carry the following patch:

  https://src.fedoraproject.org/rpms/gdb/blob/8ac06474ff1e2aa4920d14e0666b083eeaca8952/f/gdb-attach-fail-reasons-5of5.patch

Its purpose is to try to detect a specific scenario where SELinux's
'deny_ptrace' option is enabled, which prevents GDB from ptrace'ing in
order to debug the inferior (PTRACE_ATTACH and PTRACE_TRACEME will
fail with EACCES in this case).

I like the idea of improving error detection and providing more
information to the user (a simple "Permission denied" can be really
frustrating), but I don't fully agree with the way the patch was
implemented: it makes GDB link against libselinux only for the sake of
consulting the 'deny_ptrace' setting, and then prints a warning if
ptrace failed and this setting is on.

There is now a new function, 'linux_ptrace_restricted_fail_reason',
which does a few things to check what's wrong with ptrace:

  - It dlopen's "libselinux.so.1" and checks if the "deny_ptrace"
    option is enabled.

  - It reads the contents of "/proc/sys/kernel/yama/ptrace_scope" and
    checks if it's different than 0.

For each of these checks, if it succeeds, the user will see a message
informing about the restriction in place, and how it can be disabled.
For example, if "deny_ptrace" is enabled, the user will see:

  # gdb /usr/bin/true
  ...
  (gdb) run
  Starting program: /usr/bin/true
  warning: Could not trace the inferior process.
  warning: ptrace: Permission denied

  The SELinux 'deny_ptrace' option is enabled and preventing GDB
  from using 'ptrace'.  You can disable it by executing (as root):

    setsebool deny_ptrace off

  If you are debugging the inferior remotely, the ptrace restriction(s) must
  be disabled in the target system (e.g., where GDBserver is running).

In case "/proc/sys/kernel/yama/ptrace_scope" is > 0:

  # gdb /usr/bin/true
  ...
  (gdb) run
  Starting program: /usr/bin/true
  warning: Could not trace the inferior process.
  warning: ptrace: Operation not permitted

  The Linux kernel's Yama ptrace scope is in effect, which can prevent
  GDB from using 'ptrace'.  You can disable it by executing (as root):

    echo 0 > /proc/sys/kernel/yama/ptrace_scope

  If you are debugging the inferior remotely, the ptrace restriction(s) must
  be disabled in the target system (e.g., where GDBserver is running).

If both restrictions are enabled, both messages will show up.

This works for gdbserver as well, and actually fixes a latent bug I
found: when ptrace is restricted, gdbserver would hang due to an
unchecked ptrace call:

  # gdbserver :9988 /usr/bin/true
  gdbserver: linux_ptrace_test_ret_to_nx: Cannot PTRACE_TRACEME: Operation not permitted
  gdbserver: linux_ptrace_test_ret_to_nx: status 256 is not WIFSTOPPED!
  gdbserver: linux_ptrace_test_ret_to_nx: failed to kill child pid 2668100 No such process
  [ Here you would have to issue a C-c ]

Now, you will see:

  # gdbserver :9988 /usr/bin/true
  gdbserver: linux_ptrace_test_ret_to_nx: Cannot PTRACE_TRACEME: Permission denied
  gdbserver: linux_ptrace_test_ret_to_nx: status 256 is not WIFSTOPPED!
  gdbserver: linux_ptrace_test_ret_to_nx: failed to kill child pid 2766868 No such process
  gdbserver: Could not trace the inferior process.
  gdbserver: ptrace: Permission denied

  The SELinux 'deny_ptrace' option is enabled and preventing GDB
  from using 'ptrace'.  You can disable it by executing (as root):

    setsebool deny_ptrace off

  If you are debugging the inferior remotely, the ptrace restriction(s) need
  to be disabled in the target system (e.g., where GDBserver is running).
  Exiting.
  #

(I decided to keep all the other messages, even though I find them a
bit distracting).

If GDB can't determine the cause for the failure, it will still print
the generic error message which tells the user to check our
documentation:

  There might be restrictions preventing ptrace from working.  Please see
  the appendix "Linux kernel ptrace restrictions" in the GDB documentation
  for more details.
  If you are debugging the inferior remotely, the ptrace restriction(s) need
  to be disabled in the target system (e.g., where GDBserver is running).

This means that the series expands our documentation (in the next
patch) and creates a new appendix section named "Linux kernel ptrace
restrictions", with sub-sections for each possible restriction that
might be in place.

Notice how, on every message, we instruct the user to "do the right
thing" if gdbserver is being used.  This is because if the user
started gdbserver *before* any ptrace restriction was in place, and
then, for some reason, one or more restrictions get enabled, then the
error message will be displayed both on gdbserver *and* on the
connected GDB.  Since the user will be piloting GDB, it's important to
explicitly say that the ptrace restrictions are enabled in the target,
where gdbserver is running.

The current list of possible restrictions is:

  - SELinux's 'deny_ptrace' option (detected).

  - YAMA's /proc/sys/kernel/yama/ptrace_scope setting (detected).

  - seccomp on Docker containers (I couldn't find how to detect).

It's important to mention that all of this is Linux-specific; as far
as I know, SELinux, YAMA and seccomp are Linux-only features.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * inf-ptrace.c: Include "nat/fork-inferior.h".
        (inf_ptrace_me): New parameter "trace_errno_pipe".  Check
        "ptrace" errno.
        * linux-nat.c: Include "nat/fork-inferior.h".
        (attach_proc_task_lwp_callback): Call
        "linux_ptrace_attach_fail_reason_lwp" instead of
        "linux_ptrace_attach_fail_reason_string".
        (linux_nat_target::attach): Save "ERRNO".  Pass it to
        "linux_ptrace_attach_fail_reason".
        (_initialize_linux_nat): Set "trace_me_fail_reason".
        * nat/linux-ptrace.c: Include "gdbsupport/gdb-dlfcn.h",
        "nat/fork-inferior.h" and "gdbsupport/filestuff.h".
        (selinux_ftype): New type.
        (linux_ptrace_restricted_fail_reason): New function.
        (linux_ptrace_attach_fail_reason_1): New function, renamed
        from "linux_ptrace_attach_fail_reason".
        (linux_ptrace_attach_fail_reason): New function.
        (linux_ptrace_attach_fail_reason_lwp): Likewise.
        (linux_ptrace_me_fail_reason): Likewise.
        (errno_pipe): New variable.
        (linux_child_function): Check "ptrace" errno.  Send it through
        the pipe.
        (linux_check_ptrace_features): Initialize pipe.  Check
        "ptrace" errno sent through the pipe.
        * nat/linux-ptrace.h (linux_ptrace_attach_fail_reason): New
        function.
        (linux_ptrace_attach_fail_reason_lwp): Likewise.
        (linux_ptrace_me_fail_reason): Likewise.
        * remote.c (extended_remote_target::attach): Check error
        message on PACKET_ERROR.
        (remote_target::extended_remote_run): Likewise.

gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * linux-low.cc (linux_ptrace_fun): New parameter
        "trace_errno_wpipe".  Check "ptrace" errno.
        (attach_proc_task_lwp_callback): Call
        "linux_ptrace_attach_fail_reason_lwp" instead of
        "linux_ptrace_attach_fail_reason_string".
        (linux_process_target::attach): Likewise.
        (initialize_low): Set "trace_me_fail_reason".
        * server.cc (handle_v_attach): Check if "attach_inferior"
        succeeded.
        (handle_v_run): Likewise.
        * thread-db.cc (attach_thread): Call
        "linux_ptrace_attach_fail_reason_lwp" instead of
        "linux_ptrace_attach_fail_reason_string".
---
 gdb/inf-ptrace.c        |  18 +++-
 gdb/linux-nat.c         |  10 ++-
 gdb/nat/fork-inferior.c |   6 +-
 gdb/nat/fork-inferior.h |   2 +-
 gdb/nat/linux-ptrace.c  | 178 ++++++++++++++++++++++++++++++++++++++--
 gdb/nat/linux-ptrace.h  |  27 ++++--
 gdb/remote.c            |  40 ++++++++-
 gdbserver/linux-low.cc  |  31 +++++--
 gdbserver/server.cc     |  38 ++++++++-
 gdbserver/thread-db.cc  |   2 +-
 10 files changed, 318 insertions(+), 34 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index db17a76d94..8fb7264700 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -34,6 +34,7 @@
 #include "nat/fork-inferior.h"
 #include "utils.h"
 #include "gdbarch.h"
+#include "nat/fork-inferior.h"
 
 
 
@@ -97,10 +98,23 @@ inf_ptrace_target::remove_fork_catchpoint (int pid)
 /* Prepare to be traced.  */
 
 static void
-inf_ptrace_me (void)
+inf_ptrace_me (int trace_errno_wpipe)
 {
   /* "Trace me, Dr. Memory!"  */
-  if (ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3) 0, 0) < 0)
+  int ret = ptrace (PT_TRACE_ME, 0, (PTRACE_TYPE_ARG3) 0, 0);
+  int ptrace_errno = ret < 0 ? errno : 0;
+
+  try
+    {
+      write_trace_errno_to_pipe (trace_errno_wpipe, ptrace_errno);
+    }
+  catch (const gdb_exception &e)
+    {
+      warning ("%s", e.what ());
+      _exit (0177);
+    }
+
+  if (ret < 0)
     trace_start_error_with_name ("ptrace");
 }
 
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 81af83c4ac..31a5da8b52 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -31,6 +31,7 @@
 #include "nat/linux-ptrace.h"
 #include "nat/linux-procfs.h"
 #include "nat/linux-personality.h"
+#include "nat/fork-inferior.h"
 #include "linux-fork.h"
 #include "gdbthread.h"
 #include "gdbcmd.h"
@@ -1136,7 +1137,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 ());
@@ -1191,8 +1192,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 (),
@@ -4582,6 +4584,10 @@ Enables printf debugging output."),
   sigemptyset (&blocked_mask);
 
   lwp_lwpid_htab_create ();
+
+  /* Set the proper function to generate a message when ptrace
+     fails.  */
+  trace_me_fail_reason = linux_ptrace_me_fail_reason;
 }
 
 
diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 223ff44195..eb4c4625d7 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -394,9 +394,9 @@ struct traceme_info
 
        This function will usually perform the call to whatever trace
        function needed to start tracing the inferior, but will also
-       write its errno value to TRACE_ERRNO_PIPE, so that
+       write its errno value to TRACE_ERRNO_WPIPE, so that
        fork_inferior_1 can check whether it suceeded.  */
-    void (*traceme_fun_check) (int trace_errno_pipe);
+    void (*traceme_fun_check) (int trace_errno_wpipe);
   } u;
 };
 
@@ -626,7 +626,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
 
 pid_t
 fork_inferior (const char *exec_file_arg, const std::string &allargs,
-       char **env, void (*traceme_fun) (int trace_errno_pipe),
+       char **env, void (*traceme_fun) (int trace_errno_wpipe),
        gdb::function_view<void (int)> init_trace_fun,
        void (*pre_trace_fun) (),
        const char *shell_file_arg,
diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
index b67215353f..3fbead2e33 100644
--- a/gdb/nat/fork-inferior.h
+++ b/gdb/nat/fork-inferior.h
@@ -66,7 +66,7 @@ extern pid_t fork_inferior (const char *exec_file_arg,
 extern pid_t fork_inferior (const char *exec_file_arg,
     const std::string &allargs,
     char **env,
-    void (*traceme_fun) (int trace_errno_pipe),
+    void (*traceme_fun) (int trace_errno_wpipe),
     gdb::function_view<void (int)> init_trace_fun,
     void (*pre_trace_fun) (),
     const char *shell_file_arg,
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index 5335d69092..b3fcf8bc07 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -21,6 +21,9 @@
 #include "linux-procfs.h"
 #include "linux-waitpid.h"
 #include "gdbsupport/buffer.h"
+#include "gdbsupport/gdb-dlfcn.h"
+#include "nat/fork-inferior.h"
+#include "gdbsupport/filestuff.h"
 #ifdef HAVE_SYS_PROCFS_H
 #include <sys/procfs.h>
 #endif
@@ -30,11 +33,93 @@
    of 0 means there are no supported features.  */
 static int supported_ptrace_options = -1;
 
-/* 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.  */
+typedef int (*selinux_ftype) (const char *);
 
-std::string
-linux_ptrace_attach_fail_reason (pid_t pid)
+/* Helper function which checks if ptrace is probably restricted
+   (i.e., if ERR is either EACCES or EPERM), and returns a string with
+   possible workarounds.  */
+
+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)
+    {
+    }
+
+  if (handle != nullptr)
+    {
+      selinux_ftype selinux_get_bool
+ = (selinux_ftype) gdb_dlsym (handle, "security_get_boolean_active");
+
+      if (selinux_get_bool != NULL
+  && (*selinux_get_bool) ("deny_ptrace") == 1)
+ string_appendf (ret,
+ _("\n\
+The SELinux 'deny_ptrace' option is enabled and preventing GDB\n\
+from using 'ptrace'.  You can disable it by executing (as root):\n\
+\n\
+  setsebool deny_ptrace off\n"));
+    }
+
+  gdb_file_up yama_ptrace_scope
+    = gdb_fopen_cloexec ("/proc/sys/kernel/yama/ptrace_scope", "r");
+
+  if (yama_ptrace_scope != nullptr)
+    {
+      char yama_scope = fgetc (yama_ptrace_scope.get ());
+
+      if (yama_scope != '0')
+ string_appendf (ret,
+ _("\n\
+The Linux kernel's Yama ptrace scope is in effect, which can prevent\n\
+GDB from using 'ptrace'.  You can disable it by executing (as root):\n\
+\n\
+  echo 0 > /proc/sys/kernel/yama/ptrace_scope\n"));
+    }
+
+  if (ret.empty ())
+    {
+      /* It wasn't possible to determine the exact reason for the
+ ptrace error.  Let's just emit a generic error message
+ pointing the user to our documentation, where she can find
+ instructions on how to try to diagnose the problem.  */
+      ret = _("\n\
+There might be restrictions preventing ptrace from working.  Please see\n\
+the appendix \"Linux kernel ptrace restrictions\" in the GDB documentation\n\
+for more details.");
+    }
+
+  /* The user may be debugging remotely, so we have to warn that
+     the instructions above should be performed in the target.  */
+  string_appendf (ret,
+  _("\n\
+If you are debugging the inferior remotely, the ptrace restriction(s) must\n\
+be disabled in the target system (e.g., where GDBserver is running)."));
+
+  return ret;
+}
+
+/* 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.  */
+
+static std::string
+linux_ptrace_attach_fail_reason_1 (pid_t pid)
 {
   pid_t tracerpid = linux_proc_get_tracerpid_nowarn (pid);
   std::string result;
@@ -56,10 +141,24 @@ linux_ptrace_attach_fail_reason (pid_t pid)
 /* See linux-ptrace.h.  */
 
 std::string
-linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err)
+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;
+
+  return result;
+}
+
+/* See linux-ptrace.h.  */
+
+std::string
+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,
@@ -68,6 +167,14 @@ linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err)
     return string_printf ("%s (%d)", safe_strerror (err), err);
 }
 
+/* See linux-ptrace.h.  */
+
+std::string
+linux_ptrace_me_fail_reason (int err)
+{
+  return linux_ptrace_restricted_fail_reason (err);
+}
+
 #if defined __i386__ || defined __x86_64__
 
 /* Address of the 'ret' instruction in asm code block below.  */
@@ -257,6 +364,12 @@ linux_ptrace_test_ret_to_nx (void)
 #endif /* defined __i386__ || defined __x86_64__ */
 }
 
+/* If the PTRACE_TRACEME call on linux_child_function errors, we need
+   to be able to send ERRNO back to the parent so that it can check
+   whether there are restrictions in place preventing ptrace from
+   working.  We do that with a pipe.  */
+static int errno_pipe[2];
+
 /* Helper function to fork a process and make the child process call
    the function FUNCTION, passing CHILD_STACK as parameter.
 
@@ -321,7 +434,30 @@ linux_grandchild_function (void *child_stack)
 static int
 linux_child_function (void *child_stack)
 {
-  ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0);
+  /* Close read end.  */
+  close (errno_pipe[0]);
+
+  int ret = ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
+    (PTRACE_TYPE_ARG4) 0);
+  int ptrace_errno = ret < 0 ? errno : 0;
+
+  /* Write ERRNO to the pipe, even if it's zero, and close the writing
+     end of the pipe.  */
+  try
+    {
+      write_trace_errno_to_pipe (errno_pipe[1], ptrace_errno);
+    }
+  catch (const gdb_exception &e)
+    {
+      warning ("%s", e.what ());
+      _exit (0177);
+    }
+
+  close (errno_pipe[1]);
+
+  if (ret != 0)
+    trace_start_error_with_name ("ptrace");
+
   kill (getpid (), SIGSTOP);
 
   /* Fork a grandchild.  */
@@ -346,12 +482,40 @@ linux_check_ptrace_features (void)
   /* Initialize the options.  */
   supported_ptrace_options = 0;
 
+  /* Initialize our pipe.  */
+  if (gdb_pipe_cloexec (errno_pipe) < 0)
+    perror_with_name ("gdb_pipe_cloexec");
+
   /* Fork a child so we can do some testing.  The child will call
      linux_child_function and will get traced.  The child will
      eventually fork a grandchild so we can test fork event
      reporting.  */
   child_pid = linux_fork_to_function (NULL, linux_child_function);
 
+  /* We don't need the write end of the pipe anymore.  */
+  close (errno_pipe[1]);
+
+  try
+    {
+      /* Check whether 'ptrace (PTRACE_ME, ...)' failed when being
+ invoked by the child.  If it did, we might get the
+ possible reason for it as the exception message.  */
+      check_child_trace_me_errno (errno_pipe[0]);
+    }
+  catch (const gdb_exception &e)
+    {
+      /* Close the pipe so we don't leak fd's.  */
+      close (errno_pipe[0]);
+
+      /* A failure here means that PTRACE_ME failed, which means that
+ GDB/gdbserver will most probably not work correctly.  If we
+ want to be pedantic, we could just call 'exit' here.
+ However, let's just re-throw the exception.  */
+      throw;
+    }
+
+  close (errno_pipe[0]);
+
   ret = my_waitpid (child_pid, &status, 0);
   if (ret == -1)
     perror_with_name (("waitpid"));
diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index 65568301f2..7cb77114ca 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -176,12 +176,27 @@ struct buffer;
 # define TRAP_HWBKPT 4
 #endif
 
-extern std::string linux_ptrace_attach_fail_reason (pid_t pid);
-
-/* 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);
+/* 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);
+
+/* Find all possible reasons we could have failed to attach to PID's
+   LWPID and return them as a string.  ERR is the error PTRACE_ATTACH
+   failed 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 pid, 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
+   obtain the reason why ptrace failed.  ERR should be the ERRNO value
+   returned by ptrace.
+
+   This function will return a 'std::string' containing the fail
+   reason, or an empty string otherwise.  */
+extern std::string linux_ptrace_me_fail_reason (int err);
 
 extern void linux_ptrace_init_warnings (void);
 extern void linux_check_ptrace_features (void);
diff --git a/gdb/remote.c b/gdb/remote.c
index 4a70ab3fb0..7e0655974c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5882,9 +5882,26 @@ extended_remote_target::attach (const char *args, int from_tty)
       break;
     case PACKET_UNKNOWN:
       error (_("This target does not support attaching to a process"));
+    case PACKET_ERROR:
+      {
+ std::string errmsg = rs->buf.data ();
+
+ /* Check if we have a specific error (i.e., not a generic
+   "E01") coming from the target.  If there is, we print it
+   here.  */
+ if (startswith (errmsg.c_str (), "E."))
+  {
+    /* Get rid of the "E." prefix.  */
+    errmsg.erase (0, 2);
+  }
+
+ error (_("Attaching to %s failed%s%s"),
+       target_pid_to_str (ptid_t (pid)).c_str (),
+       !errmsg.empty () ? "\n" : "",
+       errmsg.c_str ());
+      }
     default:
-      error (_("Attaching to %s failed"),
-     target_pid_to_str (ptid_t (pid)).c_str ());
+      gdb_assert_not_reached (_("bad switch"));
     }
 
   set_current_inferior (remote_add_inferior (false, pid, 1, 0));
@@ -10003,8 +10020,23 @@ remote_target::extended_remote_run (const std::string &args)
  error (_("Running the default executable on the remote target failed; "
  "try \"set remote exec-file\"?"));
       else
- error (_("Running \"%s\" on the remote target failed"),
-       remote_exec_file);
+ {
+  std::string errmsg = rs->buf.data ();
+
+  /* Check if we have a specific error (i.e., not a generic
+     "E01") coming from the target.  If there is, we print it
+     here.  */
+  if (startswith (errmsg.c_str (), "E."))
+    {
+      /* Get rid of the "E." prefix.  */
+      errmsg.erase (0, 2);
+    }
+
+  error (_("Running \"%s\" on the remote target failed%s%s"),
+ remote_exec_file,
+ !errmsg.empty () ? "\n" : "",
+ errmsg.c_str ());
+ }
     default:
       gdb_assert_not_reached (_("bad switch"));
     }
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 2872bc78da..42283802dd 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -968,10 +968,24 @@ add_lwp (ptid_t ptid)
    actually initiating the tracing of the inferior.  */
 
 static void
-linux_ptrace_fun ()
+linux_ptrace_fun (int ptrace_errno_wpipe)
 {
-  if (ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
-      (PTRACE_TYPE_ARG4) 0) < 0)
+  int ret = ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
+    (PTRACE_TYPE_ARG4) 0);
+  int ptrace_errno = ret < 0 ? errno : 0;
+
+  try
+    {
+      write_trace_errno_to_pipe (ptrace_errno_wpipe, ptrace_errno);
+    }
+  catch (const gdb_exception &e)
+    {
+      warning ("%s", e.what ());
+      _exit (0177);
+    }
+
+  errno = ptrace_errno;
+  if (ret < 0)
     trace_start_error_with_name ("ptrace");
 
   if (setpgid (0, 0) < 0)
@@ -1170,7 +1184,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,8 +1216,8 @@ linux_process_target::attach (unsigned long pid)
     {
       remove_process (proc);
 
-      std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
-      error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
+      std::string reason = linux_ptrace_attach_fail_reason (pid, err);
+      error (_("Cannot attach to process %ld: %s"), pid, reason.c_str ());
     }
 
   /* Don't ignore the initial SIGSTOP if we just attached to this
@@ -7552,5 +7566,10 @@ initialize_low (void)
 
   initialize_low_arch ();
 
+  /* Initialize the 'trace_me_fail_reason' function pointer.  We will
+     use this to determine the reason for possible failures when
+     invoking 'ptrace (PTRACE_ME, ...)'.  */
+  trace_me_fail_reason = linux_ptrace_me_fail_reason;
+
   linux_check_ptrace_features ();
 }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index a4cb1eb418..d0b0c5a4ad 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2891,9 +2891,31 @@ handle_v_attach (char *own_buf)
 {
   client_state &cs = get_client_state ();
   int pid;
+  int ret;
 
   pid = strtol (own_buf + 8, NULL, 16);
-  if (pid != 0 && attach_inferior (pid) == 0)
+
+  if (pid <= 0)
+    {
+      write_enn (own_buf);
+      return 0;
+    }
+
+  try
+    {
+      /* Attach to the specified PID.  This function can throw, so we
+ make sure to catch the exception and send it (as an error
+ packet) back to GDB.  */
+      ret = attach_inferior (pid);
+    }
+  catch (const gdb_exception_error &e)
+    {
+      fprintf (stderr, "%s\n", e.what ());
+      snprintf (own_buf, PBUFSIZ, "E.%s", e.what ());
+      return 0;
+    }
+
+  if (ret == 0)
     {
       /* Don't report shared library events after attaching, even if
  some libraries are preloaded.  GDB will always poll the
@@ -3029,7 +3051,19 @@ handle_v_run (char *own_buf)
   free_vector_argv (program_args);
   program_args = new_argv;
 
-  target_create_inferior (program_path.get (), program_args);
+  try
+    {
+      /* Create the inferior.  This function can throw, so we make
+ sure to catch the exception and send it (as an error packet)
+ back to GDB.  */
+      target_create_inferior (program_path.get (), program_args);
+    }
+  catch (const gdb_exception_error &e)
+    {
+      fprintf (stderr, "%s\n", e.what ());
+      snprintf (own_buf, PBUFSIZ, "E.%s", e.what ());
+      return 0;
+    }
 
   if (cs.last_status.kind == TARGET_WAITKIND_STOPPED)
     {
diff --git a/gdbserver/thread-db.cc b/gdbserver/thread-db.cc
index 2bb6d28820..60ceb7b663 100644
--- a/gdbserver/thread-db.cc
+++ b/gdbserver/thread-db.cc
@@ -224,7 +224,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 ());
--
2.24.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 5/6] Document Linux-specific possible ptrace restrictions

Sergio Durigan Junior
In reply to this post by Sergio Durigan Junior
This patch creates a new "Linux kernel ptrace restrictions" which
documents possible causes that can be prevent the inferior from being
correctly started/debugged.

This has been pre-approved by Eli.

gdb/doc/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * gdb.texinfo (Linux kernel ptrace restrictions): New appendix
        section.
---
 gdb/doc/gdb.texinfo | 143 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f1798e35b5..a95158d5d3 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -182,6 +182,9 @@ software in general.  We will miss him.
                                 @value{GDBN}
 * Operating System Information:: Getting additional information from
                                  the operating system
+* Linux kernel ptrace restrictions::        Restrictions sometimes
+                                            imposed by the Linux
+                                            kernel on @code{ptrace}
 * Trace File Format:: GDB trace file format
 * Index Section Format::        .gdb_index section format
 * Man Pages:: Manual pages
@@ -45629,6 +45632,146 @@ should contain a comma-separated list of cores that this process
 is running on.  Target may provide additional columns,
 which @value{GDBN} currently ignores.
 
+@node Linux kernel ptrace restrictions
+@appendix Linux kernel @code{ptrace} restrictions
+@cindex linux kernel ptrace restrictions, attach
+
+The @code{ptrace} system call is used by @value{GDBN} and
+@code{gdbserver} on GNU/Linux to, among other things, attach to a new
+or existing inferior in order to start debugging it.  Due to security
+concerns, some distributions and vendors disable or severely restrict
+the ability to perform these operations, which can make @value{GDBN}
+or @code{gdbserver} malfunction.  In this section, we will expand on
+how this malfunction can manifest itself, and how to modify the
+system's settings in order to be able to use @value{GDBN} and
+@code{gdbserver} properly.
+
+@menu
+* The error message::                   The error message displayed when the
+                                        system prevents @value{GDBN}
+                                        or @code{gdbserver} from using
+                                        @code{ptrace}
+* SELinux's deny_ptrace::               SELinux and the @code{deny_ptrace} option
+* Yama's ptrace_scope::                 Yama and the @code{ptrace_scope} setting
+* Docker and seccomp::                  Docker and the @code{seccomp}
+                                        infrastructure
+@end menu
+
+@node The error message
+@appendixsection The error message
+
+When the system prevents @value{GDBN} or @code{gdbserver} from using
+the @code{ptrace} system call, you will likely see a descriptive error
+message explaining what is wrong and how to attempt to fix the
+problem.  For example, when SELinux's @code{deny_ptrace} option is
+enabled, you can see:
+
+@smallexample
+$ gdb program
+...
+(@value{GDBP}) run
+Starting program: program
+warning: Could not trace the inferior process.
+Error:
+warning: ptrace: Permission denied
+The SELinux 'deny_ptrace' option is enabled and preventing @value{GDBN}
+from using 'ptrace'.  You can disable it by executing (as root):
+
+  setsebool deny_ptrace off
+
+If you are debugging the inferior remotely, the instruction(s) above must
+be performed in the target system (e.g., where GDBserver is running).
+During startup program exited with code 127.
+(@value{GDBP})
+@end smallexample
+
+Sometimes, it may not be possible to acquire the necessary data to
+determine the root cause of the failure.  In this case, you will see a
+generic error message pointing you to this section:
+
+@smallexample
+$ gdb program
+...
+Starting program: program
+warning: Could not trace the inferior process.
+Error:
+warning: ptrace: Permission denied
+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
+
+@node SELinux's deny_ptrace
+@appendixsection SELinux's @code{deny_ptrace}
+@cindex SELinux
+@cindex deny_ptrace
+
+If you are using SELinux, you might want to check whether the
+@code{deny_ptrace} option is enabled by doing:
+
+@smallexample
+$ getsebool deny_ptrace
+deny_ptrace --> on
+@end smallexample
+
+If the option is enabled, you can disable it by doing, as root:
+
+@smallexample
+# setsebool deny_ptrace off
+@end smallexample
+
+The option will be disabled until the next reboot.  If you would like
+to disable it permanently, you can do (as root):
+
+@smallexample
+# setsebool -P deny_ptrace off
+@end smallexample
+
+@node Yama's ptrace_scope
+@appendixsection Yama's @code{ptrace_scope}
+@cindex yama, ptrace_scope
+
+If your system has Yama enabled, you might want to check whether the
+@code{ptrace_scope} setting is enabled by checking the value of
+@file{/proc/sys/kernel/yama/ptrace_scope}:
+
+@smallexample
+$ cat /proc/sys/kernel/yama/ptrace_scope
+0
+@end smallexample
+
+If you see anything other than @code{0}, @value{GDBN} or
+@code{gdbserver} can be affected by it.  You can temporarily disable
+the feature by doing, as root:
+
+@smallexample
+# sysctl kernel.yama.ptrace_scope=0
+kernel.yama.ptrace_scope = 0
+@end smallexample
+
+You can make this permanent by doing, as root:
+
+@smallexample
+# sysctl -w kernel.yama.ptrace_scope=0
+kernel.yama.ptrace_scope = 0
+@end smallexample
+
+@node Docker and seccomp
+@appendixsection Docker and @code{seccomp}
+@cindex docker, seccomp
+
+If you are using Docker (@uref{https://www.docker.com/}) containers,
+you will probably have to disable its @code{seccomp} protections in
+order to be able to use @value{GDBN} or @code{gdbserver}.  To do that,
+you can use the options @code{--cap-add=SYS_PTRACE --security-opt
+seccomp=unconfined} when invoking Docker:
+
+@smallexample
+$ docker run --cap-add=SYS_PTRACE --security-opt seccomp=unconfined
+@end smallexample
+
 @node Trace File Format
 @appendix Trace File Format
 @cindex trace file format
--
2.24.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 6/6] Fix comment for 'gdb_dlopen'

Sergio Durigan Junior
In reply to this post by Sergio Durigan Junior
The 'gdb_dlopen' function doesn't return NULL if the shlib load
fails;n it actually throws an error.  This patch updates the comment
to reflect this.

gdbsupport/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

        * gdb-dlfcn.h (gdb_dlopen): Update comment.
---
 gdbsupport/gdb-dlfcn.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdbsupport/gdb-dlfcn.h b/gdbsupport/gdb-dlfcn.h
index 258cfebbc3..9e72a53dc0 100644
--- a/gdbsupport/gdb-dlfcn.h
+++ b/gdbsupport/gdb-dlfcn.h
@@ -32,8 +32,8 @@ struct dlclose_deleter
 typedef std::unique_ptr<void, dlclose_deleter> gdb_dlhandle_up;
 
 /* Load the dynamic library file named FILENAME, and return a handle
-   for that dynamic library.  Return NULL if the loading fails for any
-   reason.  */
+   for that dynamic library.  Throw an error if the loading fails for
+   any reason.  */
 
 gdb_dlhandle_up gdb_dlopen (const char *filename);
 
--
2.24.1

12345