[PATCH 0/5] fix test/unittest/usdt/tst.multitrace.sh

[PATCH 0/5] fix test/unittest/usdt/tst.multitrace.sh

[Nick Alcock]

So Eugene's fix for test/unittest/proc/tst.self-grab.sh caused this test to
pass, even though self-grabbing should have nothing whatsoever to do with
the "multiple dtraces tracing multiple processes" case that
tst.multitrace.sh is testing.  This caused me to look more closely at
Eugene's fix and write something better, but also caused me to finally
figure out what was really going on in tst.multitrace.sh and fix it.

Eugene's self-grabbing fix fixed tst.multitrace.sh by accident: it
accidentally disabled ptrace()-based (invasive) tracing for *everything*,
due to mistakenly assuming that things that were not being debugged were in
fact being debugged by a non-dtrace process.  So first we had to fix that
fix: the underlying problem with self-grabbing was that we had not
considered the case where a process was being traced by some other thread in
*this process*, which is actually commonplace because many libproc
operations are carried out by the main dtrace thread, but ptrace()ing is
done by a distinct, dedicated thread.

Fixing that didn't help tst.multitrace.sh, though, so further digging found
and fixed two other problems, one relating to the victim process terminating
at just the wrong instant (while DTrace was carrying out libproc operations
on it), triggering a coredump, and the other relating to obsolete special-
casing in Ptrace() causing grabbing of processes being debugged by other
processes to be overlooked, with the system thinking we had grabbed it
ourselves when we hadn't (and trying to do ptrace() ops on things we haven't
actually grabbed had painful results).

Most of these bugs date back to 2013, but were very hard to spot before
noninvasive tracing was added, could in any case not be seen unless you had
multiple dtraces tracing the same process, and were hard to spot until the
new USDT dynamic discovery code started calling libproc operations much more
often.

Nick Alcock (5):
  Revert "Tweak self-armouring"
  proc: more self-grab improvements
  libproc: debugging improvements
  libproc: guard against Puntrace() of terminated processes
  libproc: drop Pgrab() special cases in Ptrace()

 libdtrace/dt_proc.c                  | 51 ++++++++++++++---
 libproc/Pcontrol.c                   | 86 +++++++++++++++++++++-------
 libproc/rtld_db.c                    |  2 +-
 test/unittest/usdt/tst.multitrace.sh | 17 +++++-
 4 files changed, 125 insertions(+), 31 deletions(-)

-- 
2.47.1.279.g84c5f4e78e

[PATCH 1/5] Revert "Tweak self-armouring"

[Nick Alcock]

This reverts commit 39cf54d2e98ac877d4b5e5ba6313f717173ca380.
---
 libdtrace/dt_proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
index 7c3eb2a24a512..a052abbacf204 100644
--- a/libdtrace/dt_proc.c
+++ b/libdtrace/dt_proc.c
@@ -954,7 +954,7 @@ dt_proc_control(void *arg)
 
 			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
 				    dtp->dt_sysslice) > 0) ||
-			    (tracer_pid != getpid()) ||
+			    (tracer_pid == getpid()) ||
 			    (tgid == getpid()))
 				noninvasive = 2;
 		}
-- 
2.47.1.279.g84c5f4e78e

Re: [DTrace-devel] [PATCH 1/5] Revert "Tweak self-armouring"

[Kris Van Hees]

On Tue, Dec 03, 2024 at 11:36:06AM +0000, Nick Alcock via DTrace-devel wrote:
> This reverts commit 39cf54d2e98ac877d4b5e5ba6313f717173ca380.

Signed-off-by added

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  libdtrace/dt_proc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
> index 7c3eb2a24a512..a052abbacf204 100644
> --- a/libdtrace/dt_proc.c
> +++ b/libdtrace/dt_proc.c
> @@ -954,7 +954,7 @@ dt_proc_control(void *arg)
>  
>  			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
>  				    dtp->dt_sysslice) > 0) ||
> -			    (tracer_pid != getpid()) ||
> +			    (tracer_pid == getpid()) ||
>  			    (tgid == getpid()))
>  				noninvasive = 2;
>  		}
> -- 
> 2.47.1.279.g84c5f4e78e
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel

[PATCH 2/5] proc: more self-grab improvements

[Nick Alcock]

The self-grab armouring code is clearly too hard to read: the change just
reverted broke it entirely and caused DTrace to never take out self-grabs on
anything (because it misinterpreted processes that were not being debugged
as processes that *were* being debugged: Ptracer_pid() returns zero if
no tracer is active).

Refactor it into something more readable via giving some of the conditions
names.  Doing this forces us to think things through properly.  Some things
can never work reliably and should always be blocked:

 - grabbing the thread doing the tracing
 - grabbing any other of this DTrace instance's threads, given the
   complexity of the proxy-call back-and-forth
 - grabbing a thread being debugged by someone else (a process cannot have
   two tracers at once)
 - grabbing PID 1 (init)

Some things are just a bit risky and are reasonable to do if the user
explicitly asks for it via dtrace -p, but not if it's implicitly requested
by some thread doing a ustack() or something:

 - grabbing a system daemon

Grabbing a thread we have already grabbed is probably impossible from this
location (process-control thread initialization), but if it does happen it's
fine: you can PTRACE_SEIZE the same thread from the same debugger more than
once, that's routine and normal operation for a debugger.

So split things up accordingly, and implement the "any other of DTrace's
threads" case, which was not implemented before: "we grabbed ourself"
i.e. the PID is ourself or the tgid of ourself and the tgid of the PID we're
grabbing are the same; and "someone else is debugging us", i.e.  there is a
tracer in force already and it's not the current thread.

This makes the code *ever* so much easier to read, and makes it possible to
give decent error messages when things go wrong as well.

(The lack of handling of the "any other of our threads" case explains the
tst.multitrace.sh failure: when a shortlived grab from a ustack() etc hits,
the initial grab and release request is issued by the main DTrace thread.
If a shortlived grab hits for the main thread itself, only this case will
prevent the tracer thread from stopping it and then trying to return to the
stopped thread, deadlocking forever.)

This has survived a thousand iterations of test/unittest/proc/tst.self-grab.sh
and test/unittest/usdt/tst.multitrace.sh with no failures (after the other
patches in this series are applied).

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 libdtrace/dt_proc.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
index a052abbacf204..41b148e807f84 100644
--- a/libdtrace/dt_proc.c
+++ b/libdtrace/dt_proc.c
@@ -929,6 +929,9 @@ dt_proc_control(void *arg)
 		dpr->dpr_pid = Pgetpid(dpr->dpr_proc);
 	} else {
 		int noninvasive = 0;
+		int self_grab = 0;
+		int other_tracer = 0;
+		pid_t tracer_pid;
 
 		/*
 		 * "Shortlived" means that the monitoring of this process is not
@@ -942,21 +945,49 @@ dt_proc_control(void *arg)
 		 * else (like another DTrace instance).  No death notification
 		 * is ever sent.
 		 *
-		 * Also, obviously enough, never drop breakpoints in ourself!
+		 * Also, obviously enough, never drop breakpoints in ourself:
+		 * we define that widely enough that no grabs of any thread of
+		 * this DTrace process will be invasive.
+		 *
+		 * If this is *not* a shortlived grab, simply refuse the grab
+		 * if this is being debugged by someone else or is ourself, or
+		 * is PID 1: on explicit request, we'll still grab system
+		 * daemons (if you use dtrace -p, we assume you actually want to
+		 * do what you asked for), but grabs that cannot succeed should
+		 * still be refused.
+		 *
+		 * (If the process is being *debugged* by ourself -- as in
+		 * literally this thread -- we can do invasive grabs just fine.)
 		 */
-		if (datap->dpcd_flags & DTRACE_PROC_SHORTLIVED) {
-			pid_t tracer_pid, tgid;
 
+		tracer_pid = Ptracer_pid(dpr->dpr_pid);
+		self_grab = (dpr->dpr_pid == getpid() ||
+			     Ptgid(dpr->dpr_pid) == (Ptgid(getpid())));
+		other_tracer = (tracer_pid != 0 && tracer_pid != getpid());
+
+		if (datap->dpcd_flags & DTRACE_PROC_SHORTLIVED) {
 			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 == getpid()) ||
-			    (tgid == getpid()))
+			     other_tracer || self_grab)
 				noninvasive = 2;
+		} else {
+			const char *reason;
+
+			if (dpr->dpr_pid == 1 || other_tracer || self_grab) {
+				if (dpr->dpr_pid == 1)
+					reason = "is init";
+				else if (other_tracer)
+					reason = "being traced by someone else";
+				else
+					reason = "PID is ourself";
+
+				dt_proc_error(dtp, dpr, "not safe to stop pid %li for grabbing: %s\n",
+					      (long)dpr->dpr_pid, reason);
+				pthread_exit(NULL);
+			}
 		}
 
 		if ((dpr->dpr_proc = Pgrab(dpr->dpr_pid, noninvasive, 0,
-- 
2.47.1.279.g84c5f4e78e

Re: [PATCH 2/5] proc: more self-grab improvements

[Nick Alcock]

On 3 Dec 2024, Nick Alcock outgrape:
> This has survived a thousand iterations of test/unittest/proc/tst.self-grab.sh
> and test/unittest/usdt/tst.multitrace.sh with no failures (after the other
> patches in this series are applied).

Also survived internals/libproc tests (just making sure I didn't break
grabbing in a more fundamental way):

1300 cases (1100 PASS, 0 FAIL, 0 XPASS, 200 XFAIL, 0 SKIP)

(and survived a more conventional full test run as well).

Alas tst.lmid-consistency.sh is still failing: I must figure out why and
fix it one of these days, now lmids are becoming slightly less
absolutely useless on very modern glibc.

Re: [PATCH 2/5] proc: more self-grab improvements

[Nick Alcock]

On 3 Dec 2024, Nick Alcock uttered the following:

> On 3 Dec 2024, Nick Alcock outgrape:
>> This has survived a thousand iterations of test/unittest/proc/tst.self-grab.sh
>> and test/unittest/usdt/tst.multitrace.sh with no failures (after the other
>> patches in this series are applied).
>
> Also survived internals/libproc tests (just making sure I didn't break
> grabbing in a more fundamental way):
>
> 1300 cases (1100 PASS, 0 FAIL, 0 XPASS, 200 XFAIL, 0 SKIP)
>
> (and survived a more conventional full test run as well).

Survived test runs on the usual 8x OCI systems too.

Did nonstop runs on them overnight of libproc internals tests,
proc/tst.self-grab.sh, and usdt/tst.multitrace.sh. No failures :)
Even proc/tst.signals.sh seems to be consistently passing now,
and that's another unstable-marked nasty test I hadn't got round to
looking at in ever so long...

-- 
NULL && (void)

Re: [DTrace-devel] [PATCH 2/5] proc: more self-grab improvements

[Nick Alcock]

On 5 Dec 2024, Nick Alcock via DTrace-devel spake thusly:
> Even proc/tst.signals.sh seems to be consistently passing now,
> and that's another unstable-marked nasty test I hadn't got round to
> looking at in ever so long...

Never mind: if this one fails once, it starts XFAILing for every
subsequent run, with different failures every time, and seemingly never
passes again. Very strange, given the lack of any way for the process to
pass state between runs (it doesn't rely on dtprobed or anything). Needs
more debugging after all.

-- 
NULL && (void)

Re: [PATCH 2/5] proc: more self-grab improvements

[Kris Van Hees]

On Tue, Dec 03, 2024 at 11:36:07AM +0000, Nick Alcock wrote:
> The self-grab armouring code is clearly too hard to read: the change just
> reverted broke it entirely and caused DTrace to never take out self-grabs on
> anything (because it misinterpreted processes that were not being debugged
> as processes that *were* being debugged: Ptracer_pid() returns zero if
> no tracer is active).
> 
> Refactor it into something more readable via giving some of the conditions
> names.  Doing this forces us to think things through properly.  Some things
> can never work reliably and should always be blocked:
> 
>  - grabbing the thread doing the tracing
>  - grabbing any other of this DTrace instance's threads, given the
>    complexity of the proxy-call back-and-forth
>  - grabbing a thread being debugged by someone else (a process cannot have
>    two tracers at once)
>  - grabbing PID 1 (init)
> 
> Some things are just a bit risky and are reasonable to do if the user
> explicitly asks for it via dtrace -p, but not if it's implicitly requested
> by some thread doing a ustack() or something:
> 
>  - grabbing a system daemon
> 
> Grabbing a thread we have already grabbed is probably impossible from this
> location (process-control thread initialization), but if it does happen it's
> fine: you can PTRACE_SEIZE the same thread from the same debugger more than
> once, that's routine and normal operation for a debugger.
> 
> So split things up accordingly, and implement the "any other of DTrace's
> threads" case, which was not implemented before: "we grabbed ourself"
> i.e. the PID is ourself or the tgid of ourself and the tgid of the PID we're
> grabbing are the same; and "someone else is debugging us", i.e.  there is a
> tracer in force already and it's not the current thread.
> 
> This makes the code *ever* so much easier to read, and makes it possible to
> give decent error messages when things go wrong as well.
> 
> (The lack of handling of the "any other of our threads" case explains the
> tst.multitrace.sh failure: when a shortlived grab from a ustack() etc hits,
> the initial grab and release request is issued by the main DTrace thread.
> If a shortlived grab hits for the main thread itself, only this case will
> prevent the tracer thread from stopping it and then trying to return to the
> stopped thread, deadlocking forever.)
> 
> This has survived a thousand iterations of test/unittest/proc/tst.self-grab.sh
> and test/unittest/usdt/tst.multitrace.sh with no failures (after the other
> patches in this series are applied).
> 
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  libdtrace/dt_proc.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
> index a052abbacf204..41b148e807f84 100644
> --- a/libdtrace/dt_proc.c
> +++ b/libdtrace/dt_proc.c
> @@ -929,6 +929,9 @@ dt_proc_control(void *arg)
>  		dpr->dpr_pid = Pgetpid(dpr->dpr_proc);
>  	} else {
>  		int noninvasive = 0;
> +		int self_grab = 0;
> +		int other_tracer = 0;
> +		pid_t tracer_pid;
>  
>  		/*
>  		 * "Shortlived" means that the monitoring of this process is not
> @@ -942,21 +945,49 @@ dt_proc_control(void *arg)
>  		 * else (like another DTrace instance).  No death notification
>  		 * is ever sent.
>  		 *
> -		 * Also, obviously enough, never drop breakpoints in ourself!
> +		 * Also, obviously enough, never drop breakpoints in ourself:
> +		 * we define that widely enough that no grabs of any thread of
> +		 * this DTrace process will be invasive.
> +		 *
> +		 * If this is *not* a shortlived grab, simply refuse the grab
> +		 * if this is being debugged by someone else or is ourself, or
> +		 * is PID 1: on explicit request, we'll still grab system
> +		 * daemons (if you use dtrace -p, we assume you actually want to
> +		 * do what you asked for), but grabs that cannot succeed should
> +		 * still be refused.
> +		 *
> +		 * (If the process is being *debugged* by ourself -- as in
> +		 * literally this thread -- we can do invasive grabs just fine.)
>  		 */
> -		if (datap->dpcd_flags & DTRACE_PROC_SHORTLIVED) {
> -			pid_t tracer_pid, tgid;
>  
> +		tracer_pid = Ptracer_pid(dpr->dpr_pid);
> +		self_grab = (dpr->dpr_pid == getpid() ||
> +			     Ptgid(dpr->dpr_pid) == (Ptgid(getpid())));
> +		other_tracer = (tracer_pid != 0 && tracer_pid != getpid());
> +
> +		if (datap->dpcd_flags & DTRACE_PROC_SHORTLIVED) {
>  			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 == getpid()) ||
> -			    (tgid == getpid()))
> +			     other_tracer || self_grab)
>  				noninvasive = 2;
> +		} else {
> +			const char *reason;
> +
> +			if (dpr->dpr_pid == 1 || other_tracer || self_grab) {
> +				if (dpr->dpr_pid == 1)
> +					reason = "is init";
> +				else if (other_tracer)
> +					reason = "being traced by someone else";
> +				else
> +					reason = "PID is ourself";
> +
> +				dt_proc_error(dtp, dpr, "not safe to stop pid %li for grabbing: %s\n",
> +					      (long)dpr->dpr_pid, reason);
> +				pthread_exit(NULL);
> +			}
>  		}
>  
>  		if ((dpr->dpr_proc = Pgrab(dpr->dpr_pid, noninvasive, 0,
> -- 
> 2.47.1.279.g84c5f4e78e
> 

[PATCH 3/5] libproc: debugging improvements

[Nick Alcock]

Attempting to track down the intermittent failures in
test/unittest/proc/tst.multitrace.sh is rendered difficult by the fact that
multiple dtraces are running at once, tracing multiple processes, but the
debugging messages emitted by libproc do not provide either the TID of the
tracing thread or the PID of the process being traced in too many cases.

Worse yet, if you do turn debugging on, both dtraces emit debugging output
simultaneously to the same stderr stream.  The interleaving is bad enough,
but very often this causes lines to be emitted to stderr that do not start
with the standard libproc DEBUG time-since-epoch: string, because it got
interspersed into the previous line. runtest then partitions that partial
line off into a separate part of the log, rendering everything entirely
incomprehensible.

So emit more PID-related info, including the TID of the process-control
thread; and arrange for tst.multitrace.sh to capture DTrace debugging output
itself and dump it as two separated pieces: if a dtrace exits nonzero, note
which one exited as well as dumping its debug output.

Also, when this-should-never-happen conditions like a Pwait() returning
-ECHILD happen (usually indicating that we were not the victim's tracer
after all) by dumping the entire /proc/$pid/status of that process to
the debug stream, so we can tell what the tracer *was* and whether the
process was even stopped.  (No impact at all if this condition never
happens, which it never should, of course, or if debugging is off.)

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 libdtrace/dt_proc.c                  |  6 ++-
 libproc/Pcontrol.c                   | 57 +++++++++++++++++++++++++---
 libproc/rtld_db.c                    |  2 +-
 test/unittest/usdt/tst.multitrace.sh | 17 ++++++++-
 4 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
index 41b148e807f84..6ad57cc5f12ef 100644
--- a/libdtrace/dt_proc.c
+++ b/libdtrace/dt_proc.c
@@ -870,6 +870,9 @@ dt_proc_control(void *arg)
 	int err;
 	jmp_buf exec_jmp;
 
+	dt_dprintf("%i: process control thread %i starting.\n", dpr->dpr_pid,
+		   gettid());
+
 	/*
 	 * Set up global libproc hooks that must be active before any processes
 	 * are grabbed or created.
@@ -1421,7 +1424,8 @@ dt_proc_control_cleanup(void *arg)
 	 * out by ptrace() wrappers above us in the call stack, since the whole
 	 * thread is going away.
 	 */
-	dt_dprintf("%i: process control thread going away.\n", dpr->dpr_pid);
+	dt_dprintf("%i: process control thread %i going away.\n", dpr->dpr_pid,
+		   gettid());
 	if(dpr->dpr_lock_count_ctrl == 0 ||
 	    !pthread_equal(dpr->dpr_lock_holder, pthread_self()))
 		dt_proc_lock(dpr);
diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
index c96a410b70b3f..a32a362eaa1fb 100644
--- a/libproc/Pcontrol.c
+++ b/libproc/Pcontrol.c
@@ -66,6 +66,7 @@ static int add_bkpt(struct ps_prochandle *P, uintptr_t addr,
     void *data);
 static void delete_bkpt_handler(struct bkpt *bkpt);
 static jmp_buf **single_thread_unwinder_pad(struct ps_prochandle *unused);
+static void Pdump_proc_status(pid_t pid);
 
 static ptrace_lock_hook_fun *ptrace_lock_hook;
 static waitpid_lock_hook_fun *waitpid_lock_hook;
@@ -332,10 +333,12 @@ Pgrab(pid_t pid, int noninvasiveness, int already_ptraced, void *wrap_arg,
 
 			if (*perr < 0) {
 				if (noninvasiveness < 1) {
+					_dprintf("%i: Pgrab(): not grabbed.\n", P->pid);
 					Pfree_internal(P);
 					return NULL;
 				}
 				close(P->memfd);
+				_dprintf("%i: Pgrab(): grabbed noninvasively.\n", P->pid);
 				noninvasiveness = 2;
 			}
 		} else {
@@ -719,7 +722,9 @@ Pwait_internal(struct ps_prochandle *P, boolean_t block, int *return_early)
 				return 0;
 
 			if (errno == ECHILD) {
+				_dprintf("%i: Pwait: got ECHILD from waitpid(), state %i, trace count %i, halted %i\n", P->pid, P->state, P->ptrace_count, P->ptrace_halted);
 				P->state = PS_DEAD;
+				Pdump_proc_status(P->pid);
 				return 0;
 			}
 
@@ -1262,7 +1267,7 @@ Ppush_state(struct ps_prochandle *P, int state)
 
 	s->state = state;
 	dt_list_prepend(&P->ptrace_states, s);
-	_dprintf("Ppush_state(): ptrace_count %i, state %i\n", P->ptrace_count, s->state);
+	_dprintf("%i: Ppush_state(): ptrace_count %i, state %i\n", P->pid, P->ptrace_count, s->state);
 
 	return s;
 }
@@ -1278,7 +1283,7 @@ Ppop_state(struct ps_prochandle *P)
 
 	s = dt_list_next(&P->ptrace_states);
 	dt_list_delete(&P->ptrace_states, s);
-	_dprintf("Ppop_state(): ptrace_count %i, state %i\n", P->ptrace_count+1, s->state);
+	_dprintf("%i: Ppop_state(): ptrace_count %i, state %i\n", P->pid, P->ptrace_count+1, s->state);
 	state = s->state;
 	free(s);
 	return state;
@@ -1422,6 +1427,9 @@ err_nostate:
 	if (P->ptrace_count == 0 && ptrace_lock_hook)
 		ptrace_lock_hook(P, P->wrap_arg, 0);
 
+	_dprintf("Ptrace(): error return (possibly other tracer), trace count now %i: %s\n",
+		 P->ptrace_count, strerror(errno));
+
 	if (err != -ECHILD)
 		return err;
 	else
@@ -1511,8 +1519,11 @@ Puntrace(struct ps_prochandle *P, int leave_stopped)
 		P->state = PS_RUN;
 		P->ptraced = FALSE;
 		if ((wrapped_ptrace(P, PTRACE_DETACH, P->pid, 0, 0) < 0) &&
-		    (errno == ESRCH))
+		    (errno == ESRCH)) {
+			_dprintf("%i: Punbkpt(): -ESRCH, process is dead.\n",
+				 P->pid);
 			P->state = PS_DEAD;
+		}
 		P->ptrace_halted = FALSE;
 		P->info_valid = 0;
 	}
@@ -1812,7 +1823,7 @@ Punbkpt(struct ps_prochandle *P, uintptr_t addr)
 		if (Preset_bkpt_ip(P, P->tracing_bkpt) < 0)
 			switch (errno) {
 			case ESRCH:
-				_dprintf("%i: -ESRCH, process is dead.\n",
+				_dprintf("%i: Punbkpt(): -ESRCH, process is dead.\n",
 				    P->pid);
 				P->state = PS_DEAD;
 				return;
@@ -1870,7 +1881,8 @@ Punbkpt_child_poke(struct ps_prochandle *P, pid_t pid, bkpt_t *bkpt)
 		bkpt->bkpt_addr, bkpt->orig_insn) < 0)
 		switch (errno) {
 		case ESRCH:
-			_dprintf("%i: -ESRCH, process is dead.\n", child_pid);
+			_dprintf("%i: Punbkpt_child_poke(): -ESRCH, process is dead.\n",
+				 child_pid);
 			if (!pid)
 				P->state = PS_DEAD;
 			return;
@@ -2181,8 +2193,11 @@ Pbkpt_continue(struct ps_prochandle *P)
 		if (wrapped_ptrace(P, PTRACE_CONT, P->pid, 0, 0) < 0) {
 			int err = errno;
 			if (err == ESRCH) {
-				if ((kill(P->pid, 0) < 0) && errno == ESRCH)
+				if ((kill(P->pid, 0) < 0) && errno == ESRCH) {
+					_dprintf("%i: Pbpkt_continue(): Got ESRCH, process is dead.\n",
+						 P->pid);
 					P->state = PS_DEAD;
+				}
 			}
 			/*
 			 * Since we must have an outstanding Ptrace() anyway,
@@ -2760,6 +2775,36 @@ Phastty(pid_t pid)
 	return tty != 0;
 }
 
+/*
+ * Dump /proc/$pid/status into the debug log.
+ */
+static void
+Pdump_proc_status(pid_t pid)
+{
+	char status[PATH_MAX];
+	FILE *fp;
+	char *line = NULL;
+	size_t len;
+
+	snprintf(status, sizeof(status), "/proc/%i/status", pid);
+
+	if ((fp = fopen(status, "r")) == NULL) {
+		_dprintf("Process is dead.\n");
+		return;
+	}
+
+	while (getline(&line, &len, fp) >= 0) {
+		if (strlen(line) > 0) {
+			if (line[strlen(line) - 1] == '\n')
+				line[strlen(line)-1] = '\0';
+			_dprintf("%li: %s\n", (long)pid, line);
+		}
+	}
+	free(line);
+	fclose(fp);
+	return;
+}
+
 /*
  * Get a specific field out of /proc/$pid/status and return the portion after
  * the colon in a new dynamically allocated string, or NULL if no field matches.
diff --git a/libproc/rtld_db.c b/libproc/rtld_db.c
index b6d33ce38425a..9eb63138326dc 100644
--- a/libproc/rtld_db.c
+++ b/libproc/rtld_db.c
@@ -1771,7 +1771,7 @@ rd_loadobj_iter(rd_agent_t *rd, rl_iter_f *fun, void *state)
 	if (rd->P->state == PS_DEAD) {
 		*jmp_pad = old_exec_jmp;
 
-		_dprintf("%i: link map iteration failed: process is dead..\n",
+		_dprintf("%i: link map iteration failed: process is dead.\n",
 		    rd->P->pid);
 		return RD_ERR;
 	}
diff --git a/test/unittest/usdt/tst.multitrace.sh b/test/unittest/usdt/tst.multitrace.sh
index 4e53dbb992508..262c782821ef0 100755
--- a/test/unittest/usdt/tst.multitrace.sh
+++ b/test/unittest/usdt/tst.multitrace.sh
@@ -73,7 +73,7 @@ if [ $? -ne 0 ]; then
 fi
 
 script() {
-	exec $dtrace $dt_flags -qws /dev/stdin $1 $2 $3 <<'EOF'
+	exec $dtrace $dt_flags -qws /dev/stdin $1 $2 $3 2> debug.$3 <<'EOF'
 	int fired[pid_t];
 	int exited[pid_t];
 
@@ -142,13 +142,28 @@ DONE=$!
 script $ONE $TWO 2 &
 DTWO=$!
 
+dump_debug() {
+    if [[ -n $DTRACE_DEBUG ]]; then
+        echo "runtest DEBUG $(date +%s): Debug output of first dtrace so far, PID $DONE" >&2
+        cat debug.1 >&2
+
+        echo "runtest DEBUG $(date +%s): Debug output of second dtrace so far, PID $DTWO" >&2
+        cat debug.2 >&2
+    fi
+}
+
 if ! wait $DONE; then
+    dump_debug
+    echo "first dtrace exited nonzero at $(date +%s)" >&2
     exit 1
 fi
 
 if ! wait $DTWO; then
+    dump_debug
+    echo "second dtrace exited nonzero at $(date +%s)" >&2
     exit 1
 fi
+dump_debug
 
 wait $ONE $TWO
 
-- 
2.47.1.279.g84c5f4e78e

Re: [PATCH 3/5] libproc: debugging improvements

[Kris Van Hees]

On Tue, Dec 03, 2024 at 11:36:08AM +0000, Nick Alcock wrote:
> Attempting to track down the intermittent failures in
> test/unittest/proc/tst.multitrace.sh is rendered difficult by the fact that
> multiple dtraces are running at once, tracing multiple processes, but the
> debugging messages emitted by libproc do not provide either the TID of the
> tracing thread or the PID of the process being traced in too many cases.
> 
> Worse yet, if you do turn debugging on, both dtraces emit debugging output
> simultaneously to the same stderr stream.  The interleaving is bad enough,
> but very often this causes lines to be emitted to stderr that do not start
> with the standard libproc DEBUG time-since-epoch: string, because it got
> interspersed into the previous line. runtest then partitions that partial
> line off into a separate part of the log, rendering everything entirely
> incomprehensible.
> 
> So emit more PID-related info, including the TID of the process-control
> thread; and arrange for tst.multitrace.sh to capture DTrace debugging output
> itself and dump it as two separated pieces: if a dtrace exits nonzero, note
> which one exited as well as dumping its debug output.
> 
> Also, when this-should-never-happen conditions like a Pwait() returning
> -ECHILD happen (usually indicating that we were not the victim's tracer
> after all) by dumping the entire /proc/$pid/status of that process to
> the debug stream, so we can tell what the tracer *was* and whether the
> process was even stopped.  (No impact at all if this condition never
> happens, which it never should, of course, or if debugging is off.)
> 
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  libdtrace/dt_proc.c                  |  6 ++-
>  libproc/Pcontrol.c                   | 57 +++++++++++++++++++++++++---
>  libproc/rtld_db.c                    |  2 +-
>  test/unittest/usdt/tst.multitrace.sh | 17 ++++++++-
>  4 files changed, 73 insertions(+), 9 deletions(-)
> 
> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
> index 41b148e807f84..6ad57cc5f12ef 100644
> --- a/libdtrace/dt_proc.c
> +++ b/libdtrace/dt_proc.c
> @@ -870,6 +870,9 @@ dt_proc_control(void *arg)
>  	int err;
>  	jmp_buf exec_jmp;
>  
> +	dt_dprintf("%i: process control thread %i starting.\n", dpr->dpr_pid,
> +		   gettid());
> +
>  	/*
>  	 * Set up global libproc hooks that must be active before any processes
>  	 * are grabbed or created.
> @@ -1421,7 +1424,8 @@ dt_proc_control_cleanup(void *arg)
>  	 * out by ptrace() wrappers above us in the call stack, since the whole
>  	 * thread is going away.
>  	 */
> -	dt_dprintf("%i: process control thread going away.\n", dpr->dpr_pid);
> +	dt_dprintf("%i: process control thread %i going away.\n", dpr->dpr_pid,
> +		   gettid());
>  	if(dpr->dpr_lock_count_ctrl == 0 ||
>  	    !pthread_equal(dpr->dpr_lock_holder, pthread_self()))
>  		dt_proc_lock(dpr);
> diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
> index c96a410b70b3f..a32a362eaa1fb 100644
> --- a/libproc/Pcontrol.c
> +++ b/libproc/Pcontrol.c
> @@ -66,6 +66,7 @@ static int add_bkpt(struct ps_prochandle *P, uintptr_t addr,
>      void *data);
>  static void delete_bkpt_handler(struct bkpt *bkpt);
>  static jmp_buf **single_thread_unwinder_pad(struct ps_prochandle *unused);
> +static void Pdump_proc_status(pid_t pid);
>  
>  static ptrace_lock_hook_fun *ptrace_lock_hook;
>  static waitpid_lock_hook_fun *waitpid_lock_hook;
> @@ -332,10 +333,12 @@ Pgrab(pid_t pid, int noninvasiveness, int already_ptraced, void *wrap_arg,
>  
>  			if (*perr < 0) {
>  				if (noninvasiveness < 1) {
> +					_dprintf("%i: Pgrab(): not grabbed.\n", P->pid);
>  					Pfree_internal(P);
>  					return NULL;
>  				}
>  				close(P->memfd);
> +				_dprintf("%i: Pgrab(): grabbed noninvasively.\n", P->pid);
>  				noninvasiveness = 2;
>  			}
>  		} else {
> @@ -719,7 +722,9 @@ Pwait_internal(struct ps_prochandle *P, boolean_t block, int *return_early)
>  				return 0;
>  
>  			if (errno == ECHILD) {
> +				_dprintf("%i: Pwait: got ECHILD from waitpid(), state %i, trace count %i, halted %i\n", P->pid, P->state, P->ptrace_count, P->ptrace_halted);
>  				P->state = PS_DEAD;
> +				Pdump_proc_status(P->pid);
>  				return 0;
>  			}
>  
> @@ -1262,7 +1267,7 @@ Ppush_state(struct ps_prochandle *P, int state)
>  
>  	s->state = state;
>  	dt_list_prepend(&P->ptrace_states, s);
> -	_dprintf("Ppush_state(): ptrace_count %i, state %i\n", P->ptrace_count, s->state);
> +	_dprintf("%i: Ppush_state(): ptrace_count %i, state %i\n", P->pid, P->ptrace_count, s->state);
>  
>  	return s;
>  }
> @@ -1278,7 +1283,7 @@ Ppop_state(struct ps_prochandle *P)
>  
>  	s = dt_list_next(&P->ptrace_states);
>  	dt_list_delete(&P->ptrace_states, s);
> -	_dprintf("Ppop_state(): ptrace_count %i, state %i\n", P->ptrace_count+1, s->state);
> +	_dprintf("%i: Ppop_state(): ptrace_count %i, state %i\n", P->pid, P->ptrace_count+1, s->state);
>  	state = s->state;
>  	free(s);
>  	return state;
> @@ -1422,6 +1427,9 @@ err_nostate:
>  	if (P->ptrace_count == 0 && ptrace_lock_hook)
>  		ptrace_lock_hook(P, P->wrap_arg, 0);
>  
> +	_dprintf("Ptrace(): error return (possibly other tracer), trace count now %i: %s\n",
> +		 P->ptrace_count, strerror(errno));
> +
>  	if (err != -ECHILD)
>  		return err;
>  	else
> @@ -1511,8 +1519,11 @@ Puntrace(struct ps_prochandle *P, int leave_stopped)
>  		P->state = PS_RUN;
>  		P->ptraced = FALSE;
>  		if ((wrapped_ptrace(P, PTRACE_DETACH, P->pid, 0, 0) < 0) &&
> -		    (errno == ESRCH))
> +		    (errno == ESRCH)) {
> +			_dprintf("%i: Punbkpt(): -ESRCH, process is dead.\n",
> +				 P->pid);
>  			P->state = PS_DEAD;
> +		}
>  		P->ptrace_halted = FALSE;
>  		P->info_valid = 0;
>  	}
> @@ -1812,7 +1823,7 @@ Punbkpt(struct ps_prochandle *P, uintptr_t addr)
>  		if (Preset_bkpt_ip(P, P->tracing_bkpt) < 0)
>  			switch (errno) {
>  			case ESRCH:
> -				_dprintf("%i: -ESRCH, process is dead.\n",
> +				_dprintf("%i: Punbkpt(): -ESRCH, process is dead.\n",
>  				    P->pid);
>  				P->state = PS_DEAD;
>  				return;
> @@ -1870,7 +1881,8 @@ Punbkpt_child_poke(struct ps_prochandle *P, pid_t pid, bkpt_t *bkpt)
>  		bkpt->bkpt_addr, bkpt->orig_insn) < 0)
>  		switch (errno) {
>  		case ESRCH:
> -			_dprintf("%i: -ESRCH, process is dead.\n", child_pid);
> +			_dprintf("%i: Punbkpt_child_poke(): -ESRCH, process is dead.\n",
> +				 child_pid);
>  			if (!pid)
>  				P->state = PS_DEAD;
>  			return;
> @@ -2181,8 +2193,11 @@ Pbkpt_continue(struct ps_prochandle *P)
>  		if (wrapped_ptrace(P, PTRACE_CONT, P->pid, 0, 0) < 0) {
>  			int err = errno;
>  			if (err == ESRCH) {
> -				if ((kill(P->pid, 0) < 0) && errno == ESRCH)
> +				if ((kill(P->pid, 0) < 0) && errno == ESRCH) {
> +					_dprintf("%i: Pbpkt_continue(): Got ESRCH, process is dead.\n",
> +						 P->pid);
>  					P->state = PS_DEAD;
> +				}
>  			}
>  			/*
>  			 * Since we must have an outstanding Ptrace() anyway,
> @@ -2760,6 +2775,36 @@ Phastty(pid_t pid)
>  	return tty != 0;
>  }
>  
> +/*
> + * Dump /proc/$pid/status into the debug log.
> + */
> +static void
> +Pdump_proc_status(pid_t pid)
> +{
> +	char status[PATH_MAX];
> +	FILE *fp;
> +	char *line = NULL;
> +	size_t len;
> +
> +	snprintf(status, sizeof(status), "/proc/%i/status", pid);
> +
> +	if ((fp = fopen(status, "r")) == NULL) {
> +		_dprintf("Process is dead.\n");
> +		return;
> +	}
> +
> +	while (getline(&line, &len, fp) >= 0) {
> +		if (strlen(line) > 0) {
> +			if (line[strlen(line) - 1] == '\n')
> +				line[strlen(line)-1] = '\0';
> +			_dprintf("%li: %s\n", (long)pid, line);
> +		}
> +	}
> +	free(line);
> +	fclose(fp);
> +	return;
> +}
> +
>  /*
>   * Get a specific field out of /proc/$pid/status and return the portion after
>   * the colon in a new dynamically allocated string, or NULL if no field matches.
> diff --git a/libproc/rtld_db.c b/libproc/rtld_db.c
> index b6d33ce38425a..9eb63138326dc 100644
> --- a/libproc/rtld_db.c
> +++ b/libproc/rtld_db.c
> @@ -1771,7 +1771,7 @@ rd_loadobj_iter(rd_agent_t *rd, rl_iter_f *fun, void *state)
>  	if (rd->P->state == PS_DEAD) {
>  		*jmp_pad = old_exec_jmp;
>  
> -		_dprintf("%i: link map iteration failed: process is dead..\n",
> +		_dprintf("%i: link map iteration failed: process is dead.\n",
>  		    rd->P->pid);
>  		return RD_ERR;
>  	}
> diff --git a/test/unittest/usdt/tst.multitrace.sh b/test/unittest/usdt/tst.multitrace.sh
> index 4e53dbb992508..262c782821ef0 100755
> --- a/test/unittest/usdt/tst.multitrace.sh
> +++ b/test/unittest/usdt/tst.multitrace.sh
> @@ -73,7 +73,7 @@ if [ $? -ne 0 ]; then
>  fi
>  
>  script() {
> -	exec $dtrace $dt_flags -qws /dev/stdin $1 $2 $3 <<'EOF'
> +	exec $dtrace $dt_flags -qws /dev/stdin $1 $2 $3 2> debug.$3 <<'EOF'
>  	int fired[pid_t];
>  	int exited[pid_t];
>  
> @@ -142,13 +142,28 @@ DONE=$!
>  script $ONE $TWO 2 &
>  DTWO=$!
>  
> +dump_debug() {
> +    if [[ -n $DTRACE_DEBUG ]]; then
> +        echo "runtest DEBUG $(date +%s): Debug output of first dtrace so far, PID $DONE" >&2
> +        cat debug.1 >&2
> +
> +        echo "runtest DEBUG $(date +%s): Debug output of second dtrace so far, PID $DTWO" >&2
> +        cat debug.2 >&2
> +    fi
> +}
> +
>  if ! wait $DONE; then
> +    dump_debug
> +    echo "first dtrace exited nonzero at $(date +%s)" >&2
>      exit 1
>  fi
>  
>  if ! wait $DTWO; then
> +    dump_debug
> +    echo "second dtrace exited nonzero at $(date +%s)" >&2
>      exit 1
>  fi
> +dump_debug
>  
>  wait $ONE $TWO
>  
> -- 
> 2.47.1.279.g84c5f4e78e
> 

[PATCH 4/5] libproc: guard against Puntrace() of terminated processes

[Nick Alcock]

If processes terminate while the main dtrace thread is doing something in
libproc, the process-control thread will clean up, releasing all resources,
including cancelling all ptraces.  Unfortunately if the main thread is in
the middle of a Ptrace()-related operation at the time, it will finish off
by doing a balancing Puntrace().  This is of course now unbalanced, because
the process cleanup did all the Puntrace()s for us; it will then try to pop
a state vector that has already been freed, yielding a crash that looks like
this:

    at libproc/rtld_db.c:1934
    at libdtrace/dt_pid.c:987
    at libdtrace/dt_pid.c:1265
    rfunc=0x40419e <chewrec>, arg=0x0) at libdtrace/dt_work.c:377

(This can also kick in when DTrace erroneously considers a process dead even
though it isn't, which is actually what happened here: we fix that in a
later commit.)

Fixed by simply checking to see if the process has been Prelease()d in
Puntrace(), and returning early.  The process is released and all
Puntrace()s have already been done: there is nothing left to do.

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 libproc/Pcontrol.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
index a32a362eaa1fb..6a454ef86bc3b 100644
--- a/libproc/Pcontrol.c
+++ b/libproc/Pcontrol.c
@@ -1455,12 +1455,22 @@ Puntrace(struct ps_prochandle *P, int leave_stopped)
 	int prev_state;
 
 	/*
-	 * Protect against unbalanced Ptrace()/Puntrace().
+	 * Protect against unbalanced Ptrace()/Puntrace() and already-
+	 * terminated processes; operations interrupted by process termination
+	 * might reasonably do a Puntrace() to balance out a previous Ptrace(),
+	 * but everything is freed and we just want to drop out after balancing
+	 * the ptrace() count.
 	 */
 	if ((!P->ptraced) || (P->ptrace_count == 0))
 		return;
 
 	P->ptrace_count--;
+
+	if (P->released) {
+		_dprintf("%i: Puntrace(): early return, process is released\n", P->pid);
+		return;
+	}
+
 	prev_state = Ppop_state(P);
 
 	/*
-- 
2.47.1.279.g84c5f4e78e

Re: [DTrace-devel] [PATCH 4/5] libproc: guard against Puntrace() of terminated processes

[Nick Alcock]

On 3 Dec 2024, Nick Alcock via DTrace-devel spake thusly:

> If processes terminate while the main dtrace thread is doing something in
> libproc, the process-control thread will clean up, releasing all resources,
> including cancelling all ptraces.  Unfortunately if the main thread is in
> the middle of a Ptrace()-related operation at the time, it will finish off
> by doing a balancing Puntrace().  This is of course now unbalanced, because
> the process cleanup did all the Puntrace()s for us; it will then try to pop
> a state vector that has already been freed, yielding a crash that looks like
> this:
>
>     at libproc/rtld_db.c:1934
>     at libdtrace/dt_pid.c:987
>     at libdtrace/dt_pid.c:1265
>     rfunc=0x40419e <chewrec>, arg=0x0) at libdtrace/dt_work.c:377

Oh yuck, what the hell happened here?

... gdb backtraces start with a # character and git filtered it out!

Fixed commit coming right away.

-- 
NULL && (void)

[PATCH 4/5] libproc: guard against Puntrace() of terminated processes

[Nick Alcock]

If processes terminate while the main dtrace thread is doing something in
libproc, the process-control thread will clean up, releasing all resources,
including cancelling all ptraces.  Unfortunately if the main thread is in
the middle of a Ptrace()-related operation at the time, it will finish off
by doing a balancing Puntrace().  This is of course now unbalanced, because
the process cleanup did all the Puntrace()s for us; it will then try to pop
a state vector that has already been freed, yielding a crash that looks like
this:

 #0  0x00007f55dbe8035f in dt_list_delete (dlp=0x7f55d0001428, existing=0x0) at libcommon/dt_list.c:81
 #1  0x00007f55dbe8239b in Ppop_state (P=0x7f55d0001410) at libproc/Pcontrol.c:1280
 #2  0x00007f55dbe827fb in Puntrace (P=0x7f55d0001410, leave_stopped=0) at libproc/Pcontrol.c:1456
 #3  0x00007f55dbe8bffd in rd_ldso_consistent_end (rd=0x7f55d00046e0) at libproc/rtld_db.c:1113
 #4  0x00007f55dbe8d5d8 in rd_loadobj_iter (rd=0x7f55d00046e0, fun=0x7f55dbe863cb <map_iter>, state=0x7f55d0001410)
     at libproc/rtld_db.c:1934
 #5  0x00007f55dbe876d3 in Pupdate_lmids (P=0x7f55d0001410) at libproc/Psymtab.c:813
 #6  0x00007f55dbe87827 in Paddr_to_map (P=0x7f55d0001410, addr=4199075) at libproc/Psymtab.c:883
 #7  0x00007f55dbe5354c in dt_pid_create_usdt_probes_proc (dtp=0x1a47ebb0, dpr=0x29234ea0, pdp=0x7fff392bb090, pcb=0x7fff392bb170)
     at libdtrace/dt_pid.c:987
 #8  0x00007f55dbe54056 in dt_pid_create_usdt_probes (pdp=0x2ac157c0, dtp=0x1a47ebb0, pcb=0x7fff392bb170)
     at libdtrace/dt_pid.c:1265
 #9  0x00007f55dbe71ce2 in discover (dtp=0x1a47ebb0) at libdtrace/dt_prov_uprobe.c:520
 #10 0x00007f55dbe747a2 in dt_provider_discover (dtp=0x1a47ebb0) at libdtrace/dt_provider.c:183
 #11 0x00007f55dbe7c1b1 in dtrace_work (dtp=0x1a47ebb0, fp=0x7f55dbcfc780 <_IO_2_1_stdout_>, pfunc=0x404211 <chew>,
     rfunc=0x40419e <chewrec>, arg=0x0) at libdtrace/dt_work.c:377
 #12 0x00000000004066d5 in main (argc=11, argv=0x7fff392bb7b8) at cmd/dtrace.c:1556

(This can also kick in when DTrace erroneously considers a process dead even
though it isn't, which is actually what happened here: we fix that in a
later commit.)

Fixed by simply checking to see if the process has been Prelease()d in
Puntrace(), and returning early.  The process is released and all
Puntrace()s have already been done: there is nothing left to do.

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 libproc/Pcontrol.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
index a32a362eaa1fb..6a454ef86bc3b 100644
--- a/libproc/Pcontrol.c
+++ b/libproc/Pcontrol.c
@@ -1455,12 +1455,22 @@ Puntrace(struct ps_prochandle *P, int leave_stopped)
 	int prev_state;
 
 	/*
-	 * Protect against unbalanced Ptrace()/Puntrace().
+	 * Protect against unbalanced Ptrace()/Puntrace() and already-
+	 * terminated processes; operations interrupted by process termination
+	 * might reasonably do a Puntrace() to balance out a previous Ptrace(),
+	 * but everything is freed and we just want to drop out after balancing
+	 * the ptrace() count.
 	 */
 	if ((!P->ptraced) || (P->ptrace_count == 0))
 		return;
 
 	P->ptrace_count--;
+
+	if (P->released) {
+		_dprintf("%i: Puntrace(): early return, process is released\n", P->pid);
+		return;
+	}
+
 	prev_state = Ppop_state(P);
 
 	/*
-- 
2.47.1.279.g84c5f4e78e

Re: [PATCH 4/5] libproc: guard against Puntrace() of terminated processes

[Kris Van Hees]

On Tue, Dec 03, 2024 at 06:09:46PM +0000, Nick Alcock wrote:
> If processes terminate while the main dtrace thread is doing something in
> libproc, the process-control thread will clean up, releasing all resources,
> including cancelling all ptraces.  Unfortunately if the main thread is in
> the middle of a Ptrace()-related operation at the time, it will finish off
> by doing a balancing Puntrace().  This is of course now unbalanced, because
> the process cleanup did all the Puntrace()s for us; it will then try to pop
> a state vector that has already been freed, yielding a crash that looks like
> this:
> 
>  #0  0x00007f55dbe8035f in dt_list_delete (dlp=0x7f55d0001428, existing=0x0) at libcommon/dt_list.c:81
>  #1  0x00007f55dbe8239b in Ppop_state (P=0x7f55d0001410) at libproc/Pcontrol.c:1280
>  #2  0x00007f55dbe827fb in Puntrace (P=0x7f55d0001410, leave_stopped=0) at libproc/Pcontrol.c:1456
>  #3  0x00007f55dbe8bffd in rd_ldso_consistent_end (rd=0x7f55d00046e0) at libproc/rtld_db.c:1113
>  #4  0x00007f55dbe8d5d8 in rd_loadobj_iter (rd=0x7f55d00046e0, fun=0x7f55dbe863cb <map_iter>, state=0x7f55d0001410)
>      at libproc/rtld_db.c:1934
>  #5  0x00007f55dbe876d3 in Pupdate_lmids (P=0x7f55d0001410) at libproc/Psymtab.c:813
>  #6  0x00007f55dbe87827 in Paddr_to_map (P=0x7f55d0001410, addr=4199075) at libproc/Psymtab.c:883
>  #7  0x00007f55dbe5354c in dt_pid_create_usdt_probes_proc (dtp=0x1a47ebb0, dpr=0x29234ea0, pdp=0x7fff392bb090, pcb=0x7fff392bb170)
>      at libdtrace/dt_pid.c:987
>  #8  0x00007f55dbe54056 in dt_pid_create_usdt_probes (pdp=0x2ac157c0, dtp=0x1a47ebb0, pcb=0x7fff392bb170)
>      at libdtrace/dt_pid.c:1265
>  #9  0x00007f55dbe71ce2 in discover (dtp=0x1a47ebb0) at libdtrace/dt_prov_uprobe.c:520
>  #10 0x00007f55dbe747a2 in dt_provider_discover (dtp=0x1a47ebb0) at libdtrace/dt_provider.c:183
>  #11 0x00007f55dbe7c1b1 in dtrace_work (dtp=0x1a47ebb0, fp=0x7f55dbcfc780 <_IO_2_1_stdout_>, pfunc=0x404211 <chew>,
>      rfunc=0x40419e <chewrec>, arg=0x0) at libdtrace/dt_work.c:377
>  #12 0x00000000004066d5 in main (argc=11, argv=0x7fff392bb7b8) at cmd/dtrace.c:1556
> 
> (This can also kick in when DTrace erroneously considers a process dead even
> though it isn't, which is actually what happened here: we fix that in a
> later commit.)
> 
> Fixed by simply checking to see if the process has been Prelease()d in
> Puntrace(), and returning early.  The process is released and all
> Puntrace()s have already been done: there is nothing left to do.
> 
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  libproc/Pcontrol.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
> index a32a362eaa1fb..6a454ef86bc3b 100644
> --- a/libproc/Pcontrol.c
> +++ b/libproc/Pcontrol.c
> @@ -1455,12 +1455,22 @@ Puntrace(struct ps_prochandle *P, int leave_stopped)
>  	int prev_state;
>  
>  	/*
> -	 * Protect against unbalanced Ptrace()/Puntrace().
> +	 * Protect against unbalanced Ptrace()/Puntrace() and already-
> +	 * terminated processes; operations interrupted by process termination
> +	 * might reasonably do a Puntrace() to balance out a previous Ptrace(),
> +	 * but everything is freed and we just want to drop out after balancing
> +	 * the ptrace() count.
>  	 */
>  	if ((!P->ptraced) || (P->ptrace_count == 0))
>  		return;
>  
>  	P->ptrace_count--;
> +
> +	if (P->released) {
> +		_dprintf("%i: Puntrace(): early return, process is released\n", P->pid);
> +		return;
> +	}
> +
>  	prev_state = Ppop_state(P);
>  
>  	/*
> -- 
> 2.47.1.279.g84c5f4e78e
> 

[PATCH 5/5] libproc: drop Pgrab() special cases in Ptrace()

[Nick Alcock]

Way back in 2013, in commit f5f05eb28058f2a62efeefef7c5faeca62b09578, we
added a special case to Ptrace() causing it to not fail with an error
if ptrace() failed and Ptrace() was being called by Pgrab().

The need for this is long past: noninvasive tracing provides the semantics
this change was meant to provide, far less unpleasantly.  Worse yet, the
patch is not threadsafe (even though we can have arbitrarily many threads
monitoring arbitrarily many processes), and worse yet, the noninvasive
tracing support in Pgrab() wants to *detect* failure to ptrace() so we
can switch to tracing noninvasively instead.  If the failure is hidden,
we assume ptrace() has worked, and our first attempt to use this and
waitpid() on the traced child fails with an -ECHILD and causes us to
assume the process dead.  Since it's not dead, bad things happen:

libproc DEBUG 1733155118: 386060: Ppush_state(): ptrace_count 1, state 1
libproc DEBUG 1733155118: 386060: Ppop_state(): ptrace_count 2, state 1
libproc DEBUG 1733155118: Pgrab: grabbed PID 386060.
[...]
libproc DEBUG 1733155118: 386060: Activated rtld_db agent.
libproc DEBUG 1733155118: 386060: link map iteration failed: process is dead.
libdtrace DEBUG 1733155118: Called dt_attach() with attach_time 0
libdtrace DEBUG 1733155118: pid 386060: dropping breakpoint on AT_ENTRY
libproc DEBUG 1733155118: 386060: Ppush_state(): ptrace_count 1, state 4
libproc DEBUG 1733155118: 386060: Ppop_state(): ptrace_count 2, state 4
libproc DEBUG 1733155118: 386060: Cannot add breakpoint on ffffffffffffffff: Operation not permitted
libdtrace DEBUG 1733155118: Cannot drop breakpoint in child process: acting as if evaltime=exec were in force.

(Note that we weren't even logging the fact that Pgrab() had failed, up ther
before the [...], and the first visible failure happened some time later,
with entirely inaccurate messages about processes being dead and the like.)

The solution is simple: take out the whole horrible Pgrab() special case,
and treat invocations of Ptrace() from Pgrab() just like any other
invocation from anywhere else.  Pgrab() already deals with failure-to-grab
errors perfectly well, if we only let it see the errors at all.

With this in place, test/unittest/usdt/tst.multitrace.sh survives 200+
invocations with zero failures.

Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
 libproc/Pcontrol.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
index 6a454ef86bc3b..7d9b5055f8201 100644
--- a/libproc/Pcontrol.c
+++ b/libproc/Pcontrol.c
@@ -39,8 +39,6 @@
 
 char	procfs_path[PATH_MAX] = "/proc";
 
-static	int Pgrabbing = 0; 	/* A Pgrab() is underway. */
-
 static int systemd_system = -1; /* 1 if this is a system running systemd. */
 
 static void Pfree_internal(struct ps_prochandle *P);
@@ -327,9 +325,7 @@ Pgrab(pid_t pid, int noninvasiveness, int already_ptraced, void *wrap_arg,
 			/*
 			 * Pmemfd() grabbed, try to ptrace().
 			 */
-			Pgrabbing = 1;
 			*perr = Ptrace(P, 1);
-			Pgrabbing = 0;
 
 			if (*perr < 0) {
 				if (noninvasiveness < 1) {
@@ -1373,10 +1369,7 @@ Ptrace(struct ps_prochandle *P, int stopped)
 
 	if (wrapped_ptrace(P, PTRACE_SEIZE, P->pid, 0, LIBPROC_PTRACE_OPTIONS |
 		    PTRACE_O_TRACECLONE) < 0) {
-		if (!Pgrabbing)
-			goto err;
-		else
-			goto err2;
+		goto err;
 	}
 
 	P->ptraced = TRUE;
@@ -1386,10 +1379,7 @@ Ptrace(struct ps_prochandle *P, int stopped)
 
 		if (wrapped_ptrace(P, PTRACE_INTERRUPT, P->pid, 0, 0) < 0) {
 			wrapped_ptrace(P, PTRACE_DETACH, P->pid, 0, 0);
-			if (!Pgrabbing)
-				goto err;
-			else
-				goto err2;
+			goto err;
 		}
 
 		/*
@@ -1406,14 +1396,13 @@ Ptrace(struct ps_prochandle *P, int stopped)
 		if ((P->state != PS_TRACESTOP) &&
 		    (P->state != PS_STOP)) {
 			err = -ECHILD;
-			goto err2;
+			goto err;
 		}
 	}
 
 	return err;
 err:
 	err = -errno;
-err2:
 	/*
 	 * Note a subtlety here: the Ptrace_count may have been reduced, and the state
 	 * popped to match, by an exec() or other operation within the Pwait().
-- 
2.47.1.279.g84c5f4e78e

Re: [PATCH 5/5] libproc: drop Pgrab() special cases in Ptrace()

[Kris Van Hees]

On Tue, Dec 03, 2024 at 11:36:10AM +0000, Nick Alcock wrote:
> Way back in 2013, in commit f5f05eb28058f2a62efeefef7c5faeca62b09578, we
> added a special case to Ptrace() causing it to not fail with an error
> if ptrace() failed and Ptrace() was being called by Pgrab().
> 
> The need for this is long past: noninvasive tracing provides the semantics
> this change was meant to provide, far less unpleasantly.  Worse yet, the
> patch is not threadsafe (even though we can have arbitrarily many threads
> monitoring arbitrarily many processes), and worse yet, the noninvasive
> tracing support in Pgrab() wants to *detect* failure to ptrace() so we
> can switch to tracing noninvasively instead.  If the failure is hidden,
> we assume ptrace() has worked, and our first attempt to use this and
> waitpid() on the traced child fails with an -ECHILD and causes us to
> assume the process dead.  Since it's not dead, bad things happen:
> 
> libproc DEBUG 1733155118: 386060: Ppush_state(): ptrace_count 1, state 1
> libproc DEBUG 1733155118: 386060: Ppop_state(): ptrace_count 2, state 1
> libproc DEBUG 1733155118: Pgrab: grabbed PID 386060.
> [...]
> libproc DEBUG 1733155118: 386060: Activated rtld_db agent.
> libproc DEBUG 1733155118: 386060: link map iteration failed: process is dead.
> libdtrace DEBUG 1733155118: Called dt_attach() with attach_time 0
> libdtrace DEBUG 1733155118: pid 386060: dropping breakpoint on AT_ENTRY
> libproc DEBUG 1733155118: 386060: Ppush_state(): ptrace_count 1, state 4
> libproc DEBUG 1733155118: 386060: Ppop_state(): ptrace_count 2, state 4
> libproc DEBUG 1733155118: 386060: Cannot add breakpoint on ffffffffffffffff: Operation not permitted
> libdtrace DEBUG 1733155118: Cannot drop breakpoint in child process: acting as if evaltime=exec were in force.
> 
> (Note that we weren't even logging the fact that Pgrab() had failed, up ther
> before the [...], and the first visible failure happened some time later,
> with entirely inaccurate messages about processes being dead and the like.)
> 
> The solution is simple: take out the whole horrible Pgrab() special case,
> and treat invocations of Ptrace() from Pgrab() just like any other
> invocation from anywhere else.  Pgrab() already deals with failure-to-grab
> errors perfectly well, if we only let it see the errors at all.
> 
> With this in place, test/unittest/usdt/tst.multitrace.sh survives 200+
> invocations with zero failures.
> 
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  libproc/Pcontrol.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
> index 6a454ef86bc3b..7d9b5055f8201 100644
> --- a/libproc/Pcontrol.c
> +++ b/libproc/Pcontrol.c
> @@ -39,8 +39,6 @@
>  
>  char	procfs_path[PATH_MAX] = "/proc";
>  
> -static	int Pgrabbing = 0; 	/* A Pgrab() is underway. */
> -
>  static int systemd_system = -1; /* 1 if this is a system running systemd. */
>  
>  static void Pfree_internal(struct ps_prochandle *P);
> @@ -327,9 +325,7 @@ Pgrab(pid_t pid, int noninvasiveness, int already_ptraced, void *wrap_arg,
>  			/*
>  			 * Pmemfd() grabbed, try to ptrace().
>  			 */
> -			Pgrabbing = 1;
>  			*perr = Ptrace(P, 1);
> -			Pgrabbing = 0;
>  
>  			if (*perr < 0) {
>  				if (noninvasiveness < 1) {
> @@ -1373,10 +1369,7 @@ Ptrace(struct ps_prochandle *P, int stopped)
>  
>  	if (wrapped_ptrace(P, PTRACE_SEIZE, P->pid, 0, LIBPROC_PTRACE_OPTIONS |
>  		    PTRACE_O_TRACECLONE) < 0) {
> -		if (!Pgrabbing)
> -			goto err;
> -		else
> -			goto err2;
> +		goto err;
>  	}
>  
>  	P->ptraced = TRUE;
> @@ -1386,10 +1379,7 @@ Ptrace(struct ps_prochandle *P, int stopped)
>  
>  		if (wrapped_ptrace(P, PTRACE_INTERRUPT, P->pid, 0, 0) < 0) {
>  			wrapped_ptrace(P, PTRACE_DETACH, P->pid, 0, 0);
> -			if (!Pgrabbing)
> -				goto err;
> -			else
> -				goto err2;
> +			goto err;
>  		}
>  
>  		/*
> @@ -1406,14 +1396,13 @@ Ptrace(struct ps_prochandle *P, int stopped)
>  		if ((P->state != PS_TRACESTOP) &&
>  		    (P->state != PS_STOP)) {
>  			err = -ECHILD;
> -			goto err2;
> +			goto err;
>  		}
>  	}
>  
>  	return err;
>  err:
>  	err = -errno;
> -err2:
>  	/*
>  	 * Note a subtlety here: the Ptrace_count may have been reduced, and the state
>  	 * popped to match, by an exec() or other operation within the Pwait().
> -- 
> 2.47.1.279.g84c5f4e78e
>