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

[Kris Van Hees]

On Thu, Sep 09, 2021 at 04:33:08PM -0400, Eugene Loh wrote:
> 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.

These edge cases are important to have reasonably stable string support.
While not perfect (which is why we are reworking some stuff), avoiding
rather obvious BPF verifier failures and corruption cases makes sense.
> 
> 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.

As we already discussed, we are not covering all cases and all potential
issues right now.  And incidentally, the way we store things, an overrun
would also mean that the next string will cause the NUL byte to be overwritten
which again would be quite noticable.  So yes, we can definitely add more
tests (and should0, the overall case of overruns and incorrect offset
calculations is captured by the tests we have (and further exercised by
the strjoin tests).

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

That is correct but practically, we already are limitinh support to strings
up to 256 bytes.  The upcoming work that rewoks string support will provide
support for > 256 string sizes.  So, again, for this case we do not have to
go out of our way yet to test for this.

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

The bugs it fixes are important.  The commit history complexity is worth it.

> 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

Thanks.

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

As it stands, the DTrace implementation of strings and the max string size
behaviour is (in most cases) including the NUL byte in the max string size
value.  I add "(in most cases)" because DTrace has been slightly inconsistent
in its implementation when it comes to whether to include the NUL byte in the
count or not.  For now, including it seems to be most prudent.  But we need
to do a thorough review to ensure that consistent behaviour is provided in
all of DTrace.