[DTrace-devel] [PATCH 5/7] proc: improve armouring against self-grabs

[Nick Alcock]

The DTrace victim process-monitoring loop (used by -c in particular, but also
by u*() actions) has some protection to keep DTrace from trying to ptrace()
itself: processes whose PID is the same as DTrace's, or which is already
being traced by DTrace, or which is a system daemon, do not get traced.

But this is not quite good enough. DTrace is a multithreaded process, but we
check the tracer pid (which is a kernel task ID derived from
/proc/$pid/status, i.e. a *thread* ID) against getpid() (which is a POSIX
pid, i.e. a *task group* ID).  We should dig out the task group ID of the
process being traced and compare *that* against our own PID.  (This still
does not stop two distinct dtraces trying to trace each other, but in that
case they shouldn't deadlock: each should sometimes let the other run and
they'll both make some sort of stuttering forward progress.)

To armour things a bit further, we shouldn't try to maintain a Pwait()ing
process-monitoring loop for anything libproc has already said it cannot
ptrace().  It doesn't matter whether we *asked* for it to be noninvasively
traced in the first place: what matters is purely whether we got a ptrace()
grab on it, because that's what controls whether we need to do the whole
waitpid() signal-redirection dance to it.

Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
---
 libdtrace/dt_proc.c | 10 +++++-----
 libproc/Pcontrol.c  | 17 +++++++++++++++++
 libproc/libproc.h   |  1 +
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
index 99ea94d273edc..a052abbacf204 100644
--- a/libdtrace/dt_proc.c
+++ b/libdtrace/dt_proc.c
@@ -945,17 +945,17 @@ dt_proc_control(void *arg)
 		 * Also, obviously enough, never drop breakpoints in ourself!
 		 */
 		if (datap->dpcd_flags & DTRACE_PROC_SHORTLIVED) {
-			pid_t tracer_pid;
+			pid_t tracer_pid, tgid;
 
 			noninvasive = 1;
 			dpr->dpr_notifiable = 0;
 			tracer_pid = Ptracer_pid(dpr->dpr_pid);
+			tgid = Ptgid(dpr->dpr_pid);
 
 			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
 				    dtp->dt_sysslice) > 0) ||
-			    ((tracer_pid != 0) &&
-			     (tracer_pid != getpid())) ||
-			    (dpr->dpr_pid == getpid()))
+			    (tracer_pid == getpid()) ||
+			    (tgid == getpid()))
 				noninvasive = 2;
 		}
 
@@ -971,7 +971,7 @@ dt_proc_control(void *arg)
 		 * the cleanup handlers: the process is running, but does not
 		 * need a monitoring thread.
 		 */
-		if (noninvasive && !Ptraceable(dpr->dpr_proc)) {
+		if (!Ptraceable(dpr->dpr_proc)) {
 			dt_dprintf("%i: noninvasive grab, control thread "
 			    "suiciding\n", dpr->dpr_pid);
 			pthread_exit(NULL);
diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
index 1a19601411c8e..c96a410b70b3f 100644
--- a/libproc/Pcontrol.c
+++ b/libproc/Pcontrol.c
@@ -2837,6 +2837,23 @@ Ptracer_pid(pid_t pid)
 	return tracer_pid;
 }
 
+/*
+ * Get the thread-group ID of the given task (comparable to getpid().
+ */
+pid_t
+Ptgid(pid_t pid)
+{
+	char *txt;
+	pid_t tgid;
+
+	if ((txt = Pget_proc_status(pid, "Tgid")) == NULL)
+		return 0;
+
+	tgid = strtol(txt, NULL, 10);
+	free(txt);
+	return tgid;
+}
+
 /*
  * Return 1 if this process is a system daemon, 0 if it is not, and -1 on error.
  *
diff --git a/libproc/libproc.h b/libproc/libproc.h
index a3163964331cd..59c73051bd85c 100644
--- a/libproc/libproc.h
+++ b/libproc/libproc.h
@@ -102,6 +102,7 @@ extern	int	Pdynamically_linked(struct ps_prochandle *);
 extern	int	Ptraceable(struct ps_prochandle *);
 extern	int	Pelf64(struct ps_prochandle *);
 extern	pid_t	Ptracer_pid(pid_t);
+extern	pid_t	Ptgid(pid_t);
 
 /*
  * Calls that do not take a process structure.  These are used to determine
-- 
2.44.0.273.ge0bd14271f