[DTrace-devel] [PATCH 07/13] Set interrupt signal handler earlier

[Kris Van Hees]

On Wed, Jul 01, 2020 at 10:41:12PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> We were creating probes and attaching BPF programs to them before
> setting the interrupt signal handler.  That meant that there was a
> window during which an interrupt signal would kill dtrace without
> probe cleanup taking place, polluting /sys/kernel/debug/tracing with
> custom DTrace uprobes and kprobes.
> 
> Set the interrupt signal handler before probes are attached and BPF
> programs created.  One can test g_intr==0 more liberally to bypass
> operations and move to dt_close() more quickly.
> 
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  cmd/dtrace.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/cmd/dtrace.c b/cmd/dtrace.c
> index cef86ef2..868cd3b9 100644
> --- a/cmd/dtrace.c
> +++ b/cmd/dtrace.c
> @@ -1373,10 +1373,18 @@ main(int argc, char *argv[])
>  		if (g_ofile != NULL && (g_ofp = fopen(g_ofile, "a")) == NULL)
>  			fatal("failed to open output file '%s'", g_ofile);
>  
> +		(void) sigemptyset(&act.sa_mask);

Drop the (void).

> +		act.sa_flags = 0;
> +		act.sa_handler = intr;
> +		if (sigaction(SIGINT, NULL, &oact) == 0 && oact.sa_handler != SIG_IGN)
> +			(void) sigaction(SIGINT, &act, NULL);

Drop the (void).

> +		if (sigaction(SIGTERM, NULL, &oact) == 0 && oact.sa_handler != SIG_IGN)
> +			(void) sigaction(SIGTERM, &act, NULL);

Drop the (void).

> +
>  		for (i = 0; i < g_cmdc; i++)
>  			exec_prog(&g_cmdv[i]);
>  
> -		if (done) {
> +		if (done || g_intr) {
>  			dtrace_close(g_dtp);
>  			return (g_status);
>  		}
> @@ -1480,7 +1488,8 @@ main(int argc, char *argv[])
>  	/*
>  	 * Start tracing.
>  	 */
> -	go();
> +	if (g_intr == 0)
> +		go();

I think the following would be more clear (to retain the overall normal flow
of statements when Ctrl-C is not issued):

	if (g_intr) {
		dtrace_close(g_dtp);
		return g_status;
	}

or alternatively, you could use:

	if (g_intr)
		goto out;

>  	(void) dtrace_getopt(g_dtp, "flowindent", &opt);
>  	g_flowindent = opt != DTRACEOPT_UNSET;
> @@ -1492,23 +1501,14 @@ main(int argc, char *argv[])
>  	if (opt != DTRACEOPT_UNSET)
>  		notice("allowing destructive actions\n");
>  
> -	(void) sigemptyset(&act.sa_mask);
> -	act.sa_flags = 0;
> -	act.sa_handler = intr;
> -
> -	if (sigaction(SIGINT, NULL, &oact) == 0 && oact.sa_handler != SIG_IGN)
> -		(void) sigaction(SIGINT, &act, NULL);
> -
> -	if (sigaction(SIGTERM, NULL, &oact) == 0 && oact.sa_handler != SIG_IGN)
> -		(void) sigaction(SIGTERM, &act, NULL);
> -
>  	/*
>  	 * Now that tracing is active and we are ready to consume trace data,
>  	 * continue any grabbed or created processes, setting them running
>  	 * using the /proc control mechanism inside of libdtrace.
>  	 */
>  	for (i = 0; i < g_psc; i++)
> -		dtrace_proc_continue(g_dtp, g_psv[i]);
> +		if (g_intr == 0)
> +			dtrace_proc_continue(g_dtp, g_psv[i]);

Put curly brackets around the body of the loop because that tends to be more
clear.  I am also not 100% certain it is safe to potentially interrupt the
resuming of user processes here and yet still apply dtrace_proc_release to all
of processes (even those that were not resumed) before we do dtrace_close().  

I would use if (g_intr) goto release_procs; anhd set a label release_procs
before the loop to do dtrace_proc_release.  Again, this highlghts that code
path as the exception case.

>  	g_pslive = g_psc; /* count for prochandler() */
>  
> -- 
> 2.18.2
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel