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

[Kris Van Hees]

On Thu, Sep 09, 2021 at 01:29:30AM -0400, Eugene Loh wrote:
> 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.

Yes, that actually has been wrong since varints were introduced for string
lenth prefixes.  It just never got caught because all strings used were
always less than 128 bytes and thus only required a 1-byte prefix.
> 
> *)  We copy in stuff that can be STRSIZE+DT_STRLEN_BYTES big. Wouldn't 
> that overwrite another string or even outside the map?

Yes.  Fixed.  Long standing bug that was not noticed until now (see above).

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

Yes, it may but that is something we will have to let go for now because other
code will correct that.  So, while it is not ideal, it is not causing issues.
Since we are doing this work as an intermediate step to revamping the way we
handle strings, we can live with it getting corrected by all other uses of
that var.

v2 on its way...

> 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.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel