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

[Eugene Loh]

I do not understand why these changes are being made now.  If string 
handling will be revamped (getting rid of the length prefix?), then it 
would seem that the first thing to do would be to clean that up before 
worrying about getting the (soon to be obsolete) length prefix correct 
in edge cases.

As far as tests go, it would seem that two additions are needed:

1)  Assigning strings in "reverse order."  E.g., if strings are stored 
x, then y, and then z, if we assign first x and then y and then z, then 
any overruns are cleaned up by the subsequent assignment.  So there 
should also be tests that assign in reverse order.  Say, assign y, then 
assign x (possibly tainting y???), and then check what y looks like.

2)  Testing strings greater than 256 bytes.  Currently, each string is 
prefixed by two length bytes.  For the strings being tested, the first 
of these is always 0, perhaps providing a safety net for a string that 
comes before it.  Some tests should check strsize>256.  Or, again, start 
by getting rid of the length prefix.

Again, I don't really see the sense in cleaning up these edge cases if 
string handling will soon be revamped.  It fixes some bugs at the cost 
of more complicated commit history (and development time).  Nevertheless,
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
if you want to push this patch anyhow.

And...

On 9/9/21 3:15 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.
>
> The offsets for lvars and gvars was also not taking into account the
> length prefix to be sotred for each string value.

stored

> diff --git a/libdtrace/dt_ident.c b/libdtrace/dt_ident.c
> @@ -1009,8 +1009,13 @@ dt_ident_set_data(dt_ident_t *idp, void *data)
>   void
>   dt_ident_set_storage(dt_ident_t *idp, uint_t alignment, uint_t size)
>   {
> +	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
>   	dt_idhash_t	*dhp = idp->di_hash;
>   
> +	if (idp->di_ctfp == DT_STR_CTFP(dtp) &&
> +	    idp->di_type == DT_STR_TYPE(dtp))
> +		size += DT_STRLEN_BYTES;

Why no room for a terminating NULL byte?