[DTrace-devel] [PATCH] Enforce restrictions on destructive actions when -w is not specified

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

The assertions for
         test/unittest/actions/system/err.system-destructive.d
         test/unittest/actions/freopen/err.freopen-destructive.d
are
          * ASSERTION: system() is a destructive action
          * ASSERTION: freopen() is a destructive action
but it seems to me the point of the tests is that these (destructive) 
actions are restricted when -w is not specified, as the subject says.  
So how about something like
          * ASSERTION: system(), a destructive action, prohibited without -w
          * ASSERTION: freopen(), a destructive action, prohibited 
without -w

In the same vein, we report
         dtrace: could not enable tracing: Destructive actions not allowed
It should seem to be
         dtrace: could not enable tracing: Destructive actions not 
allowed without -w

A few further tiny comments/questions below.


On 4/8/21 3:01 AM, Kris Van Hees wrote:
> diff --git a/libdtrace/dt_as.c b/libdtrace/dt_as.c
> index ed8cc544..760f281c 100644
> --- a/libdtrace/dt_as.c
> +++ b/libdtrace/dt_as.c
> @@ -366,6 +366,9 @@ fail:
>   		 */
>   		dp->dtdo_buf[i].off = labels[instr.off] - i - 1;
>   	}
> +	if (pcb->pcb_stmt != NULL &&
> +	    pcb->pcb_stmt->dtsd_clauseflags & DT_CLSFLAG_DESTRUCT)
> +		dp->dtdo_flags |= DIFOFLG_DESTRUCTIVE;
>   
>   	pcb->pcb_asvidx = 0;
Is the FIXME right before this now obsolete?

Also, did you want a blank line before the new code?  I could see it 
either way.

> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index b232f52b..f111165a 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -786,6 +786,9 @@ 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))
> +		*cfp |= DT_CLSFLAG_DESTRUCT;
> +
>   	if (kind == DTRACEACT_COMMIT) {
>   		if (*cfp & DT_CLSFLAG_COMMIT)
>   			dnerror(dnp, D_COMM_COMM,

What will be the fate of the FIXME dt_action_destructive() that appears 
right above here?

Also, while the order should not matter, moving this 
"destructive-handling" code further down in the function would better 
mimic the dt_stmt_append() code from which all this is derived.  The 
heritage of the code would be better preserved (not that that's a huge 
deal).