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

[Kris Van Hees <kris.van.hees@oracle.com>]

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
>