Re: [PATCH v2 4/5] Extend GNU/Linux to check for ptrace error

Pedro Alves
2020-03-27T15:28:31Z

On 3/17/20 3:47 PM, Sergio Durigan Junior via Gdb-patches wrote:
> 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 <sergiodj@redhat.com>
>
> * inf-ptrace.c: Include "nat/fork-inferior.h".
> (inf_ptrace_me): New parameter "trace_errno_wpipe". Check
> "ptrace" errno.
> (inf_ptrace_target::attach): Rewrite to use
> "inf_ptrace_target::ptrace_attach".
> (inf_ptrace_target::ptrace_attach): New function, almost
> identical to the previous "inf_ptrace_target::attach".
> * inf-ptrace.h (struct inf_ptrace_target) <int ptrace_attach>:
> New method.
> * 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 <sergiodj@redhat.com>
>
> * 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 | 34 +++++++-
> gdb/inf-ptrace.h | 2 +
> gdb/linux-nat.c | 24 +++---
> 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 +-
> 11 files changed, 339 insertions(+), 45 deletions(-)
>
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index db17a76d94..941f019709 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"
>
> \f
>
> @@ -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);
> + }

We've been avoiding try/catch in an async-signal-safe-only environment
(between fork and exec/exit). This is another spot leaking in.

AFAICT, all cases of write_trace_errno_to_pipe throwing end up
caught be the immediate caller catching the error, warning and
calling _exit. So how about doing the warning+_exit directly
within write_trace_errno_to_pipe and thus avoid the try/catch?

> +
> + if (ret < 0)
> trace_start_error_with_name ("ptrace");

Seems like errno was already lost when you get here, since
write_trace_errno_to_pipe clobbers errno.

BTW, you could do with a single ret < 0 check, I think? Like:

if (ret < 0)
{
int ptrace_errno = errno;

write_trace_errno_to_pipe (trace_errno_wpipe, ptrace_errno);

errno = ptrace_errno;

trace_start_error_with_name ("ptrace");
}

Though you could make (and document) write_trace_errno_to_pipe
preserve errno itself. I'd even consider removing its
trace_errno parameter, resulting in this on the caller side:

if (ret < 0)
{
write_trace_errno_to_pipe (trace_errno_wpipe);
trace_start_error_with_name ("ptrace");
}

Note how trace_start_error_with_name already uses the global
errno, so it wouldn't be strange.

> }
>
> @@ -185,6 +199,18 @@ inf_ptrace_target::mourn_inferior ()
>
> void
> inf_ptrace_target::attach (const char *args, int from_tty)
> +{
> + errno = ptrace_attach (args, from_tty);
> + if (errno != 0)
> + perror_with_name (("ptrace"));
> +}
> +
> +/* Attach to the process specified by ARGS. If FROM_TTY is non-zero,
> + be chatty about it. Return ERRNO if the call to ptrace failed; 0
> + otherwise. */

This should also mention that we throw an error for other reasons.

Something like:

Returns ERRNO if the call to ptrace failed; 0 if ptrace succeeded.
Throws an error if it fails for reasons other than a ptrace failure.

> +
> +int
> +inf_ptrace_target::ptrace_attach (const char *args, int from_tty)
> {
> pid_t pid;
> struct inferior *inf;
> @@ -223,7 +249,7 @@ inf_ptrace_target::attach (const char *args, int from_tty)
> errno = 0;
> ptrace (PT_ATTACH, pid, (PTRACE_TYPE_ARG3)0, 0);
> if (errno != 0)
> - perror_with_name (("ptrace"));
> + return errno;
> #else
> error (_("This system does not support attaching to a process"));
> #endif
> @@ -241,6 +267,8 @@ inf_ptrace_target::attach (const char *args, int from_tty)
> set_executing (this, thr->ptid, true);
>
> unpusher.release ();
> +
> + return 0;
> }
>
> #ifdef PT_GET_PROCESS_STATE
> diff --git a/gdb/inf-ptrace.h b/gdb/inf-ptrace.h
> index dd0733736f..ca36ca3af3 100644
> --- a/gdb/inf-ptrace.h
> +++ b/gdb/inf-ptrace.h
> @@ -31,6 +31,8 @@ struct inf_ptrace_target : public inf_child_target
>
> void attach (const char *, int) override;
>
> + int ptrace_attach (const char *, int);
> +
> void detach (inferior *inf, int) override;
>
> void resume (ptid_t, int, enum gdb_signal) override;
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 81af83c4ac..cc044ee3ac 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 ());
> @@ -1185,20 +1186,15 @@ linux_nat_target::attach (const char *args, int from_tty)
> /* Make sure we report all signals during attach. */
> pass_signals ({});
>
> - try
> - {
> - inf_ptrace_target::attach (args, from_tty);
> - }
> - catch (const gdb_exception_error &ex)
> + int err = inf_ptrace_target::ptrace_attach (args, from_tty);
> +
> + if (err != 0)
> {
> 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, err);
>
> - if (!reason.empty ())
> - throw_error (ex.error, "warning: %s\n%s", reason.c_str (),
> - ex.what ());
> - else
> - throw_error (ex.error, "%s", ex.what ());
> + error (_("warning: ptrace: %s\n%s"),
> + safe_strerror (err), reason.c_str ());
> }
>
> /* The ptrace base target adds the main thread with (pid,0,0)
> @@ -4582,6 +4578,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;
> }
> \f
>
> 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);

I was surprised to see this renaming going on this patch. Why not
name the variable like that in the previous patch, where it was
introduced, to begin with?

> } 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),

Ditto. Etc.

> 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];
> +

We're missing a comment somewhere saying that we do that on the parent side
to avoid doing non-async-signal-safe things in the child. Here might be
a good place, but on the gdb side we could use some comment about it too.

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

Here's another spot that should do away with try/catch.

> +
> + 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]);

This is assuming that close doesn't clober errno, which in general
is not garanteed:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/errno.html#tag_16_110

"The setting of errno after a successful call to a function is unspecified
unless the description of that function specifies that errno shall not
be modified."

A quick web search finds this for example:

https://git.furworks.de/opensourcemirror/git/commit/06121a0a8328c8aaa7a023cf6ebb142e9dc2b45c

> + }
> + catch (const gdb_exception &e)
> + {
> + /* Close the pipe so we don't leak fd's. */

fd's -> fds

> + 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 0f78b1be1b..aacbdf1984 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 ();

This is taking a deep copy which seems unnecessary.

The code would look almost the same without it. Like:

const char *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, "E."))
{
/* Get rid of the "E." prefix. */
errmsg += 2;
}

error (_("Attaching to %s failed%s%s"),
target_pid_to_str (ptid_t (pid)).c_str (),
errmsg != '\0' ? "\n" : "",
errmsg);

> +
> + /* 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));
> @@ -10024,8 +10041,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 ());

Ditto.

> + }
> 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);
> + }

Another spot that could do without try/catch.

> +
> + 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 43962adc86..003385f42a 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -2892,9 +2892,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
> @@ -3030,7 +3052,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 ());
>

--
Thanks,
Pedro Alves