Re: [RFT PATCH v2 4/7] perf-x86: iostat: Change iostat_prefix() to static

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Reassignment of `evlist` pointer passed by value in `iostat_prepare` leaves caller with a freed pointer.
- [High] Missing cast for time_t in sprintf leading to argument misalignment and potential segfault.
--

commit 8f78117186fde5ac8fb2f432eade998540fdbd61
Author: Yushan Wang <wangyushan12@huawei.com>

perf-x86: iostat: Change iostat_prefix() to static

This commit changes iostat_prefix() to a static function since it is not
used outside of the local file.

> diff --git a/tools/perf/util/x86-iostat.c b/tools/perf/util/x86-iostat.c
> index 7442a2cd87eda..f8d4c97185949 100644
> --- a/tools/perf/util/x86-iostat.c
> +++ b/tools/perf/util/x86-iostat.c

[ ... ]

> @@ -332,6 +332,28 @@ static int iostat_event_group(struct evlist *evl,
>  	return ret;
>  }
>  
> +static void iostat_prefix(struct evlist *evlist,
> +			  struct perf_stat_config *config,
> +			  char *prefix, struct timespec *ts)
> +{
> +	struct iio_root_port *rp = evlist->selected->priv;
> +
> +	if (rp) {
> +		/*
> +		 * TODO: This is the incorrect format in JSON mode.
> +		 *       See prepare_timestamp()
> +		 */
> +		if (ts)
> +			sprintf(prefix, "%6lu.%09lu%s%04x:%02x%s",
> +				ts->tv_sec, ts->tv_nsec,
> +				config->csv_sep, rp->domain, rp->bus,
> +				config->csv_sep);

Does this format specifier cause a segmentation fault on 32-bit systems with
64-bit time support?

When _TIME_BITS=64 is used on a 32-bit system, time_t becomes 64-bit 
(8 bytes), but unsigned long remains 32-bit (4 bytes).

Because sprintf() is a variadic function, passing the 64-bit ts->tv_sec takes
two 32-bit slots on the stack. The %6lu specifier reads the first 4 bytes,
%09lu reads the upper half of tv_sec, and the %s specifier reads ts->tv_nsec
as a pointer.

Dereferencing that integer as a string pointer would crash.

> +		else
> +			sprintf(prefix, "%04x:%02x%s", rp->domain, rp->bus,
> +				config->csv_sep);
> +	}
> +}
> +
>  int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config)
>  {
>  	if (evlist->core.nr_entries > 0) {

Can this pointer reassignment cause a use-after-free regression for the
caller?

Looking at iostat_prepare(), it takes evlist as a pointer passed by value.
If the core entries are greater than zero, the evlist is deleted and a new
one is allocated:

tools/perf/util/x86-iostat.c:iostat_prepare() {
    ...
    if (evlist->core.nr_entries > 0) {
        ...
        evlist__delete(evlist);
        evlist = evlist__new();
    ...
}

Since the evlist reassignment only modifies the local parameter variable,
the caller's pointer is left pointing to freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507063737.3542950-1-wangyushan12@huawei.com?part=4