[DTrace-devel] [PATCH] Fix tstring length

[Eugene Loh]

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

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.

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
>   		);