Re: [PATCH 3/4] perf kvm: Kill STRDUP_FAIL_EXIT()

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Jun 23, 2026 at 07:12:59AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Memory leak of dynamically allocated strings in PowerPC default arch events due to removal of array cleanup loops.
> 
> Pre-existing issues:
> - [High] perf kvm record unconditionally crashes via BUG_ON() on non-Intel x86 hosts and architectures without default events.
> --
> 
> commit d3c8251de71559c69211fb2d23ba99dd14669107
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf kvm: Kill STRDUP_FAIL_EXIT()
> 
> This commit removes the STRDUP_FAIL_EXIT macro and replaces dynamically
> allocated copied strings with stack variables for passing command line
> options. It also removes the cleanup loops that freed these string
> copies, addressing a subtle double free issue.
> 
> > diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> > index acc6f2a706b35..477c28915c037 100644
> > --- a/tools/perf/builtin-kvm.c
> > +++ b/tools/perf/builtin-kvm.c
> [ ... ]
> > @@ -2008,9 +2004,9 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
> [ ... ]
> >  	for (j = 1; j < argc; j++, i++)
> > -		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
> > +		rec_argv[i] = argv[j];
> >  
> >  	BUG_ON(i != rec_argc);
> 
> [Severity: High]
> This is a pre-existing issue, but can this BUG_ON() crash the tool on
> non-Intel x86 hosts or architectures without default events?
> 
> If need_arch_event is true but the architecture hook returns early without
> adding events, rec_argc will be greater than the final value of i.
> 
> For example, in tools/perf/util/kvm-stat-arch/kvm-stat-x86.c:
> 
> __kvm_add_default_arch_event_x86() {
>         if (!x86__is_intel_cpu())
>                 return 0;
>         ...
> }
> 
> On AMD hosts, this returns 0 without incrementing the argument count, leaving
> i less than rec_argc and triggering the BUG_ON() assertion.
> 
> >  
> >  	ret = cmd_record(i, rec_argv);
> >  
> >  EXIT:
> > -	for (j = 0; j < i; j++)
> > -		free((void *)rec_argv[j]);
> 
> [Severity: Medium]
> Does removing this cleanup loop leak memory on PowerPC architectures?
> 
> While the central code now uses stack variables, the PowerPC architecture
> hook still dynamically allocates its default events. 
> 
> In tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c:
> 
> __kvm_add_default_arch_event_powerpc() {
>         ...
>         argv[j++] = strdup("-e");
>         argv[j++] = strdup("trace_imc/trace_cycles/");
>         ...
> }
> 
> Since these strings are allocated with strdup() and the free() loops in
> __cmd_record(), __cmd_top(), and kvm_events_record() have been removed,
> will these strings leak during command execution?

Right, will update powerpc.

Thanks,
Namhyung

> 
> >  	free(rec_argv);
> >  	return ret;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260623070313.55225-1-namhyung@kernel.org?part=3