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

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

[Pengpeng Hou]

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.

Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
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))
+				field_name = full_name;
+			else
+				field_name = field->name;
 		} else
 			field_name = field->name;
 	} else if (field->flags & HIST_FIELD_FL_TIMESTAMP)
-- 
2.50.1 (Apple Git-155)

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

[Steven Rostedt]

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

[PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Pengpeng Hou]

hist_field_name() uses a static MAX_FILTER_STR_VAL buffer for fully
qualified variable-reference names, but it currently appends into that
buffer with strcat() without rebuilding it first. As a result, repeated
calls append a new "system.event.field" name onto the previous one,
which can eventually run past the end of full_name.

Build the name with snprintf() on each call and return NULL if the fully
qualified name does not fit in MAX_FILTER_STR_VAL.

Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
v2:
- rebuild full_name on each call instead of falling back to field->name
- return NULL on overflow as suggested
- split out the snprintf() length check instead of using an inline if

 kernel/trace/trace_events_hist.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 73ea180cad55..f9c8a4f078ea 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1361,12 +1361,14 @@ static const char *hist_field_name(struct hist_field *field,
 		 field->flags & HIST_FIELD_FL_VAR_REF) {
 		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 >= sizeof(full_name))
+				return NULL;
 
-			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;
 		} else
 			field_name = field->name;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Steven Rostedt]

On Mon, 30 Mar 2026 10:46:19 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:


Please resend both patches as a separate thread series. Do not send new
versions of the patch as a reply to the old one. That just makes it much
harder for maintainers to keep track of patches, as they are hidden within
threads.

> hist_field_name() uses a static MAX_FILTER_STR_VAL buffer for fully
> qualified variable-reference names, but it currently appends into that
> buffer with strcat() without rebuilding it first. As a result, repeated
> calls append a new "system.event.field" name onto the previous one,
> which can eventually run past the end of full_name.
> 
> Build the name with snprintf() on each call and return NULL if the fully
> qualified name does not fit in MAX_FILTER_STR_VAL.
> 
> Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> v2:

Instead of saying "v2", use:

Changes since v1: https://lore.kernel.org/all/20260329030950.32503-1-pengpeng@iscas.ac.cn/

That keeps the history link of this patch compared to the previous version.

-- Steve


> - rebuild full_name on each call instead of falling back to field->name
> - return NULL on overflow as suggested
> - split out the snprintf() length check instead of using an inline if

[PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Pengpeng Hou]

hist_field_name() uses a static MAX_FILTER_STR_VAL buffer for fully
qualified variable-reference names, but it currently appends into that
buffer with strcat() without rebuilding it first. As a result, repeated
calls append a new "system.event.field" name onto the previous one,
which can eventually run past the end of full_name.

Build the name with snprintf() on each call and return NULL if the fully
qualified name does not fit in MAX_FILTER_STR_VAL.

Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1: https://lore.kernel.org/all/20260329030950.32503-1-pengpeng@iscas.ac.cn/

- rebuild full_name on each call instead of falling back to field->name
- return NULL on overflow as suggested
- split out the snprintf() length check instead of using an inline if

 kernel/trace/trace_events_hist.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 73ea180cad55..f9c8a4f078ea 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1361,12 +1361,14 @@ static const char *hist_field_name(struct hist_field *field,
 		 field->flags & HIST_FIELD_FL_VAR_REF) {
 		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 >= sizeof(full_name))
+				return NULL;
 
-			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;
 		} else
 			field_name = field->name;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Steven Rostedt]

Tom,

On Wed,  1 Apr 2026 19:22:23 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> hist_field_name() uses a static MAX_FILTER_STR_VAL buffer for fully
> qualified variable-reference names, but it currently appends into that
> buffer with strcat() without rebuilding it first. As a result, repeated
> calls append a new "system.event.field" name onto the previous one,
> which can eventually run past the end of full_name.
> 
> Build the name with snprintf() on each call and return NULL if the fully
> qualified name does not fit in MAX_FILTER_STR_VAL.
> 
> Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v1: https://lore.kernel.org/all/20260329030950.32503-1-pengpeng@iscas.ac.cn/
> 
> - rebuild full_name on each call instead of falling back to field->name
> - return NULL on overflow as suggested
> - split out the snprintf() length check instead of using an inline if
> 
>  kernel/trace/trace_events_hist.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..f9c8a4f078ea 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1361,12 +1361,14 @@ static const char *hist_field_name(struct hist_field *field,
>  		 field->flags & HIST_FIELD_FL_VAR_REF) {
>  		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 >= sizeof(full_name))
> +				return NULL;
>  
> -			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;

I wanted to test this but I can't find anything that triggers this path.
How does a field here get its ->system set?

If there's no way to hit this path, I much rather remove it than "fix" it.

-- Steve


>  		} else
>  			field_name = field->name;

Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Steven Rostedt]

On Wed, 8 Apr 2026 10:15:00 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> Hi Steve,
> 
> Thanks for looking at this.
> 
> I believe this path is reachable.
> 
> field->system is set when a VAR_REF is created with an event-qualified
> reference. The assignment happens in init_var_ref(), which is called
> from create_var_ref().
> 
> The parser accepts fully qualified references of the form
> 
>   system.event.$var
> 
> and parse_expr() passes the parsed system/event strings into
> create_var_ref(). trace_action_create() also accepts explicitly
> qualified action parameters in the same form and passes those strings
> into create_var_ref().
> 
> This is also documented in Documentation/trace/histogram.rst:
> 
>   fully-qualified name is of the form 'system.event_name.$var_name'
>   or 'system.event_name.field'.

Ah I read the document and missed this part. Thanks for pointing it out.

None of our tests use this, so that definitely needs to be fixed.

OK, yeah I can trigger that path now. I'll continue looking at the patch.

Thanks,

-- Steve


> 
> The path is user-visible because reading back the trigger goes through
> 
>   event_hist_trigger_print() -> hist_field_print() -> hist_field_name()
> 
> so creating a hist trigger that uses an inter-event variable reference
> in fully qualified form is enough to exercise it.
> 
> So I don't think this is dead code. If you would prefer a different way
> of handling the printed name, I can respin accordingly, but I don't
> think the current branch is unreachable.
> 
> Thanks,
> Pengpeng
> 

Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Pengpeng Hou]

Hi Steve,

Thanks for looking at this.

I believe this path is reachable.

field->system is set when a VAR_REF is created with an event-qualified
reference. The assignment happens in init_var_ref(), which is called
from create_var_ref().

The parser accepts fully qualified references of the form

  system.event.$var

and parse_expr() passes the parsed system/event strings into
create_var_ref(). trace_action_create() also accepts explicitly
qualified action parameters in the same form and passes those strings
into create_var_ref().

This is also documented in Documentation/trace/histogram.rst:

  fully-qualified name is of the form 'system.event_name.$var_name'
  or 'system.event_name.field'.

The path is user-visible because reading back the trigger goes through

  event_hist_trigger_print() -> hist_field_print() -> hist_field_name()

so creating a hist trigger that uses an inter-event variable reference
in fully qualified form is enough to exercise it.

So I don't think this is dead code. If you would prefer a different way
of handling the printed name, I can respin accordingly, but I don't
think the current branch is unreachable.

Thanks,
Pengpeng

Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Tom Zanussi]

Hi Steve,

On Tue, 2026-04-07 at 21:05 -0400, Steven Rostedt wrote:
> 
> Tom,
> 
> On Wed,  1 Apr 2026 19:22:23 +0800
> Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:
> 
> > hist_field_name() uses a static MAX_FILTER_STR_VAL buffer for fully
> > qualified variable-reference names, but it currently appends into that
> > buffer with strcat() without rebuilding it first. As a result, repeated
> > calls append a new "system.event.field" name onto the previous one,
> > which can eventually run past the end of full_name.
> > 
> > Build the name with snprintf() on each call and return NULL if the fully
> > qualified name does not fit in MAX_FILTER_STR_VAL.
> > 
> > Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
> > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > ---
> > Changes since v1: https://lore.kernel.org/all/20260329030950.32503-1-pengpeng@iscas.ac.cn/
> > 
> > - rebuild full_name on each call instead of falling back to field->name
> > - return NULL on overflow as suggested
> > - split out the snprintf() length check instead of using an inline if
> > 
> >  kernel/trace/trace_events_hist.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index 73ea180cad55..f9c8a4f078ea 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -1361,12 +1361,14 @@ static const char *hist_field_name(struct hist_field *field,
> >  		 field->flags & HIST_FIELD_FL_VAR_REF) {
> >  		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 >= sizeof(full_name))
> > +				return NULL;
> >  
> > -			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;
> 
> I wanted to test this but I can't find anything that triggers this path.
> How does a field here get its ->system set?
> 

->system is set when using fully-qualified variable names. For
instance:

echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> sys/kernel/debug/tracing/events/sched/sched_waking/trigger
echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
echo 'hist:keys=next_pid:lat0=common_timestamp.usecs-sched.sched_waking.$ts0:lat1=common_timestamp.usecs-sched.sched_wakeup.$ts0' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger
echo 'hist:keys=next_pid:vals=$lat0,$lat1' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger

Here, the sched_switch trigger would error out if the unqualified $ts0
variables were used instead of the fully-qualified ones because there's
no way to distinguish which $ts0 was meant.

Tom



> If there's no way to hit this path, I much rather remove it than "fix" it.
> 
> -- Steve
> 
> 
> >  		} else
> >  			field_name = field->name;
> 

Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Steven Rostedt]

On Wed, 08 Apr 2026 10:58:06 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

Hi Tom,

> ->system is set when using fully-qualified variable names. For  
> instance:
> 
> echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> sys/kernel/debug/tracing/events/sched/sched_waking/trigger
> echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
> echo 'hist:keys=next_pid:lat0=common_timestamp.usecs-sched.sched_waking.$ts0:lat1=common_timestamp.usecs-sched.sched_wakeup.$ts0' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger
> echo 'hist:keys=next_pid:vals=$lat0,$lat1' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger
> 
> Here, the sched_switch trigger would error out if the unqualified $ts0
> variables were used instead of the fully-qualified ones because there's
> no way to distinguish which $ts0 was meant.
> 

Yep I see that now. I never had a need to use it before, but I probably
should implement this in libtracefs to be safe.

We should definitely add a selftest that tests this. There's one case that
does use it but it doesn't use multiple ones. We should add a test that
does so.

trigger-multi-actions-accept.tc has the system, but it's not needed here.

We should also have a test to test the output of theses lines.

-- Steve

Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Tom Zanussi]

On Wed, 2026-04-08 at 12:25 -0400, Steven Rostedt wrote:
> On Wed, 08 Apr 2026 10:58:06 -0500
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> Hi Tom,
> 
> > ->system is set when using fully-qualified variable names. For  
> > instance:
> > 
> > echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> sys/kernel/debug/tracing/events/sched/sched_waking/trigger
> > echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
> > echo 'hist:keys=next_pid:lat0=common_timestamp.usecs-sched.sched_waking.$ts0:lat1=common_timestamp.usecs-sched.sched_wakeup.$ts0' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger
> > echo 'hist:keys=next_pid:vals=$lat0,$lat1' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger
> > 
> > Here, the sched_switch trigger would error out if the unqualified $ts0
> > variables were used instead of the fully-qualified ones because there's
> > no way to distinguish which $ts0 was meant.
> > 
> 
> Yep I see that now. I never had a need to use it before, but I probably
> should implement this in libtracefs to be safe.
> 
> We should definitely add a selftest that tests this. There's one case that
> does use it but it doesn't use multiple ones. We should add a test that
> does so.
> 
> trigger-multi-actions-accept.tc has the system, but it's not needed here.
> 
> We should also have a test to test the output of theses lines.

Yeah, definitely. I can try adding this as a test..

Tom


> 
> -- Steve

Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call

[Tom Zanussi]

On Wed, 2026-04-01 at 19:22 +0800, Pengpeng Hou wrote:
> hist_field_name() uses a static MAX_FILTER_STR_VAL buffer for fully
> qualified variable-reference names, but it currently appends into that
> buffer with strcat() without rebuilding it first. As a result, repeated
> calls append a new "system.event.field" name onto the previous one,
> which can eventually run past the end of full_name.
> 
> Build the name with snprintf() on each call and return NULL if the fully
> qualified name does not fit in MAX_FILTER_STR_VAL.
> 
> Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

Looks good to me, thanks.

Reviewed-by: Tom Zanussi <zanussi@kernel.org>
Tested-by: Tom Zanussi <zanussi@kernel.org>

> ---
> Changes since v1: https://lore.kernel.org/all/20260329030950.32503-1-pengpeng@iscas.ac.cn/
> 
> - rebuild full_name on each call instead of falling back to field->name
> - return NULL on overflow as suggested
> - split out the snprintf() length check instead of using an inline if
> 
>  kernel/trace/trace_events_hist.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..f9c8a4f078ea 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1361,12 +1361,14 @@ static const char *hist_field_name(struct hist_field *field,
>  		 field->flags & HIST_FIELD_FL_VAR_REF) {
>  		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 >= sizeof(full_name))
> +				return NULL;
>  
> -			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;
>  		} else
>  			field_name = field->name;