Re: [PATCH] options: check *frames option values against system limit

I'm confused.

Is this amending an earlier patch, which introduced dt_opt_maxframes() 
-- a patch that has not yet been applied to the dev branch?  If so, why 
not rework the earlier patch for a cleaner commit history?

What observable change in behavior does this patch produce?  Why isn't 
there a test?

Also...

On 9/30/25 12:47, Kris Van Hees wrote:
> Commit 81e99397d ("Add support for the stack() action") added code to
> retrieve the system limit for stack traces and used that value to set
> the 'maxframes' option.
>
> However, a user can set the 'maxframes' option from the command line,
> which would allow setting the option to value beyond the system limit.
>
> We now record the system limit in dtrace_hdl_t as well as setting the

In general, "now" in a commit message strikes me as ambiguous.  Does it 
mean before or after the patch?  Not a huge deal here since the patch 
quickly indicates that this is new behavior, but I'd prefer avoiding "now."

> 'maxframes' option to this value as a default.  Options processing now
> ensures that *frames options cannot be set beyond the system limit.
>
> Note: 'stackframes', 'ustackframes', and 'jstackframes' can be set to
> a value that is higher than 'maxframes'.  The existing immplementation
> in the code generator already lowers their value to 'maxframes' when
> needed.

This seems to contradict the new comment in dt_vopen(), which says you 
cannot set to a higher value, and the behavior in the new dt_opt_frames().

> Signed-off-by: Kris Van Hees <[email protected]>
> ---
>   libdtrace/dt_impl.h    |  1 +
>   libdtrace/dt_open.c    |  7 +++++--
>   libdtrace/dt_options.c | 38 +++++++++-----------------------------
>   3 files changed, 15 insertions(+), 31 deletions(-)
>
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 4dc4c9a7..723aac8b 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -310,6 +310,7 @@ struct dtrace_hdl {
>   	uint_t dt_maxtuplesize;	/* largest tuple across programs */
>   	uint_t dt_maxlvaralloc;	/* largest lvar alloc across pcbs */
>   	uint_t dt_maxaggdsize;	/* largest aggregation data size */
> +	uint64_t dt_maxframes;	/* system limit on stack traces */
>   	dt_tstring_t *dt_tstrings; /* temporary string slots */
>   	dt_list_t dt_modlist;	/* linked list of dt_module_t's */
>   	dt_htab_t *dt_mods;	/* hash table of dt_module_t's */
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index ed0b14a1..ba5463a0 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -764,13 +764,16 @@ dt_vopen(int version, int flags, int *errp,
>   	dtp->dt_options[DTRACEOPT_SCRATCHSIZE] = sizeof(uint64_t) + _dtrace_scratchsize;
>   
>   	/*
> -	 * Set the default value of maxframes.
> +	 * Set the default value of maxframes.  This is the maximum of stack
> +	 * frames that can be requested from the kernel.  The *frames options
> +	 * cannot be set to a value that exceeds this limit.
>   	 */
>   	fd = fopen("/proc/sys/kernel/perf_event_max_stack", "r");
>   	assert(fd);
> -	if (fscanf(fd, "%lu", &dtp->dt_options[DTRACEOPT_MAXFRAMES]) != 1)
> +	if (fscanf(fd, "%lu", &dtp->dt_maxframes) != 1)
>   		return set_open_errno(dtp, errp, EDT_READMAXSTACK);
>   	fclose(fd);
> +	dtp->dt_options[DTRACEOPT_MAXFRAMES] = dtp->dt_maxframes;
>   
>   	/*
>   	 * Set the default pcap capture size.
> diff --git a/libdtrace/dt_options.c b/libdtrace/dt_options.c
> index 7508f044..97516517 100644
> --- a/libdtrace/dt_options.c
> +++ b/libdtrace/dt_options.c
> @@ -913,40 +913,20 @@ dt_opt_rate(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
>   }
>   
>   /*
> - * When setting the maxframes option, set the option in the dt_options array
> - * using dt_opt_runtime() as usual, and then update the definition of the CTF
> - * type for "_stack" to be an array of the corresponding size.
> - * If any errors occur, reset dt_options[option] to its previous value.
> + * Set a *frames option, ensuring that its value is within the system
> + * constaints.
>    */
>   static int
> -dt_opt_maxframes(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
> +dt_opt_frames(dtrace_hdl_t *dtp, const char *arg, uintptr_t option)
>   {
>   	dtrace_optval_t val = dtp->dt_options[option];
> -	ctf_file_t *fp = DT_STACK_CTFP(dtp);
> -	ctf_id_t type = ctf_type_resolve(fp, DT_STACK_TYPE(dtp));
> -	ctf_arinfo_t r;
>   
>   	if (dt_opt_runtime(dtp, arg, option) != 0)
>   		return -1; /* dt_errno is set for us */
>   
> -	if (dtp->dt_options[option] > UINT_MAX) {
> -		dtp->dt_options[option] = val;
> -		return dt_set_errno(dtp, EOVERFLOW);
> -	}
> -
> -	if (ctf_array_info(fp, type, &r) == CTF_ERR) {
> -		dtp->dt_options[option] = val;
> -		dtp->dt_ctferr = ctf_errno(fp);
> -		return dt_set_errno(dtp, EDT_CTF);
> -	}
> -
> -	r.ctr_nelems = (uint_t)dtp->dt_options[option];
> -
> -	if (ctf_set_array(fp, type, &r) == CTF_ERR ||
> -	    ctf_update(fp) == CTF_ERR) {
> +	if (dtp->dt_options[option] > dtp->dt_maxframes) {
>   		dtp->dt_options[option] = val;
> -		dtp->dt_ctferr = ctf_errno(fp);
> -		return dt_set_errno(dtp, EDT_CTF);
> +		return dt_set_errno(dtp, EDT_BADOPTVAL);
>   	}
>   
>   	return 0;
> @@ -1194,19 +1174,19 @@ static const dt_option_t _dtrace_rtoptions[] = {
>   	{ "destructive", dt_opt_runtime, DTRACEOPT_DESTRUCTIVE },
>   	{ "dynvarsize", dt_opt_size, DTRACEOPT_DYNVARSIZE },
>   	{ "grabanon", dt_opt_runtime, DTRACEOPT_GRABANON },
> -	{ "jstackframes", dt_opt_runtime, DTRACEOPT_JSTACKFRAMES },
> +	{ "jstackframes", dt_opt_frames, DTRACEOPT_JSTACKFRAMES },
>   	{ "jstackstrsize", dt_opt_size, DTRACEOPT_JSTACKSTRSIZE },
>   	{ "lockmem", dt_opt_lockmem, DTRACEOPT_LOCKMEM },
> -	{ "maxframes", dt_opt_maxframes, DTRACEOPT_MAXFRAMES },
> +	{ "maxframes", dt_opt_frames, DTRACEOPT_MAXFRAMES },
>   	{ "nusdtprobes", dt_opt_runtime, DTRACEOPT_NUSDTPROBES },
>   	{ "nspec", dt_opt_runtime, DTRACEOPT_NSPEC },
>   	{ "pcapsize", dt_opt_pcapsize, DTRACEOPT_PCAPSIZE },
>   	{ "scratchsize", dt_opt_scratchsize, DTRACEOPT_SCRATCHSIZE },
>   	{ "specsize", dt_opt_size, DTRACEOPT_SPECSIZE },
> -	{ "stackframes", dt_opt_runtime, DTRACEOPT_STACKFRAMES },
> +	{ "stackframes", dt_opt_frames, DTRACEOPT_STACKFRAMES },
>   	{ "statusrate", dt_opt_rate, DTRACEOPT_STATUSRATE },
>   	{ "strsize", dt_opt_strsize, DTRACEOPT_STRSIZE },
> -	{ "ustackframes", dt_opt_runtime, DTRACEOPT_USTACKFRAMES },
> +	{ "ustackframes", dt_opt_frames, DTRACEOPT_USTACKFRAMES },
>   	{ "noresolve", dt_opt_runtime, DTRACEOPT_NORESOLVE },
>   	{ NULL }
>   };