[PATCH 0/4] perf tools: Fix memory issues

[PATCH 0/4] perf tools: Fix memory issues

[Michalis Niarchos via B4 Relay]

Running:
  $ perf kvm stat record -a sleep 10

gives this output:
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 1.361 MB perf.data.guest ]
  double free or corruption (!prev)

In the process of resolving this, I came across some memory leaks.

Signed-off-by: Michalis Niarchos <michael.niarchos@gmail.com>
---
Michalis Niarchos (4):
      perf tools: Fix memory leak in cmd_kvm()
      perf tools: Fix memory leak in kvm_events_report()
      perf tools: Fix double free and memory leak in kvm_events_record()
      perf tools: Fix memory leak in cmd_kvm()

 tools/perf/builtin-kvm.c | 63 ++++++++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 26 deletions(-)
---
base-commit: 2162d9b24ba1a9a402baabe5382b33e792aa074f
change-id: 20260731-perf-kvm-fixes-a17f0aa048ad

Best regards,
--  
Michalis Niarchos <michael.niarchos@gmail.com>

[PATCH 1/4] perf tools: Fix memory leak in cmd_kvm()

[Michalis Niarchos via B4 Relay]

From: Michalis Niarchos <michael.niarchos@gmail.com>

filename may get allocated by get_filename_for_perf_kvm(), but is
never freed. Use string literals to remove the need for freeing.

Signed-off-by: Michalis Niarchos <michael.niarchos@gmail.com>
---
 tools/perf/builtin-kvm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 394302ebdb16..44c6998f2ee5 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -606,11 +606,11 @@ static const char *get_filename_for_perf_kvm(void)
 	const char *filename;
 
 	if (perf_host && !perf_guest)
-		filename = strdup("perf.data.host");
+		filename = "perf.data.host";
 	else if (!perf_host && perf_guest)
-		filename = strdup("perf.data.guest");
+		filename = "perf.data.guest";
 	else
-		filename = strdup("perf.data.kvm");
+		filename = "perf.data.kvm";
 
 	return filename;
 }

-- 
2.55.0

Re: [PATCH 1/4] perf tools: Fix memory leak in cmd_kvm()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The error check evaluating `if (!file_name)` immediately following `get_filename_for_perf_kvm()` is functionally dead code and prints an obsolete error message.
--

--- Patch [1]: [PATCH 1/4] perf tools: Fix memory leak in cmd_kvm() ---
commit 85aa78ecdd5ada6c616d04d112b0f46542981162
Author: Michalis Niarchos <michael.niarchos@gmail.com>

perf tools: Fix memory leak in cmd_kvm()

filename may get allocated by get_filename_for_perf_kvm(), but is
never freed. Use string literals to remove the need for freeing.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 394302ebdb161..44c6998f2ee5d 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -606,11 +606,11 @@ static const char *get_filename_for_perf_kvm(void)
>  	const char *filename;
>  
>  	if (perf_host && !perf_guest)
> -		filename = strdup("perf.data.host");
> +		filename = "perf.data.host";
>  	else if (!perf_host && perf_guest)
> -		filename = strdup("perf.data.guest");
> +		filename = "perf.data.guest";
>  	else
> -		filename = strdup("perf.data.kvm");
> +		filename = "perf.data.kvm";
>  
>  	return filename;
>  }

[Severity: Low]
Since get_filename_for_perf_kvm() now exclusively returns static string
literals, it can never return NULL.

Is it worth cleaning up the now-dead error handling block in cmd_kvm() that
still checks for allocation failures?

tools/perf/builtin-kvm.c:cmd_kvm() {
    ...
	if (!file_name) {
		file_name = get_filename_for_perf_kvm();

		if (!file_name) {
			pr_err("Failed to allocate memory for filename\n");
			return -ENOMEM;
		}
	}
    ...
}

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

[PATCH 2/4] perf tools: Fix memory leak in kvm_events_report()

[Michalis Niarchos via B4 Relay]

From: Michalis Niarchos <michael.niarchos@gmail.com>

parse_options() calls parse_options_subcommand(), which allocates the
usage string. Free it.

Signed-off-by: Michalis Niarchos <michael.niarchos@gmail.com>
---
 tools/perf/builtin-kvm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 44c6998f2ee5..1a013698cae3 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1755,6 +1755,9 @@ kvm_events_report(struct perf_kvm_stat *kvm, int argc, const char **argv)
 		if (argc)
 			usage_with_options(kvm_events_report_usage,
 					   kvm_events_report_options);
+
+		/* free usage string allocated by parse_options */
+		free((void *)kvm_events_report_usage[0]);
 	}
 
 #ifndef HAVE_SLANG_SUPPORT

-- 
2.55.0

Re: [PATCH 2/4] perf tools: Fix memory leak in kvm_events_report()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Invalid free of a string literal causes a segmentation fault.
--

--- Patch [2]: [PATCH 2/4] perf tools: Fix memory leak in kvm_events_report() ---
commit b736af0d386356e57597151f619466500011da0a
Author: Michalis Niarchos <michael.niarchos@gmail.com>

perf tools: Fix memory leak in kvm_events_report()

This commit attempts to fix a memory leak in kvm_events_report() by freeing
the usage string after parse_options() completes.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1755,6 +1755,9 @@ kvm_events_report(struct perf_kvm_stat *kvm, int argc, const char **argv)
>  		if (argc)
>  			usage_with_options(kvm_events_report_usage,
>  					   kvm_events_report_options);
> +
> +		/* free usage string allocated by parse_options */
> +		free((void *)kvm_events_report_usage[0]);

[Severity: Medium]
Will this result in an invalid free and a segmentation fault?

Looking at the definition of kvm_events_report_usage in builtin-kvm.c:

const char * const kvm_events_report_usage[] = {
        "perf kvm stat report [<options>]",
        NULL
};

Since parse_options() bypasses allocation when subcommands is NULL, it
appears kvm_events_report_usage[0] remains pointing to the statically
allocated string literal rather than dynamically allocated memory. Freeing a
read-only string literal will crash the perf tool unconditionally on every
successful invocation.

>  	}
>  
>  #ifndef HAVE_SLANG_SUPPORT

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

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

[Michalis Niarchos via B4 Relay]

From: Michalis Niarchos <michael.niarchos@gmail.com>

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.

All the entries of rec_argv come from literals or pointers that are
valid for the lifetime of this call. Reference them directly instead
of duplicating to remove the need to individually track and free each
entry.

Signed-off-by: Michalis Niarchos <michael.niarchos@gmail.com>
---
 tools/perf/builtin-kvm.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 1a013698cae3..189ef3576e34 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1681,18 +1681,18 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(record_args); i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(record_args[i]);
+		rec_argv[i] = record_args[i];
 
 	for (j = 0; j < events_tp_size; j++) {
-		rec_argv[i++] = STRDUP_FAIL_EXIT("-e");
-		rec_argv[i++] = STRDUP_FAIL_EXIT(kvm_events_tp(e_machine)[j]);
+		rec_argv[i++] = "-e";
+		rec_argv[i++] = kvm_events_tp(e_machine)[j];
 	}
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(kvm->file_name);
+	rec_argv[i++] = "-o";
+	rec_argv[i++] = kvm->file_name;
 
 	for (j = 1; j < (unsigned int)argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	set_option_flag(record_options, 'e', "event", PARSE_OPT_HIDDEN);
 	set_option_flag(record_options, 0, "filter", PARSE_OPT_HIDDEN);
@@ -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;
 }

-- 
2.55.0

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

[sashiko-bot]

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

[PATCH 4/4] perf tools: Fix memory leak in cmd_kvm()

[Michalis Niarchos via B4 Relay]

From: Michalis Niarchos <michael.niarchos@gmail.com>

The usage string is allocated by parse_options_subcommand() and freed
only on one return path. Using a single return point guarantees it is
freed on all occasions.

Signed-off-by: Michalis Niarchos <michael.niarchos@gmail.com>
---
 tools/perf/builtin-kvm.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 189ef3576e34..56062c305db1 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2145,6 +2145,7 @@ int cmd_kvm(int argc, const char **argv)
 	const char *const kvm_subcommands[] = { "top", "record", "report", "diff",
 						"buildid-list", "stat", NULL };
 	const char *kvm_usage[] = { NULL, NULL };
+	int ret = 0;
 
 	exclude_GH_default = true;
 	perf_host  = 0;
@@ -2163,29 +2164,39 @@ int cmd_kvm(int argc, const char **argv)
 
 		if (!file_name) {
 			pr_err("Failed to allocate memory for filename\n");
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto exit;
 		}
 	}
 
-	if (strlen(argv[0]) > 2 && strstarts("record", argv[0]))
-		return __cmd_record(file_name, argc, argv);
-	else if (strlen(argv[0]) > 2 && strstarts("report", argv[0]))
-		return __cmd_report(file_name, argc, argv);
-	else if (strlen(argv[0]) > 2 && strstarts("diff", argv[0]))
-		return cmd_diff(argc, argv);
-	else if (!strcmp(argv[0], "top"))
-		return __cmd_top(argc, argv);
-	else if (strlen(argv[0]) > 2 && strstarts("buildid-list", argv[0]))
-		return __cmd_buildid_list(file_name, argc, argv);
+	if (strlen(argv[0]) > 2 && strstarts("record", argv[0])) {
+		ret = __cmd_record(file_name, argc, argv);
+		goto exit;
+	} else if (strlen(argv[0]) > 2 && strstarts("report", argv[0])) {
+		ret = __cmd_report(file_name, argc, argv);
+		goto exit;
+	} else if (strlen(argv[0]) > 2 && strstarts("diff", argv[0])) {
+		ret = cmd_diff(argc, argv);
+		goto exit;
+	} else if (!strcmp(argv[0], "top")) {
+		ret = __cmd_top(argc, argv);
+		goto exit;
+	} else if (strlen(argv[0]) > 2 && strstarts("buildid-list", argv[0])) {
+		ret = __cmd_buildid_list(file_name, argc, argv);
+		goto exit;
+	}
 #if defined(HAVE_LIBTRACEEVENT)
-	else if (strlen(argv[0]) > 2 && strstarts("stat", argv[0]))
-		return kvm_cmd_stat(file_name, argc, argv);
+	else if (strlen(argv[0]) > 2 && strstarts("stat", argv[0])) {
+		ret = kvm_cmd_stat(file_name, argc, argv);
+		goto exit;
+	}
 #endif
 	else
 		usage_with_options(kvm_usage, kvm_options);
 
+exit:
 	/* free usage string allocated by parse_options_subcommand */
 	free((void *)kvm_usage[0]);
 
-	return 0;
+	return ret;
 }

-- 
2.55.0