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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Tue, Jun 09, 2026 at 01:17:17AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Addressing this one in v5.
> 
> > +++ 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?

I've added this to a cleanups TODO file for future work, nice
observation, the same pattern may happen in other places, worth a
series.

- Arnaldo