From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87667 invoked by alias); 26 Sep 2019 18:13:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 87652 invoked by uid 89); 26 Sep 2019 18:13:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=HX-Google-Smtp-Source:APXvYqx, E01, sends, e01 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 26 Sep 2019 18:13:32 +0000 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6BF0A6E77E for ; Thu, 26 Sep 2019 18:13:31 +0000 (UTC) Received: by mail-wr1-f69.google.com with SMTP id q10so1248168wro.22 for ; Thu, 26 Sep 2019 11:13:31 -0700 (PDT) Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id w125sm6988834wmg.32.2019.09.26.11.13.28 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Sep 2019 11:13:29 -0700 (PDT) Subject: Re: [PATCH v5] Improve ptrace-error detection on Linux targets To: Sergio Durigan Junior , GDB Patches References: <20190819032918.3536-1-sergiodj@redhat.com> <20190926042155.31481-1-sergiodj@redhat.com> Cc: Eli Zaretskii , Ruslan Kabatsayev , Tom Tromey From: Pedro Alves Message-ID: <106c1a0b-1bdb-f139-26a1-5ec5d7304d9c@redhat.com> Date: Thu, 26 Sep 2019 18:13:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190926042155.31481-1-sergiodj@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-09/txt/msg00538.txt.bz2 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