Re: [PATCH 3/4] perf tools: Fix double free and memory leak in kvm_events_record()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The patch resolves a double-free and memory leak in `kvm_events_record()` but leaves identical vulnerabilities unpatched in `__cmd_record()`, `__cmd_report()`, `__cmd_buildid_list()`, and `__cmd_top()`.
--

--- Patch [3]: [PATCH 3/4] perf tools: Fix double free and memory leak in kvm_events_record() ---
commit 03f3f88662347e4848a16bffad56206678f9263b
Author: Michalis Niarchos <michael.niarchos@gmail.com>

perf tools: Fix double free and memory leak in kvm_events_record()

cmd_record() reorders the contents of the rec_argv pointer array, so
its entry order no longer matches the order in which the caller
originally allocated them. Freeing the contents of rec_argv by
iterating the reordered array results in some pointers being freed
twice and others never freed. This patch fixes it by referencing the
arguments directly instead of duplicating them.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 1a013698cae34..189ef3576e34e 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
[ ... ]
> @@ -1717,9 +1717,6 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
>  	record_usage = kvm_stat_record_usage;
>  	ret = cmd_record(i, rec_argv);
>  
> -EXIT:
> -	for (i = 0; i < rec_argc; i++)
> -		free((void *)rec_argv[i]);
>  	free(rec_argv);
>  	return ret;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but since you are fixing the
double-free and memory leak here, should the same fix be applied to other
functions in this file that follow the exact same pattern?

Looking at tools/perf/builtin-kvm.c, the same iteration and free approach
is still used in __cmd_record(), __cmd_report(), __cmd_buildid_list(), and
__cmd_top(). For example, in __cmd_record():

	ret = cmd_record(i, rec_argv);

	for (i = 0; i < rec_argc; i++)
		free((void *)rec_argv[i]);

Because parse_options() shifts elements around in rec_argv during the
cmd_*() calls, wouldn't these other functions also suffer from the same
double-free and memory leak when the elements are freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-perf-kvm-fixes-v1-0-a5db849ac973@gmail.com?part=3