Re: [PATCH v2 4/4] perf tools: Fix double frees and memory leaks in cmd_kvm()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch fixes memory leaks and double-free vulnerabilities in `__cmd_record`, `__cmd_report`, `__cmd_buildid_list`, and `__cmd_top` by removing `STRDUP_FAIL_EXIT` and the element-wise `free()` loop on `rec_argv`, but it misses applying the exact same fix to `kvm_events_record()` which suffers from the identical bug.
--

--- Patch [4]: [PATCH v2 4/4] perf tools: Fix double frees and memory leaks in cmd_kvm() ---
commit 0b9d8e748caa2edae90fe11689ed1a0bf618e63b
Author: Michalis Niarchos <michael.niarchos@gmail.com>

perf tools: Fix double frees and memory leaks in cmd_kvm()

This commit fixes double free and memory leak vulnerabilities in various
builtin KVM commands (like __cmd_record, __cmd_report, __cmd_buildid_list,
and __cmd_top). It achieves this by removing the use of STRDUP_FAIL_EXIT and
dropping the element-wise free loop on rec_argv.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 9504c83e20744..04cf9bd5b5959 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2018,8 +2018,6 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
>  	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: High]
This is a pre-existing issue and was not introduced by this patch, but does
kvm_events_record() in tools/perf/builtin-kvm.c require this exact same fix?

The commit message notes that the fixed functions follow the same pattern
as kvm_events_record(). However, looking at kvm_events_record(), it appears
it still contains the flawed cleanup loop:

tools/perf/builtin-kvm.c:kvm_events_record() {
    ...
EXIT:
    for (i = 0; i < rec_argc; i++)
        free((void *)rec_argv[i]);
    free(rec_argv);
    return ret;
}

When a user runs 'perf kvm stat record', cmd_record() processes the arguments
by calling parse_options(). Since parse_options() shifts the array in-place
using memmove() to consume recognized arguments, iterating over the original
length of rec_argv (rec_argc) during cleanup encounters duplicated pointers at
the end of the array.

Will leaving this loop in kvm_events_record() lead to a double-free memory
corruption when those duplicate pointers are freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-perf-kvm-fixes-v2-0-1b337189fffc@gmail.com?part=4