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

[Nick Alcock]

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

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

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

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

-- 
NULL && (void)