From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101163 invoked by alias); 26 Sep 2019 18:23: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 101155 invoked by uid 89); 26 Sep 2019 18:23:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy= 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:23:32 +0000 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 79AE2210E; Thu, 26 Sep 2019 18:23:31 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id EF72C1FD3C; Thu, 26 Sep 2019 18:23:30 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Eli Zaretskii , Ruslan Kabatsayev , Tom Tromey Subject: Re: [PATCH v5] Improve ptrace-error detection on Linux targets References: <20190819032918.3536-1-sergiodj@redhat.com> <20190926042155.31481-1-sergiodj@redhat.com> <106c1a0b-1bdb-f139-26a1-5ec5d7304d9c@redhat.com> Date: Thu, 26 Sep 2019 18:23:00 -0000 In-Reply-To: <106c1a0b-1bdb-f139-26a1-5ec5d7304d9c@redhat.com> (Pedro Alves's message of "Thu, 26 Sep 2019 19:13:28 +0100") Message-ID: <8736gij4rh.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00540.txt.bz2 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/