[DTrace-devel] [PATCH v3 09/21] proc: don't ptrace shortlived-grabbed processes if someone else is ptracing them

[Kris Van Hees]

On Tue, Jan 16, 2024 at 09:13:05PM +0000, Nick Alcock wrote:
> Shortlived grabs, by definition, are grabs that won't last long and where we
> don't expect things like dlopen monitoring to be useful.  We ptrace for
> this, and to extract dynamic loader scope info and take things like symbol
> interposition into accout when doing symbol lookup; but if someone else is
> already tracing the process we should probably just try not ptracing it,
> since the worst that will happen if we do is that some name lookups of
> interposed symbols or symbols in alternate dlmopen namespaces might yield
> the wrong address (probably leading to a probe not firing when it
> should).
> 
> Interposed symbols are rare enough that this failure mode is probably not
> justification for refusing to trace entirely (perhaps a warning message
> might be desirable, but honestly I think it's rare enough that not even that
> is justifiable: most ptrace failures will have no visible negative
> consequences at all, since not only must an interposed symbol exist but we
> must also be tracing it *and* tracing the process with multiple debuggers at
> once, which seems likely to be very rare: nearly all warnings will be
> warning when nothing will go wrong at all).
> 
> It is even rarer in the modern toolchain world in which many ELF objects
> cannot be interposed at all, whether due to -z now or
> -fno-semantic-interposition or whatever.
> 
> Doing this makes it practical to have multiple dtraces USDT-probing the same
> process at the same time: one of them will ptrace it, the other one won't,
> and both will get the mapping info they need for a
> reliable-enough-to-be-useful trace (and if we're lucky and can ptrace,
> symbol resolution quality goes up a bit).
> 
> test/unittest/usdt/tst.multitrace.sh tests this (though there are other
> things stopping it working well, it cannot work at all without something
> like this).
> 
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>

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

> ---
>  libdtrace/dt_proc.c | 16 ++++++++++++----
>  libproc/Pcontrol.c  | 18 ++++++++++++++++++
>  libproc/libproc.h   |  1 +
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
> index dd5f69a60e17..73f35135a7b6 100644
> --- a/libdtrace/dt_proc.c
> +++ b/libdtrace/dt_proc.c
> @@ -931,16 +931,24 @@ dt_proc_control(void *arg)
>  		 * be worth ptracing it so that we get better symbol resolution,
>  		 * but if the process is a crucial system daemon, avoid ptracing
>  		 * it entirely, to avoid rtld_db dropping breakpoints in crucial
> -		 * system daemons unless specifically demanded.  No death
> -		 * notification is ever sent.
> +		 * system daemons unless specifically demanded.  Also avoid
> +		 * ptracing if the process is already being traced by someone
> +		 * else (like another DTrace instance).  No death notification
> +		 * is ever sent.
>  		 *
>  		 * Also, obviously enough, never drop breakpoints in ourself!
>  		 */
>  		if (datap->dpcd_flags & DTRACE_PROC_SHORTLIVED) {
> -			noninvasive = 1;
> +			pid_t tracer_pid;
> +
> +                        noninvasive = 1;
>  			dpr->dpr_notifiable = 0;
> -			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
> +			tracer_pid = Ptracer_pid(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()))
>  				noninvasive = 2;
>  		}
> diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
> index eb450bdc2981..68b372da62eb 100644
> --- a/libproc/Pcontrol.c
> +++ b/libproc/Pcontrol.c
> @@ -2811,6 +2811,24 @@ Puid(pid_t pid)
>  	return uid;
>  }
>  
> +/*
> + * Determine if this process is already being ptraced, and if so, the pid of the
> + * ptracer.  The caller should eschew invasive tracing in this case.
> + */
> +pid_t
> +Ptracer_pid(pid_t pid)
> +{
> +	char *traced;
> +	pid_t tracer_pid;
> +
> +	if ((traced = Pget_proc_status(pid, "TracerPid")) == NULL)
> +		return 0;
> +
> +	tracer_pid = strtol(traced, NULL, 10);
> +	free(traced);
> +	return tracer_pid;
> +}
> +
>  /*
>   * 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 22ff54d58d3f..9e5c25990b4f 100644
> --- a/libproc/libproc.h
> +++ b/libproc/libproc.h
> @@ -101,6 +101,7 @@ extern	void	Pset_procfs_path(const char *);
>  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);
>  
>  /*
>   * Calls that do not take a process structure.  These are used to determine
> -- 
> 2.43.0.272.gce700b77fd
> 
>