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

[Kris Van Hees]

On Thu, Apr 08, 2021 at 02:43:28PM -0400, Eugene Loh wrote:
> 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

The main point is that freopen() and system() are destructive actions and
they are therefore not allowed in general.  I actually should add two tests 
to specifically verify that with '-w' they are allowed.

> 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

The error message texts come from the legacy implementation and have been used
in DTrace for many many years.

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

No, we don't have subroutine implementations yet.  Once those are done (I hope
pretty soon) this will probably change since we could set the destruct clause
flag explicitly when a destructive subroutine is used.  But since that code
does not exist yet, the FIXME should stay.
> 
> 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?

That is future work because it ties in with translator support and things
like that.  More work is needed on when a DIFEXPR may have a DIFO that is
marked as destructive, and how we will handle that 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).

This setting of a destructive clause flag is new - it did not exist in
dt_stmt_append so there is nothing to mimic here.  Also, it is important
to set the destructive clause flag before handling the specific cases for
actions and action kinds because some of the cases here bail out of the
function and therefore would never get to the check for destructive actions.