[DTrace-devel] [PATCH] Fix tstring length

[Kris Van Hees]

On Mon, Nov 15, 2021 at 04:24:13PM -0500, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> 
> If there are other NULL-byte issues, should we file patches for them?  E.g.,
> when we use dt_cg_memcpy() with a length that does not account for the NULL
> byte?  Also...

Sure, yes, if there are issues and you have a patch for it, post it.

> On 11/15/21 1:26 PM, Kris Van Hees via DTrace-devel wrote:
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > @@ -295,12 +295,13 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> >   	 *	- the greater of:
> >   	 *		+ the maximum stack trace size
> > -	 *		+ four times the maximum string size (incl. length
> > -	 *		  and allowing round up to multiple of 8)
> > +	 *		+ four times the maximum string storage size (incl.
> > +	 *		  length prefix and terminating NUL byte) rounded up to
> > +	 *		  the nearest multiple of 8),
> >   	 *		  plus the maximum string size (to accomodate the BPF
> >   	 *		  verifier)
> >   	 */
> 
> Odd to see "four times" hard-wired into the comments when we do not
> hard-wire "4" into the code.
> 
> It seems to me that "maximum string size" is clear and could be left as is. 
> Not sure why "storage" is being added -- or, if one wants to say "storage"
> why not just "string storage size."  Not a big deal one way or the other. 
> But then odd that "maximum string size" is not renamed consistently, in
> every place it occurs.  Why "maximum string storage size" in one place but
> "maximum string size" in the next?
> 
> If the terminating NUL byte deserves mention in the comments, then it would
> seem that it should also be added in the description of the verifier
> padding.

Comment block updated.

> Over all, though, these comments are bulky and do not really clarify what's
> going on in the more succinct code that follows.  I'd be up for paring these
> comments way back.
> 
> > @@ -310,7 +311,7 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> >   		MAX(sizeof(uint64_t) * dtp->dt_options[DTRACEOPT_MAXFRAMES],
> >   		    DT_TSTRING_SLOTS *
> >   			roundup(DT_STRLEN_BYTES +
> > -				dtp->dt_options[DTRACEOPT_STRSIZE], 8) +
> > +				dtp->dt_options[DTRACEOPT_STRSIZE] + 1, 8) +
> >   		    dtp->dt_options[DTRACEOPT_STRSIZE] + 1
> >   		);
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel