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

[Nick Alcock <nick.alcock@oracle.com>]

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)