Re: [PATCH 1/2] tracing/hist: bound full field-name construction

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 29 Mar 2026 11:09:49 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> hist_field_name() builds a fully qualified synthetic field name in a
> fixed MAX_FILTER_STR_VAL buffer using repeated strcat() calls. Long
> system, event, and field names can therefore overflow the static staging
> buffer.
> 
> Build the qualified name with snprintf() and fall back to the plain
> field name if it does not fit.

The fallback breaks 
> 
> Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")

Do you have any examples where it actually does overflow or is this just theoretical?

If it's just theoretical, it does not get a "Fixes" tag.

Hmm, but actually I don't see it resetting full_name to a '\0' so I can see
this concatenating on top of a previous value. THAT would need fixing and
require a fixes tag. An example of triggering the overflow would also be
required.

> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  kernel/trace/trace_events_hist.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..4a27da628a71 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1362,12 +1362,12 @@ static const char *hist_field_name(struct hist_field *field,
>  		if (field->system) {
>  			static char full_name[MAX_FILTER_STR_VAL];
>  
> -			strcat(full_name, field->system);
> -			strcat(full_name, ".");
> -			strcat(full_name, field->event_name);
> -			strcat(full_name, ".");
> -			strcat(full_name, field->name);
> -			field_name = full_name;
> +			if (snprintf(full_name, sizeof(full_name), "%s.%s.%s",
> +				     field->system, field->event_name,
> +				     field->name) < sizeof(full_name))

Ug, please do not use a horribly looking if conditional. And it should most
definitely error on overflow: Break it up:

		if (field->system) {
  			static char full_name[MAX_FILTER_STR_VAL];
			int len;

			len = snprintf(full_name, sizeof(full_name), "%s.%s.%s",
				       field->system, field->event_name,
				       field->name);

			if (len >= size(full_name))
				return NULL;

			field_name = full_name;

> +				field_name = full_name;
> +			else
> +				field_name = field->name;
>  		} else
>  			field_name = field->name;
>  	} else if (field->flags & HIST_FIELD_FL_TIMESTAMP)

-- Steve