Re: [PATCH 10/11] perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] build_id__snprintf() fails to null-terminate the output buffer when bf_size is 1.
- [Low] hwmon_pmu__describe_items() spins doing useless sysfs operations once the output buffer fills.
--

commit 0f670823c7b8e81b469db29dc89cf4b4de7e4cad
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()

This commit switches build_id__snprintf() and hwmon_pmu__describe_items()
to use scnprintf() instead of snprintf() to prevent out-of-bounds length
tracking. It also modifies the loop guard in build_id__snprintf() to
prevent useless zero-byte writes once the buffer is full.

> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 8c0a9ae932aa5..82af3dca7e2fb 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -93,8 +93,8 @@ int build_id__snprintf(const struct build_id *build_id, char *bf, size_t bf_size
>  		return 0;
>  	}
>  
> -	for (size_t i = 0; i < build_id->size && offs < bf_size; ++i)
> -		offs += snprintf(bf + offs, bf_size - offs, "%02x", build_id->data[i]);
> +	for (size_t i = 0; i < build_id->size && offs + 1 < bf_size; ++i)
> +		offs += scnprintf(bf + offs, bf_size - offs, "%02x", build_id->data[i]);

[Severity: Medium]
If bf_size is 1, does this new loop condition skip execution entirely?

If build_id->size > 0 and bf_size == 1, the offs + 1 < bf_size condition
is false on the first iteration. Because scnprintf is never called, would
bf[0] be left un-null-terminated?

>  	return offs;
>  }

[ ... ]

> diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
> index fb3ffa8d32ad2..dbf6a71af47f9 100644
> --- a/tools/perf/util/hwmon_pmu.c
> +++ b/tools/perf/util/hwmon_pmu.c
[ ... ]
> @@ -442,12 +442,12 @@ static size_t hwmon_pmu__describe_items(struct hwmon_pmu *hwm, char *out_buf, si
>  
>  				buf[read_len] = '\0';
>  				val = strtoll(buf, /*endptr=*/NULL, 10);
> -				len += snprintf(out_buf + len, out_buf_len - len, "%s%s%s=%g%s",
> -						len == 0 ? " " : ", ",
> -						hwmon_item_strs[bit],
> -						is_alarm ? "_alarm" : "",
> -						(double)val / 1000.0,
> -						hwmon_units[key.type]);
> +				len += scnprintf(out_buf + len, out_buf_len - len, "%s%s%s=%g%s",
> +						 len == 0 ? " " : ", ",
> +						 hwmon_item_strs[bit],
> +						 is_alarm ? "_alarm" : "",
> +						 (double)val / 1000.0,
> +						 hwmon_units[key.type]);
>  			}
>  			close(fd);
>  		}

[Severity: Low]
Does this loop also need an early break condition once the buffer fills?

The commit message notes that build_id__snprintf() was updated to avoid
spinning doing zero-byte writes once the buffer fills. 

In hwmon_pmu__describe_items(), once len reaches out_buf_len - 1, scnprintf
will return 0. Does this mean the loop continues for all remaining hardware
monitor items, performing unnecessary sysfs file open, read, and close
operations only to do a 0-byte scnprintf write?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609010526.1998472-1-acme@kernel.org?part=10