[PATCH v2 0/3] perf sched stats: Fix memory leaks

[PATCH v2 0/3] perf sched stats: Fix memory leaks

[Namhyung Kim]

Hello,

This is a small series to fix memory leaks in the perf sched stats.
I've tested the changes with the address sanitizer.

v2 changes)
 * remove invalid file closing  (Sashiko)


Thanks,
Namhyung


Namhyung Kim (3):
  perf sched: Add missing perf_session__delete()
  perf sched: Fix memory leaks in perf sched stats report
  perf sched: Free subcommand string after perf sched stats

 tools/perf/builtin-sched.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.55.0.795.g602f6c329a-goog

[PATCH v2 1/3] perf sched: Add missing perf_session__delete()

[Namhyung Kim]

The perf sched stats record missed to release the session and ASAN
reported a leak.

Fixes: c3030995f23b ("perf sched stats: Add record and rawdump support")
Cc: Swapnil Sapkal <swapnil.sapkal@amd.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 6e0d8f4d270e2d88..c5728f3aa46f2630 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -4027,8 +4027,8 @@ static int perf_sched__schedstat_record(struct perf_sched *sched,
 	else
 		fprintf(stderr, "[ perf sched stats: Failed !! ]\n");
 
+	perf_session__delete(session);
 	evlist__put(evlist);
-	close(fd);
 	return err;
 }
 
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v2 2/3] perf sched: Fix memory leaks in perf sched stats report

[Namhyung Kim]

The second pass data is not saved in the list and only used to calculate
delta from the first pass.  Let's free the data after use.

Fixes: 5a357ae6ad63 ("perf sched stats: Add support for report subcommand")
Cc: Swapnil Sapkal <swapnil.sapkal@amd.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index c5728f3aa46f2630..ab9590c4d838eff3 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -4631,6 +4631,8 @@ static int perf_sched__process_schedstat(const struct perf_tool *tool __maybe_un
 			domain_second_pass = list_first_entry(&cpu_second_pass->domain_head,
 							      struct schedstat_domain, domain_list);
 			store_schedstat_cpu_diff(temp);
+			free(temp->cpu_data);
+			free(temp);
 		}
 	} else if (event->header.type == PERF_RECORD_SCHEDSTAT_DOMAIN) {
 		struct schedstat_cpu *cpu_tail;
@@ -4651,6 +4653,8 @@ static int perf_sched__process_schedstat(const struct perf_tool *tool __maybe_un
 		} else {
 			store_schedstat_domain_diff(temp);
 			domain_second_pass = list_next_entry(domain_second_pass, domain_list);
+			free(temp->domain_data);
+			free(temp);
 		}
 	}
 
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v2 3/3] perf sched: Free subcommand string after perf sched stats

[Namhyung Kim]

The first entry of the sched_usage is dynamically allocated in
parse_options_subcommand() so it should be released at the end.
Do not return from a subcommand directly.

Fixes: 064790a3d4a8 ("perf sched stats: Add support for diff subcommand")
Cc: Swapnil Sapkal <swapnil.sapkal@amd.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index ab9590c4d838eff3..acc8d81a20d3b9d1 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -5259,19 +5259,20 @@ int cmd_sched(int argc, const char **argv)
 			if (argc)
 				argc = parse_options(argc, argv, stats_options,
 						     stats_usage, 0);
-			return perf_sched__schedstat_record(&sched, argc, argv);
+			ret = perf_sched__schedstat_record(&sched, argc, argv);
 		} else if (argv[0] && !strcmp(argv[0], "report")) {
 			if (argc)
 				argc = parse_options(argc, argv, stats_options,
 						     stats_usage, 0);
-			return perf_sched__schedstat_report(&sched);
+			ret = perf_sched__schedstat_report(&sched);
 		} else if (argv[0] && !strcmp(argv[0], "diff")) {
 			if (argc)
 				argc = parse_options(argc, argv, stats_options,
 						     stats_usage, 0);
-			return perf_sched__schedstat_diff(&sched, argc, argv);
+			ret = perf_sched__schedstat_diff(&sched, argc, argv);
+		} else {
+			ret = perf_sched__schedstat_live(&sched, argc, argv);
 		}
-		return perf_sched__schedstat_live(&sched, argc, argv);
 	} else {
 		usage_with_options(sched_usage, sched_options);
 	}
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v2 2/3] perf sched: Fix memory leaks in perf sched stats report

[sashiko-bot]

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

Pre-existing issues:
- [High] Memory corruption and NULL pointer dereference when processing an early PERF_RECORD_SCHEDSTAT_DOMAIN event.
- [High] Out-of-bounds memory access and crash during second-pass list iteration due to missing bounds checks.
--

--- Patch [2]: [PATCH v2 2/3] perf sched: Fix memory leaks in perf sched stats report ---
commit e4c15a9e591e133dd7ddf5965e12d5b6792f096e
Author: Namhyung Kim <namhyung@kernel.org>

perf sched: Fix memory leaks in perf sched stats report

The second pass data is not saved in the list and only used to calculate
delta from the first pass. This frees the data after use to prevent leaks.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -4626,11 +4626,13 @@ static int perf_sched__process_schedstat(const struct perf_tool *tool __maybe_un
>  			} else {
>  				cpu_second_pass = list_next_entry(cpu_second_pass, cpu_list);
>  			}

[Severity: High]
This is a pre-existing issue, but can this out-of-bounds iteration cause a
crash if a malformed perf.data file has more CPU events in the second pass
than the first pass?

If the iterator advances past the end of the list, it will point back to the
list head itself. The subsequent code would treat the list head as a valid
struct, read misaligned bytes as a pointer, and dereference it inside
store_schedstat_cpu_diff().

>  			domain_second_pass = list_first_entry(&cpu_second_pass->domain_head,
>  							      struct schedstat_domain, domain_list);
>  			store_schedstat_cpu_diff(temp);
> +			free(temp->cpu_data);
> +			free(temp);
>  		}
>  	} else if (event->header.type == PERF_RECORD_SCHEDSTAT_DOMAIN) {
>  		struct schedstat_cpu *cpu_tail;
[ ... ]
> @@ -4647,10 +4649,12 @@ static int perf_sched__process_schedstat(const struct perf_tool *tool __maybe_un
>  		if (!after_workload_flag) {
>  			cpu_tail = list_last_entry(&cpu_head, struct schedstat_cpu, cpu_list);
>  			list_add_tail(&temp->domain_list, &cpu_tail->domain_head);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
PERF_RECORD_SCHEDSTAT_DOMAIN event to arrive before any
PERF_RECORD_SCHEDSTAT_CPU event?

If the cpu_head list is empty, list_last_entry() evaluates to an invalid
pointer based off the list head offset. The subsequent list_add_tail() would
then access invalid memory when trying to link the elements.

>  		} else {
>  			store_schedstat_domain_diff(temp);
>  			domain_second_pass = list_next_entry(domain_second_pass, domain_list);

[Severity: High]
This is also a pre-existing issue, but similarly to the CPU list, does this
code risk an out-of-bounds access if the second pass has more DOMAIN events
than the first pass?

> +			free(temp->domain_data);
> +			free(temp);
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713234536.2280374-1-namhyung@kernel.org?part=2