[DTrace-devel] [PATCH v3] Ensure curcpu values are scalarized when typecast

[Kris Van Hees]

On Wed, Aug 03, 2022 at 06:11:16PM +0100, Nick Alcock wrote:
> On 3 Aug 2022, Kris Van Hees via DTrace-devel verbalised:
> 
> > -		if (val == NULL)
> > -			return (uint64_t)NULL;	/* FIXME */
> > +		if (val == NULL) {
> > +			/*
> > +			 * Typically, we would use 'return error(...);' but
> > +			 * that confuses the verifier because it returns -1.
> > +			 * So, instead, we explicitly return 0.
> > +			 */
> > +			error(dctx, DTRACEFLT_ILLOP, 0);
> > +			return 0;
> > +		}
> 
> Presumably 0 is in some way more special than -1? :)

Absolutely.  Because we test for NULL pointers in the right places and thus
the verifier is happy with that.

> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 83fbeba9..9ac00585 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2521,7 +2521,7 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
> >  		return;
> >  
> >  	if (dt_node_is_arith(dst) && dt_node_is_pointer(src) &&
> > -	    (src->dn_flags & DT_NF_ALLOCA)) {
> > +	    (src->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))) {
> >  		int	mst;
> 
> Seems sensible.
> 
> >  
> >  		if ((mst = dt_regset_alloc(drp)) == -1)
> > diff --git a/libdtrace/dt_ident.h b/libdtrace/dt_ident.h
> > index 7a44613d..c7cb29d1 100644
> > --- a/libdtrace/dt_ident.h
> > +++ b/libdtrace/dt_ident.h
> > @@ -96,6 +96,7 @@ typedef struct dt_ident {
> >  #define	DT_IDFLG_BPF	0x1000	/* variable is BPF */
> >  #define	DT_IDFLG_ALLOCA	0x2000	/* variable holds an alloca()ed pointer */
> >  #define	DT_IDFLG_NONALLOCA 0x4000 /* variable known not to hold an alloca()ed pointer */
> > +#define DT_IDFLG_DPTR	0x8000	/* variable is ptr to DTrace-managed storage */
> 
> (cosmetic tab problem)

Will fix.

> > diff --git a/libdtrace/dt_parser.c b/libdtrace/dt_parser.c
> > index 1625e887..713c4f01 100644
> > --- a/libdtrace/dt_parser.c
> > +++ b/libdtrace/dt_parser.c
> > @@ -2819,10 +2819,13 @@ dt_xcook_ident(dt_node_t *dnp, dt_idhash_t *dhp, uint_t idkind, int create)
> >  		dnp->dn_flags |= DT_NF_LVALUE;
> >  
> >  		/*
> > -		 * If the type of the variable is a REF-type, we mark this
> > -		 * variable node as a pointer to DTrace-managed storage (DPTR).
> > +		 * If the identifier is marked as a pointer to DTrace-managed
> > +		 * storage (DPTR), or if the type of the variable is a
> > +		 * REF-type, we mark this variable node as a pointer to
> > +		 * DTrace-managed storage (DPTR).
> >  		 */
> > -		if (dnp->dn_flags & DT_NF_REF)
> > +		if ((idp->di_flags & DT_IDFLG_DPTR) ||
> > +		    (dnp->dn_flags & DT_NF_REF))
> >  			dnp->dn_flags |= DT_NF_DPTR;
> >  
> 
> I'd be a little concerned about propagation of this flag. Do we know it
> propagates everywhere it needs to? What if you assign this thing to a
> variable, what if you use it in a ternary, does it in general need to be
> treated like alloca taint is?
> 
> (No matter what, you need to add it to the relevant places in
> dt_node_printr: but it might be worth hunting for references to
> DT_TOK_ALLOCA in dt_parser and seeing how many of these, if any, need to
> have something similar done to them.)

DPTR was already introduced in an earlier patch and propagates through the
nodes.  So this is all already there.

> > diff --git a/test/unittest/arithmetic/tst.cast-curcpu.d b/test/unittest/arithmetic/tst.cast-curcpu.d
> > new file mode 100644
> > index 00000000..bc58352d
> > --- /dev/null
> > +++ b/test/unittest/arithmetic/tst.cast-curcpu.d
> > @@ -0,0 +1,26 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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: Casting a curcpu value to a scalar is allowed.
> > + *
> > + * SECTION: Types, Operators, and Expressions/Arithmetic Operators
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +BEGIN
> > +{
> > +	val = ((int64_t)curcpu) >> 8;
> 
> What about
> 
> 	val = curcpu;
> 	castval = ((int64_t)val) >> 8;
> ...
> 
> I bet this will fail :(

You lose :)  It does not.