[DTrace-devel] [PATCH 04/05] dynvar: report error if dynvarsize is too small

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
though...

On 5/5/23 11:33, Kris Van Hees via DTrace-devel wrote:
> When dynamic variables (or associative arrays) are used, ensure that the
> dynvarsize value is sufficient to hold at least one dynamic variable
> (or associative array element).

Comment is not true and in fact conflicts with the subject line.

> Signed-off-by: Kris Van Hees<kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_bpf.c                            |  5 ++--
>   .../assocs/err.dynvarsize-too-small.d         | 30 +++++++++++++++++++
>   .../assocs/err.dynvarsize-too-small.r         |  3 ++
>   3 files changed, 36 insertions(+), 2 deletions(-)
>   create mode 100644 test/unittest/assocs/err.dynvarsize-too-small.d
>   create mode 100644 test/unittest/assocs/err.dynvarsize-too-small.r
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index b553b33f..a00b353d 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -843,9 +843,10 @@ gmap_create_dvars(dtrace_hdl_t *dtp)
>   	if (dtp->dt_maxdvarsize == 0)
>   		return 0;
>   
> -	nelems = dtp->dt_options[DTRACEOPT_DYNVARSIZE] / dtp->dt_maxdvarsize;
> +	nelems = dtp->dt_options[DTRACEOPT_DYNVARSIZE] /
> +		 (dtp->dt_maxtuplesize + dtp->dt_maxdvarsize);

Okay but also +2*sizeof(uint64_t)?

I'm pretty sure these models are rather approximate.  The user might 
think they're specifying how much room is being taken up but that's not 
what's going on.  We're simply using the dynvarsize value to size 
nelems, and the actual footprint is... complicated.  So the real point 
is perhaps that dynvarsize is an out-dated notion, and ndynelems (or 
something) should take its place.  For now, I suppose it's enough to 
support the historical option in the least torturous manner we can think of.

>   	if (nelems == 0)
> -		return 0;
> +		return dt_set_errno(dtp, EDT_BUFTOOSMALL);
>   
>   	if (create_gmap(dtp, "dvars", BPF_MAP_TYPE_HASH, sizeof(uint64_t),
>   			dtp->dt_maxdvarsize, nelems) == -1)
> diff --git a/test/unittest/assocs/err.dynvarsize-too-small.d b/test/unittest/assocs/err.dynvarsize-too-small.d
> new file mode 100644
> index 00000000..02fa4f8a
> --- /dev/null
> +++ b/test/unittest/assocs/err.dynvarsize-too-small.d
> @@ -0,0 +1,30 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + *http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: If dynvarsize is too small to hold a single dynamic variable,
> + *	      report an error.
> + *
> + * SECTION: Aggregations/Aggregations
> + */
> +
> +/*
> + * Storing an int (4 byte integer) in an associative array indexed by an int
> + * requires at least 20 bytes of dynamic variable storage.
> + */
> +#pragma D option dynvarsize=15

The comment is hard to understand:  how is 4+4>=20?  And why is 
dynvarsize=15 instead of, say, 19?  And then how about a 
tst.dynvarsize.d with dynvarsize=20 (or whatever it's supposed to be)?

> +
> +BEGIN
> +{
> +	a[1] = 1;
> +	exit(0);
> +}
> +
> +ERROR
> +{
> +	exit(1);
> +}
> diff --git a/test/unittest/assocs/err.dynvarsize-too-small.r b/test/unittest/assocs/err.dynvarsize-too-small.r
> new file mode 100644
> index 00000000..4b56154d
> --- /dev/null
> +++ b/test/unittest/assocs/err.dynvarsize-too-small.r
> @@ -0,0 +1,3 @@
> +-- @@stderr --
> +dtrace: script 'test/unittest/assocs/err.dynvarsize-too-small.d' matched 2 probes
> +dtrace: could not enable tracing: Enabling exceeds size of buffer
> -- 2.40.1