[DTrace-devel] [PATCH 2/2] Fix data size for value copy in dt_cg_store_var()

[Eugene Loh]

On 9/8/21 5:07 PM, Kris Van Hees wrote:
> The size of the data to be copied in dt_cg_store_var() was determined
> wrongly.  For strings constants we were copying the size of the string
> constant even if that might be larger than the maximum string size.
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> @@ -2245,29 +2246,28 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
>   
>   			emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, idp->di_offset));
>   
>   			/*
>   			 * Determine the amount of data to be copied.  It is
> -			 * usually the size of the identifier, except for
> -			 * string constants where it is the size of the string
> -			 * constant (adjusted for the variable-width length
> -			 * prefix).  An assignment of a string constant is a
> -			 * store of type 'string' with a RHS of type
> -			 * DT_TOK_STRING.
> +			 * the lesser of the size of the identifier and the
> +			 * size of the data being copied in.
>   			 */
> -			if (dt_node_is_string(src) &&
> -			    src->dn_right->dn_op == DT_TOK_STRING)
> -				size = dt_node_type_size(src->dn_right) +
> -				       DT_STRLEN_BYTES;
> -			else
> -				size = idp->di_size;
> +			srcsz = dt_node_type_size(src->dn_right);
> +			if (dt_node_is_string(src))
> +				srcsz += DT_STRLEN_BYTES;
> +			dstsz = idp->di_size;
> +			if (idp->di_ctfp == DT_STR_CTFP(dtp) &&
> +			    idp->di_type == DT_STR_TYPE(dtp))
> +				dstsz += DT_STRLEN_BYTES;
> +			size = MIN(srcsz, dstsz);
>   
>   			dt_cg_memcpy(dlp, drp, reg, src->dn_reg, size);

I'm not convinced I understand this stuff.  It seems to me:

*)  The offsets in the gvar map for successive strings are STRSIZE 
apart...  not STRSIZE+DT_STRLEN_BYTES apart.  E.g., examine those 
idp->offset values.

*)  We copy in stuff that can be STRSIZE+DT_STRLEN_BYTES big. Wouldn't 
that overwrite another string or even outside the map?

*)  If we copy from the string table (which also has length prefixes on 
strings), we copy the length prefix over without editing it, even if we 
clipped the string.  This means that the string length prefixes in gvar 
won't be right.

E.g., the BPF verifier seems to complain about:

#pragma D option strsize=5
BEGIN
     {
         x = "abcd";
         y = "abcd";
         z = "abcd";
     }

So far as I can tell, this stuff is buggy.  The best thing to do, if the 
string length prefixes are going to disappear, is to eradicate them 
first and then clean this stuff up.