[PATCH v2] Improve ptrace-error detection on Linux targets
Sergio Durigan Junior
sergiodj@redhat.com
Thu Aug 29 19:27:00 GMT 2019
On Thursday, August 29 2019, Pedro Alves wrote:
> Hi Sergio,
>
> This is looking quite good to me. Some comments below.
Thanks for the review, Pedro.
> On 8/26/19 7:32 PM, Sergio Durigan Junior wrote:
>> Changes from v1:
>>
>> - Addressed Pedro's comments re. internal organization and gdbserver
>> support.
>>
>> - Addressed Eli's comments (doc fixes).
>>
>> - New ways of detecting what's wrong with ptrace.
>>
>>
>>
>> In Fedora GDB, we carry the following patch:
>>
>> https://src.fedoraproject.org/rpms/gdb/blob/master/f/gdb-attach-fail-reasons-5of5.patch
>
> This link will soon be dead, right? (thinking about the commit log)
> Maybe point at some reference other than master.
Yeah. I will provide a link to a commit.
>> 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_ME 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" and checks if the "deny_ptrace" option
>> is enabled.
>
> Update reference to "libselinux.so.1" here too.
Done.
>>
>> - 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 /bin/true
>> ...
>> Starting program: /usr/bin/true
>> warning: Could not trace the inferior process.
>> Error:
>> warning: ptrace: Permission denied
>
> Curious, that:
>
> warning:
> Error:
> warning:
>
> looks a bit odd, specifically the "Error:" line. Do you know where is
> that coming from?
Not offhand; I'll investigate and come back with the results.
>> The SELinux 'deny_ptrace' option is enabled and preventing GDB
>> from using 'ptrace'. Please, disable it by executing (as root):
>
> I'm really not a fan of these "Please, ". Kind of sounds like
> gdb is begging, to me. I'd rather use an informational tone, like:
>
> The SELinux 'deny_ptrace' option is enabled and preventing GDB
> from using 'ptrace'. You can disable it by executing (as root):
Fair enough. Changed as requested.
>>
>> setsebool deny_ptrace off
>>
>> During startup program exited with code 127.
>>
>> In case "/proc/sys/kernel/yama/ptrace_scope" is > 0:
>>
>> # gdb /bin/true
>> ...
>> Starting program: /usr/bin/true
>> warning: Could not trace the inferior process.
>> Error:
>> warning: ptrace: Operation not permitted
>> The Linux kernel's Yama ptrace scope is in effect, which can prevent
>> GDB from using 'ptrace'. Please, disable it by executing (as root):
>
> Ditto.
Changed.
>> echo 0 > /proc/sys/kernel/yama/ptrace_scope
>>
>> During startup program exited with code 127.
>>
>> 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 /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 /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
>> gdbserver: Could not trace the inferior process.
>> Error:
>> gdbserver: ptrace: Operation not permitted
>> The Linux kernel's Yama ptrace scope is in effect, which can prevent
>> GDB from using 'ptrace'. Please, disable it by executing (as root):
>>
>> echo 0 > /proc/sys/kernel/yama/ptrace_scope
>>
>> linux_check_ptrace_features: waitpid: unexpected status 32512.
>> Exiting
>>
>> (I decided to keep all the other messages, even though I find them a
>> bit distracting).
>
> Yeah, the other messages are implementor-speak, showing gdb function
> names. We should ideally clean this all up.
Agreed. I can propose a patch later to clean them.
>>
>> 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.
>>
>> 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.
>>
>> 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 <sergiodj@redhat.com>
>>
>> * gdb.texinfo (Linux kernel ptrace restrictions): New appendix
>> section.
>>
>> gdb/ChangeLog:
>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>> Jan Kratochvil <jan.kratochvil@redhat.com>
>>
>> * gdbsupport/gdb-dlfcn.c (gdb_dlopen): Add argument 'dont_throw'
>> Don't throw if it's true.
>> * gdbsupport/gdb-dlfcn.h (gdb_dlopen): Add optional argument
>> 'dont_throw'. Update comment.
>> * 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 (linux_nat_target::attach): Update call to
>> 'linux_ptrace_attach_fail_reason'.
>> (_initialize_linux_nat): Set 'inf_ptrace_me_fail_reason'.
>> * linux-nat.h (linux_nat_target) <ptrace_me_fail_reason>: New
>> method.
>> * 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" and
>> "nat/fork-inferior.h".
>> (selinux_ftype): New typedef.
>> (linux_ptrace_restricted_fail_reason): New function.
>> (linux_ptrace_attach_fail_reason): Add optional 'err'
>> argument. Call 'linux_ptrace_restricted_fail_reason'.
>> (linux_ptrace_me_fail_reason): New function.
>> (linux_child_function): Handle ptrace error.
>> * nat/linux-ptrace.h (linux_ptrace_attach_fail_reason): Update
>> prototype.
>> (linux_ptrace_me_fail_reason): New function.
>> * target-delegates.c: Regenerate.
>> * target.h (struct target_ops) <ptrace_me_fail_reason>: New
>> method.
>> (target_ptrace_me_fail_reason): New define.
>>
>> gdb/gdbserver/ChangeLog:
>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * linux-low.c (linux_ptrace_fun): Call
>> 'linux_ptrace_me_fail_reason'.
>> ---
>> gdb/doc/gdb.texinfo | 138 +++++++++++++++++++++++++++++++++++++
>> gdb/gdbserver/linux-low.c | 3 +-
>> gdb/gdbsupport/gdb-dlfcn.c | 4 +-
>> gdb/gdbsupport/gdb-dlfcn.h | 7 +-
>> gdb/inf-ptrace.c | 19 ++++-
>> gdb/inf-ptrace.h | 10 +++
>> gdb/linux-nat.c | 7 +-
>> gdb/nat/fork-inferior.c | 4 +-
>> gdb/nat/fork-inferior.h | 7 +-
>> gdb/nat/linux-ptrace.c | 86 +++++++++++++++++++++--
>> gdb/nat/linux-ptrace.h | 15 +++-
>> 11 files changed, 282 insertions(+), 18 deletions(-)
>>
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index e1bc8143e6..43f749c6f1 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 @code{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
>> @@ -44682,6 +44685,141 @@ 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} 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 severily restrict the ability to perform these
>
> typo: severily -> severely
Fixed.
>
>
>> +operations, which can make @value{GDBN} 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}
>> +properly.
>> +
>> +@menu
>> +* The GDB error message:: The error message displayed when the
>> + system prevents @value{GDBN} from using
>> + @code{ptrace}
>> +* SELinux's @code{deny_ptrace}:: SELinux and the @code{deny_ptrace} option
>> +* Yama's @code{ptrace_scope}:: Yama and the @code{ptrace_scope} setting
>> +* Docker and @code{seccomp}:: Docker and the @code{seccomp}
>> + infrastructure
>> +@end menu
>> +
>> +@node The GDB error message
>> +@appendixsection The @value{GDBN} error message
>> +
>> +When the system prevents @value{GDBN} 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 GDB
>> +from using 'ptrace'. Please, disable it by executing (as root):
>> +
>> + setsebool deny_ptrace off
>> +
>> +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} 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}. 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 3113017ae6..1e0a5cbf54 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -971,7 +971,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");
>> diff --git a/gdb/gdbsupport/gdb-dlfcn.c b/gdb/gdbsupport/gdb-dlfcn.c
>> index 921f10f3d8..9e5a992c17 100644
>> --- a/gdb/gdbsupport/gdb-dlfcn.c
>> +++ b/gdb/gdbsupport/gdb-dlfcn.c
>> @@ -58,7 +58,7 @@ is_dl_available (void)
>> #else /* NO_SHARED_LIB */
>>
>> gdb_dlhandle_up
>> -gdb_dlopen (const char *filename)
>> +gdb_dlopen (const char *filename, bool dont_throw)
>> {
>> void *result;
>> #ifdef HAVE_DLFCN_H
>> @@ -66,7 +66,7 @@ gdb_dlopen (const char *filename)
>> #elif __MINGW32__
>> result = (void *) LoadLibrary (filename);
>> #endif
>> - if (result != NULL)
>> + if (dont_throw || result != NULL)
>> return gdb_dlhandle_up (result);
>>
>> #ifdef HAVE_DLFCN_H
>> diff --git a/gdb/gdbsupport/gdb-dlfcn.h b/gdb/gdbsupport/gdb-dlfcn.h
>> index 6a39d38941..a8ddbc03da 100644
>> --- a/gdb/gdbsupport/gdb-dlfcn.h
>> +++ b/gdb/gdbsupport/gdb-dlfcn.h
>> @@ -32,10 +32,11 @@ 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. If the loading fails, return NULL if
>> + DONT_THROW is true, or throw an exception otherwise (default
>> + behaviour). */
>>
>> -gdb_dlhandle_up gdb_dlopen (const char *filename);
>> +gdb_dlhandle_up gdb_dlopen (const char *filename, bool dont_throw = false);
>>
>> /* Return the address of the symbol named SYMBOL inside the shared
>> library whose handle is HANDLE. Return NULL when the symbol could
>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
>> index 4a8e732373..b6cfd803cf 100644
>> --- a/gdb/inf-ptrace.c
>> +++ b/gdb/inf-ptrace.c
>> @@ -94,6 +94,22 @@ 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 {};
>> +}
>> +
>> +/* Point to "inf_ptrace_me_fail_reason",
>
> Did you mean "Pointer"? This reads strange because
> "inf_ptrace_me_fail_reason" is the name of the pointer itself,
> so how to point _to_ it?
Yes, I meant "Pointer", thanks. Fixed.
> which implements a function
>> + that can be called by "inf_ptrace_me" in order to obtain the reason
>> + for failure. */
>> +
>> +std::string (*inf_ptrace_me_fail_reason) (int err)
>> + = default_inf_ptrace_me_fail_reason;
>> +
>> /* Prepare to be traced. */
>>
>> static void
>> @@ -101,7 +117,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
>
> Ah, here it's "pointer". But still reads strange. I think the comment
> in the .c file should just say the usual "See inf-ptrace.h.".
Indeed. I'll update the comment there.
>
>> + 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 945c19f666..b5a9eaf72e 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -1191,8 +1191,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 (),
>> @@ -4696,6 +4697,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 68b51aa814..72ac623e20 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 1d0519fb26..7e6b889210 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..599d9cfb55 100644
>> --- a/gdb/nat/linux-ptrace.c
>> +++ b/gdb/nat/linux-ptrace.c
>> @@ -21,6 +21,8 @@
>> #include "linux-procfs.h"
>> #include "linux-waitpid.h"
>> #include "gdbsupport/buffer.h"
>> +#include "gdbsupport/gdb-dlfcn.h"
>> +#include "nat/fork-inferior.h"
>> #ifdef HAVE_SYS_PROCFS_H
>> #include <sys/procfs.h>
>> #endif
>> @@ -30,11 +32,70 @@
>> 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 *);
>> +
>> +/* 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 = gdb_dlopen ("libselinux.so", true);
>> +
>> + if (handle.get () != NULL)
>
> No need for .get() here. This:
>
> if (handle != NULL)
>
> works the same.
While writing this code I thought I remembered something like that, but
I wasn't sure. Thanks for clarifying. Updated.
> (I'd write nullptr throughout instead of NULL in new code.)
OK. Updated.
>> + {
>> + 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'. Please, disable it by executing (as root):\n\
>> +\n\
>> + setsebool deny_ptrace off\n"));
>> + }
>> +
>> + FILE *f = fopen ("/proc/sys/kernel/yama/ptrace_scope", "r");
>> +
>
> Use gdb_fopen_cloexec ?
Ah, I hadn't realized that this existed. Much better, thanks!
>> + if (f != NULL)
>> + {
>> + char yama_scope = fgetc (f);
>> +
>> + fclose (f);
>> +
>> + 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'. Please, disable it by executing (as root):\n\
>> +\n\
>> + echo 0 > /proc/sys/kernel/yama/ptrace_scope\n"));
>> + }
>> +
>> + if (ret.empty ())
>> + 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.\n");
>> +
>> + return ret;
>> +}
>> +
>> +/* See declaration in linux-ptrace.h. */
>>
>> std::string
>> -linux_ptrace_attach_fail_reason (pid_t pid)
>> +linux_ptrace_attach_fail_reason (pid_t pid, int err)
>> {
>> pid_t tracerpid = linux_proc_get_tracerpid_nowarn (pid);
>> std::string result;
>> @@ -50,6 +111,11 @@ linux_ptrace_attach_fail_reason (pid_t pid)
>> "terminated"),
>> (int) pid);
>>
>> + std::string ptrace_restrict = linux_ptrace_restricted_fail_reason (err);
>> +
>> + if (!ptrace_restrict.empty ())
>> + result += "\n" + ptrace_restrict;
>> +
>> return result;
>> }
>>
>> @@ -68,6 +134,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. */
>> @@ -321,7 +395,11 @@ 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);
>> + if (ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0,
>> + (PTRACE_TYPE_ARG4) 0) != 0)
>> + trace_start_error_with_name ("ptrace",
>> + linux_ptrace_me_fail_reason (errno).c_str ());
>> +
>> kill (getpid (), SIGSTOP);
>>
>> /* Fork a grandchild. */
>> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
>> index fd2f12a342..04ada53bf6 100644
>> --- a/gdb/nat/linux-ptrace.h
>> +++ b/gdb/nat/linux-ptrace.h
>> @@ -176,13 +176,26 @@ struct buffer;
>> # define TRAP_HWBKPT 4
>> #endif
>>
>> -extern std::string linux_ptrace_attach_fail_reason (pid_t pid);
>> +/* 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 = -1);
>
> If ERR is an errno number, then it's a bit odd to use -1 for default,
> since errno == 0 is the traditional "no error" number. Pedantically, I believe
> there's no garantee that a valid error number must be a positive integer.
>
> But, why the default argument in the first place? What calls this
> without passing an error?
So, the only place that calls linux_ptrace_attach_fail_reason without
passing the ERR argument is linux_ptrace_attach_fail_reason_string:
std::string
linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err)
{
long lwpid = ptid.lwp ();
std::string reason = linux_ptrace_attach_fail_reason (lwpid);
if (!reason.empty ())
return string_printf ("%s (%d), %s", safe_strerror (err), err,
reason.c_str ());
else
return string_printf ("%s (%d)", safe_strerror (err), err);
}
In this case, I opted to keep it as is because the function will compose
a string contaning like:
A (B)[: C]
Where:
A = safe_strerror
B = errno
C = fail reason (optional)
This function (linux_ptrace_attach_fail_reason_string) is called in
three places:
gdb/linux-nat.c:
std::string reason
= linux_ptrace_attach_fail_reason_string (ptid, err);
warning (_("Cannot attach to lwp %d: %s"),
lwpid, reason.c_str ());
gdb/gdbserver/linux-low.c:
std::string reason
= linux_ptrace_attach_fail_reason_string (ptid, err);
warning (_("Cannot attach to lwp %d: %s"), lwpid, reason.c_str ());
and
std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
It seems to me like these error messages are expecting a short string to
just append to their existing strings, so I didn't think it made much
sense to extend the ptrace error checking here as well. That's why I
didn't extend linux_ptrace_attach_fail_reason_string to pass ERR down to
linux_ptrace_attach_fail_reason.
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
More information about the Gdb-patches
mailing list