Re: [PATCH] Need -w for destructive actions, even if clause is not used

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

In addition to Nick's comments...

On Fri, Jul 11, 2025 at 12:40:24AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> If a clause includes a destructive action but -w is not used, dtrace
> should not start up, even if the clause is ignored (due to -Z).
> Solaris treated this as a runtime error.  We should do the same.
> 
> The test err.Z_no-w.sh was misguided and is replaced by a more
> direct test.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_bpf.c                            | 14 ++++
>  libdtrace/dt_cg.c                             |  4 +-
>  libdtrace/dt_impl.h                           |  1 +
>  .../options/err.no-w-or-destructive2.d        | 25 ++++++
>  .../options/err.no-w-or-destructive2.r        |  3 +
>  test/unittest/usdt/err.Z_no-w.r               |  4 -
>  test/unittest/usdt/err.Z_no-w.sh              | 76 -------------------
>  7 files changed, 46 insertions(+), 81 deletions(-)
>  create mode 100644 test/unittest/options/err.no-w-or-destructive2.d
>  create mode 100644 test/unittest/options/err.no-w-or-destructive2.r
>  delete mode 100644 test/unittest/usdt/err.Z_no-w.r
>  delete mode 100755 test/unittest/usdt/err.Z_no-w.sh
> 
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 4e7618e05..e2c3bfebc 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -1286,6 +1286,15 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>  	 */
>  	dtrace_getopt(dtp, "destructive", &dest_ok);
>  
> +	/*
> +	 * If we have any destructive actions at all and -w is not set,
> +	 * error out.  Solaris would reject this as a runtime error.  So,
> +	 * although we could have detected this problem at compilation,
> +	 * we mimic Solaris and wait until now to report.
> +	 */
> +	if (dtp->dt_havedest && dest_ok == DTRACEOPT_UNSET)
> +		return dt_set_errno(dtp, EDT_DESTRUCTIVE);

How about using dt_destructive as name?

> +
>  	for (prp = dt_list_next(&dtp->dt_enablings); prp != NULL;
>  	     prp = dt_list_next(prp)) {
>  		int		fd;
> @@ -1304,6 +1313,11 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>  
>  		DT_DISASM_PROG_LINKED(dtp, cflags, dp, stderr, NULL, prp->desc);
>  
> +		/*
> +		 * This check should never fail since, if any action is
> +		 * destructive and -w is not set, we should already have
> +		 * failed.
> +		 */
>  		if (dp->dtdo_flags & DIFOFLG_DESTRUCTIVE &&
>  		    dest_ok == DTRACEOPT_UNSET)
>  			return dt_set_errno(dtp, EDT_DESTRUCTIVE);

If this check should never fail, then it should be removed -or- turned into
an assert as a safeguard.  But rmoving it ought to be OK.

> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index e1bb59289..0e0648969 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1833,8 +1833,10 @@ dt_cg_clsflags(dt_pcb_t *pcb, dtrace_actkind_t kind, const dt_node_t *dnp)
>  {
>  	int		*cfp = &pcb->pcb_stmt->dtsd_clauseflags;
>  
> -	if (DTRACEACT_ISDESTRUCTIVE(kind))
> +	if (DTRACEACT_ISDESTRUCTIVE(kind)) {
>  		*cfp |= DT_CLSFLAG_DESTRUCT;
> +		pcb->pcb_hdl->dt_havedest |= 1;

dt_destructive

> +	}
>  
>  	if (kind == DTRACEACT_COMMIT) {
>  		if (*cfp & (DT_CLSFLAG_DATAREC | DT_CLSFLAG_AGGREGATION))
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 1e105b6e9..cdb222b3f 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -431,6 +431,7 @@ struct dtrace_hdl {
>  	dt_list_t dt_lib_dep_sorted;	/* dependency sorted library list */
>  	dt_global_pcap_t dt_pcap; /* global tshark/pcap state */
>  	char *dt_freopen_filename; /* filename for freopen() action */
> +	int dt_havedest;	/* have any destructive actions */

dt_destructive, and add it after dt_droptags to keep the booleans together.
In a later patch, we should combine those uint_t booleans into a single
uint_t dt_flags member.

>  };
>  
>  /*
> diff --git a/test/unittest/options/err.no-w-or-destructive2.d b/test/unittest/options/err.no-w-or-destructive2.d
> new file mode 100644
> index 000000000..eb9365fea
> --- /dev/null
> +++ b/test/unittest/options/err.no-w-or-destructive2.d
> @@ -0,0 +1,25 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Without -w or -xdestructive, destructive operations are not ok,
> + *            even if a clause will be ignored since it does not exist and
> + *            -Z was specified.

This could do with some rewriting.  The issue is that DTrace used to always
load all tracing programs, and the kernel would activate them as needed.  We
now only load programs for enabled probes (and with -Z active, we may load
some programs later).

What we need to be testing here is that if any clauses of a tracing script
contain destructive actions, and we are using the script for probing, then
an error should be reported unless we are allowing destructive actions (-w
or -xdestructive).

> + *
> + * SECTION: Options and Tunables/Consumer Options
> + */
> +/* @@runtest-opts: -Z */
> +
> +BEGIN
> +{
> +	exit(0);
> +}
> +
> +bogus:bogus:bogus:bogus
> +{
> +	system("echo ok");
> +}
> diff --git a/test/unittest/options/err.no-w-or-destructive2.r b/test/unittest/options/err.no-w-or-destructive2.r
> new file mode 100644
> index 000000000..1a6959475
> --- /dev/null
> +++ b/test/unittest/options/err.no-w-or-destructive2.r
> @@ -0,0 +1,3 @@
> +-- @@stderr --
> +dtrace: script 'test/unittest/options/err.no-w-or-destructive2.d' matched 1 probe
> +dtrace: could not enable tracing: Destructive actions not allowed
> diff --git a/test/unittest/usdt/err.Z_no-w.r b/test/unittest/usdt/err.Z_no-w.r
> deleted file mode 100644
> index b81622481..000000000
> --- a/test/unittest/usdt/err.Z_no-w.r
> +++ /dev/null
> @@ -1,4 +0,0 @@
> -dtrace is running so start the trigger
> -dtrace died as expected after trigger started
> --- @@stderr --
> -dtrace: processing aborted: Destructive actions not allowed
> diff --git a/test/unittest/usdt/err.Z_no-w.sh b/test/unittest/usdt/err.Z_no-w.sh
> deleted file mode 100755
> index 4f129341d..000000000
> --- a/test/unittest/usdt/err.Z_no-w.sh
> +++ /dev/null
> @@ -1,76 +0,0 @@
> -#!/bin/bash
> -#
> -# Oracle Linux DTrace.
> -# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> -# Licensed under the Universal Permissive License v 1.0 as shown at
> -# http://oss.oracle.com/licenses/upl.
> -#
> -# This test verifies that dtrace will not run a destructive script
> -# for USDT probes if -w is not specified.
> -#
> -# Specifically, the script is launched with -Z and no USDT processes are
> -# initially present.  Only once a USDT process is detected does dtrace
> -# fail due to the destructive action.
> -
> -dtrace=$1
> -trigger=`pwd`/test/triggers/usdt-tst-defer
> -
> -# Set up test directory.
> -
> -DIRNAME=$tmpdir/Z_no-w.$$.$RANDOM
> -mkdir -p $DIRNAME
> -cd $DIRNAME
> -
> -# Make a private copy of the trigger executable so that we get our
> -# own DOF stash.
> -
> -cp $trigger main
> -
> -# Run dtrace.
> -
> -$dtrace $dt_flags -Zq -o dtrace.out -n '
> -testprov*:::foo
> -{
> -	raise(SIGUSR1);
> -}' &
> -dtpid=$!
> -
> -# Wait up to half of the timeout period for dtrace to start up.
> -
> -iter=$((timeout / 2))
> -while [ $iter -gt 0 ]; do
> -	sleep 1
> -	if [ -e dtrace.out ]; then
> -		break
> -	fi
> -	iter=$((iter - 1))
> -done
> -if [[ $iter -eq 0 ]]; then
> -	echo ERROR starting DTrace job
> -	cat dtrace.out
> -	exit 1
> -fi
> -
> -# Start a trigger process.
> -
> -echo dtrace is running so start the trigger
> -./main > main.out &
> -pid=$!
> -
> -# Check again if dtrace is still running.
> -
> -sleep 2
> -if [[ ! -d /proc/$dtpid ]]; then
> -	echo dtrace died as expected after trigger started
> -else
> -	echo dtrace is unexpectedly still running
> -	kill -9 $dtpid
> -	wait    $dtpid
> -fi
> -
> -# Tell the trigger to proceed to completion.
> -
> -kill -USR1 $pid
> -wait       $pid
> -
> -exit 1
> -- 
> 2.43.5
>