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

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

[eugene.loh]

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);
+
 	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);
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;
+	}
 
 	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 */
 };
 
 /*
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.
+ *
+ * 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

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

[Nick Alcock]

On 11 Jul 2025, eugene loh uttered the following:

> 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>

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

modulo one microscopic annoyance (I am not a real unix programmer, I
don't like creat() and think we shouldn't add more).

> 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);
> +
>  	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

Grammar:

This check should never fail, since if any action is

> +		 * destructive and -w is not set, we should already have
> +		 * failed.
> +		 */

(but worth keeping anyway, I agree.)

> @@ -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 */
>  };

A piteous plea: could we call this dt_have_destructive or something? We
call destructive stuff "destructive", unabbreviated, everywhere else,
this flag is only checked in *one place* and thus hardly need concision,
and to me 'dest' always means 'destination' and thus causes a
double-take every time I see it used for something else.

>  
>  /*
> 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.

Pedantry: ... it does not *yet* exist :)

-- 
NULL && (void)

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

[Eugene Loh]

On 7/15/25 06:59, Nick Alcock wrote:

> On 11 Jul 2025, eugene loh uttered the following:
>> 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>
> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
>
> modulo one microscopic annoyance (I am not a real unix programmer, I
> don't like creat() and think we shouldn't add more).

Sorry, I don't know what you're asking for here.

>> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
>> @@ -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
> Grammar:
>
> This check should never fail, since if any action is

I think I see what you're saying:  separate the two phrases with a 
comma.  But there is a big appositional phrase in there ("if any action 
is destructive and -w is not set") and I wanted to set it off with the 
commas.  Maybe I'm wrong.  Dunno.

>> +		 * destructive and -w is not set, we should already have
>> +		 * failed.
>> +		 */
> (but worth keeping anyway, I agree.)

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

[Nick Alcock]

On 17 Jul 2025, Eugene Loh outgrape:

> On 7/15/25 06:59, Nick Alcock wrote:
>
>> On 11 Jul 2025, eugene loh uttered the following:
>>> 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>
>> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
>>
>> modulo one microscopic annoyance (I am not a real unix programmer, I
>> don't like creat() and think we shouldn't add more).
>
> Sorry, I don't know what you're asking for here.

"Name too short" :) referring to:

> > +	int dt_havedest;	/* have any destructive actions */
> >  };
> 
> A piteous plea: could we call this dt_have_destructive or something? We
> call destructive stuff "destructive", unabbreviated, everywhere else,
> this flag is only checked in *one place* and thus hardly need concision,
> and to me 'dest' always means 'destination' and thus causes a
> double-take every time I see it used for something else.

Anyway...

>>> +		 * This check should never fail since, if any action is
>> Grammar:
>>
>> This check should never fail, since if any action is
>
> I think I see what you're saying:  separate the two phrases with a
> comma.  But there is a big appositional phrase in there ("if any
> action is destructive and -w is not set") and I wanted to set it off
> with the commas.  Maybe I'm wrong.  Dunno.

I tried that -- it feels clumsier, since you definitely cannot set it
off with a comma but *not* set off the clause surrounding it (as you did
above).

>>> +		 * destructive and -w is not set, we should already have
>>> +		 * failed.
>>> +		 */
>> (but worth keeping anyway, I agree.)

-- 
NULL && (void)

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

[Kris Van Hees]

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
> 

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

[Kris Van Hees]

Another question...

On Fri, Aug 01, 2025 at 11:36:17AM -0400, Kris Van Hees wrote:
> 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

Why did you delete this test?  Isn't it still a valid test anyway, and since
it tests this in a more practical sense, it seems to have value.

> > 
> > 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
> > 

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

[Eugene Loh]

On 8/1/25 13:49, Kris Van Hees wrote:

> On Fri, Aug 01, 2025 at 11:36:17AM -0400, Kris Van Hees wrote:
>> 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.
>>>
>>>   delete mode 100644 test/unittest/usdt/err.Z_no-w.r
>>>   delete mode 100755 test/unittest/usdt/err.Z_no-w.sh
> Why did you delete this test?  Isn't it still a valid test anyway, and since
> it tests this in a more practical sense, it seems to have value.

The test was failing consistently on every platform;  it was based on 
assumptions that were eventually discarded.  So, no, not a valid test.  
It assumed behavior different from what this commit message asserts.

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

[Eugene Loh]

On 8/1/25 11:36, Kris Van Hees wrote:

> 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.
>>
>> 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
>> @@ -0,0 +1,25 @@
>> +/*
>> + * 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.

Sorry, what does "this" refer to?  The comment, the test, or the patch?

> 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).

I think we have such a test?

The case being tested here is where a destructive action is specified, 
no -w is specified, but the clause will not be exercised since the probe 
does not exist and -Z allows us to ignore it.  In this case, should the 
script be accepted or rejected?  The patch says the script should be 
rejected, which was the legacy behavior. (People familiar with the 
legacy implementation may understand why the legacy behavior was what it 
was, but the question now is whether that behavior should be changed.)

>> +/* @@runtest-opts: -Z */
>> +
>> +bogus:bogus:bogus:bogus
>> +{
>> +	system("echo ok");
>> +}

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

[Kris Van Hees]

On Fri, Aug 01, 2025 at 02:02:27PM -0400, Eugene Loh wrote:
> On 8/1/25 13:49, Kris Van Hees wrote:
> 
> > On Fri, Aug 01, 2025 at 11:36:17AM -0400, Kris Van Hees wrote:
> > > 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.
> > > > 
> > > >   delete mode 100644 test/unittest/usdt/err.Z_no-w.r
> > > >   delete mode 100755 test/unittest/usdt/err.Z_no-w.sh
> > Why did you delete this test?  Isn't it still a valid test anyway, and since
> > it tests this in a more practical sense, it seems to have value.
> 
> The test was failing consistently on every platform;  it was based on
> assumptions that were eventually discarded.  So, no, not a valid test.  It
> assumed behavior different from what this commit message asserts.

Ah, overlooked that it was testing for the non-compatible behaviour.  OK.

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

[Kris Van Hees]

On Fri, Aug 01, 2025 at 02:15:18PM -0400, Eugene Loh wrote:
> On 8/1/25 11:36, Kris Van Hees wrote:
> 
> > 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.
> > > 
> > > 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
> > > @@ -0,0 +1,25 @@
> > > +/*
> > > + * 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.
> 
> Sorry, what does "this" refer to?  The comment, the test, or the patch?

The text of the assertion.

> > 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).
> 
> I think we have such a test?

Yes, I just don't think that the way the assertion is phrased well.  E.g.
talking about a clause being ignored since it does not exist.  The clause
very certainly exists (since it is in the script).  It is the probe that
does not exist at the point of initial program construction and loading).

> The case being tested here is where a destructive action is specified, no -w
> is specified, but the clause will not be exercised since the probe does not
> exist and -Z allows us to ignore it.  In this case, should the script be
> accepted or rejected?  The patch says the script should be rejected, which
> was the legacy behavior. (People familiar with the legacy implementation may
> understand why the legacy behavior was what it was, but the question now is
> whether that behavior should be changed.)

Yes, again, it is the way the assertion is phrased that needs fixing I think.
> 
> > > +/* @@runtest-opts: -Z */
> > > +
> > > +bogus:bogus:bogus:bogus
> > > +{
> > > +	system("echo ok");
> > > +}