Re: [PATCH V4 2/2] tools/perf: Use scnprintf in buffer offset calculations

[Namhyung Kim <namhyung@kernel.org>]

On Mon, May 04, 2026 at 09:12:05PM +0530, Athira Rajeev wrote:
> Replace snprintf with scnprintf in buffer offset calculations to
> ensure the 'used' count will not exceed the "len".
> 
> The current logic in perf_pmu__for_each_event uses an unconditional
> + 1 increment to buf_used to account for null terminators. This can
> cause a a stack buffer overflow in the subsequent scnprintf call.
> When the local stack buffer buf (1024 bytes) is full, buf_used can
> reach 1025. This causes the subsequent remaining space calculation
> sizeof(buf) - buf_used to underflow.
> 
> Use sub_non_neg() to see if space actually existed, and only
> increment the offset if remaning space is present.
> 
> Changes includes:
> - Use sub_non_neg to check if space exists
> - Replacing snprintf with scnprintf to ensure the return value
> reflects the actual bytes written into the buffer.
> - Only increment buf_used by 1 if space exists
> - If a parameterized event uses a built-in perf keyword for its
> parameter name (eg, config=?), the lexer parses it as a predefined
> term token, which sets term->config to NULL. Add check to use
> parse_events__term_type_str() if term->config is NULL.
> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
> ---
> Changelog:
> v2 -> v3:
> - Split the scnprintf related changes in separate patch
> - Handle the overflow issues and unconditional increment
> wrapped around sub_non_neg addressing review comment from Sashiko
> 
>  tools/perf/util/pmu.c | 46 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 0b8d58543f17..4b9ade1a4cf9 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -2129,15 +2129,19 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
>  		pr_err("Failure to parse '%s' terms '%s': %d\n",
>  			alias->name, alias->terms, ret);
>  		parse_events_terms__exit(&terms);
> -		snprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
> +		scnprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
>  		return buf;
>  	}
> -	used = snprintf(buf, len, "%.*s/%s", (int)pmu_name_len, pmu->name, alias->name);
> +	used = scnprintf(buf, len, "%.*s/%s", (int)pmu_name_len, pmu->name, alias->name);
>  
>  	list_for_each_entry(term, &terms.terms, list) {
> +		const char *name = term->config;
> +
> +		if (!name)
> +			name = parse_events__term_type_str(term->type_term);
>  		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> -			used += snprintf(buf + used, sub_non_neg(len, used),
> -					",%s=%s", term->config,
> +			used += scnprintf(buf + used, sub_non_neg(len, used),
> +					",%s=%s", name,
>  					term->val.str);
>  	}
>  	parse_events_terms__exit(&terms);
> @@ -2201,6 +2205,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
>  	int ret = 0;
>  	struct hashmap_entry *entry;
>  	size_t bkt;
> +	size_t size_rem, len;
>  
>  	if (perf_pmu__is_tracepoint(pmu))
>  		return tp_pmu__for_each_event(pmu, state, cb);
> @@ -2234,17 +2239,36 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
>  			}
>  			buf_used = strlen(buf) + 1;
>  		}
> +
>  		info.scale_unit = NULL;
>  		if (strlen(event->unit) || event->scale != 1.0) {
> -			info.scale_unit = buf + buf_used;
> -			buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
> -					"%G%s", event->scale, event->unit) + 1;
> +			/* Check the remaining space */
> +			size_rem = sub_non_neg(sizeof(buf), buf_used);
> +
> +			if (size_rem > 0) {
> +				info.scale_unit = buf + buf_used;
> +				len = scnprintf(buf + buf_used, size_rem, "%G%s",
> +						event->scale, event->unit);
> +				/*
> +				 * Increment buf_used by 1 only if
> +				 * it fits remaining space
> +				 */
> +				buf_used += min(len + 1, size_rem);

Hmm.. it seems scnprintf() cannot return a number greater than or equal
to size_rem.  Can we just do like this?

			buf_used += scnprintf(...) + 1;

Thanks,
Namhyung

> +			}
>  		}
>  		info.desc = event->desc;
>  		info.long_desc = event->long_desc;
> -		info.encoding_desc = buf + buf_used;
> -		buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
> -				"%.*s/%s/", (int)pmu_name_len, info.pmu_name, event->terms) + 1;
> +		info.encoding_desc = NULL;
> +
> +		/* Check the remaining space */
> +		size_rem = sub_non_neg(sizeof(buf), buf_used);
> +		if (size_rem > 0) {
> +			info.encoding_desc = buf + buf_used;
> +			len = scnprintf(buf + buf_used, size_rem, "%.*s/%s/",
> +					(int)pmu_name_len, info.pmu_name, event->terms);
> +			buf_used += min(len + 1, size_rem);
> +		}
> +
>  		info.str = event->terms;
>  		info.topic = event->topic;
>  		info.deprecated = perf_pmu_alias__check_deprecated(pmu, event);
> @@ -2254,7 +2278,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
>  	}
>  	if (pmu->selectable) {
>  		info.name = buf;
> -		snprintf(buf, sizeof(buf), "%s//", pmu->name);
> +		scnprintf(buf, sizeof(buf), "%s//", pmu->name);
>  		info.alias = NULL;
>  		info.scale_unit = NULL;
>  		info.desc = NULL;
> -- 
> 2.47.3
>