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

[Tom Tromey <tom@tromey.com>]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> 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