[PATCH v1 00/13] perf python: Fix python extension bugs and memory leaks

[PATCH v1 00/13] perf python: Fix python extension bugs and memory leaks

[Ian Rogers]

This patch series addresses a number of leaks and python extension bugs
discovered during the review of the python scripting phase 1 refactoring
series.

These fixes have been separated from the scripting changes into their own
dedicated patch series to ease review and integration, as they address
regressions or existing bugs rather than adding new features.

The series breaks down the monolithic python extension fixes into discrete
logical patches addressing validation, initialization, and resource leaks.

Ian Rogers (13):
  perf script: Fix metric_evlist leak in script_find_metrics
  perf stat: Fix evsel_list leak in cmd_stat
  perf top: Fix sb_evlist leak
  perf python: Fix memory leak in pyrf_evlist__get_pollfd
  perf synthetic-events: Fix uninitialized pthread_join
  perf data: Fix directory file descriptor leak in perf_data__switch
  perf test: Fix skiplist leak in cmd_test
  perf python: Check counts_values size in set_values
  perf python: Validate CPU and thread maps in pyrf_evsel__open
  perf python: Validate attribute setters in pyrf_evsel
  perf python: Zero initialize perf_data in pyrf_data__init
  perf python: Add thread uninitialized checks
  perf python: Fix MetricGroup return type in perf.pyi

 tools/perf/builtin-script.c        |   2 +-
 tools/perf/builtin-stat.c          |   9 ++-
 tools/perf/builtin-top.c           |   1 +
 tools/perf/python/perf.pyi         |   4 +-
 tools/perf/tests/builtin-test.c    |   5 +-
 tools/perf/util/data.c             |   3 +-
 tools/perf/util/python.c           | 114 +++++++++++++++++++++++++----
 tools/perf/util/synthetic-events.c |   4 +-
 8 files changed, 116 insertions(+), 26 deletions(-)

-- 
2.55.0.rc0.786.g65d90a0328-goog

[PATCH v1 01/13] perf script: Fix metric_evlist leak in script_find_metrics

[Ian Rogers]

Fix a memory leak in script_find_metrics() where metric_evlist is leaked
when returning early on error paths.

Assisted-by: Antigravity:gemini-3.1-pro
Fixes: 3622990efaab ("perf script: Change metric format to use json metrics")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-script.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 47afd8cdc2b7..6b69e982ba62 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -2275,8 +2275,8 @@ static int script_find_metrics(const struct pmu_metric *pm,
 	}
 	pr_debug("Found metric '%s' whose evsels match those of in the perf data\n",
 		 pm->metric_name);
-	evlist__put(metric_evlist);
 out:
+	evlist__put(metric_evlist);
 	return 0;
 }
 
-- 
2.55.0.rc0.786.g65d90a0328-goog

[PATCH v1 02/13] perf stat: Fix evsel_list leak in cmd_stat

[Ian Rogers]

Fix a memory leak in cmd_stat() where evsel_list is leaked if an error
occurs while opening the output file.

Assisted-by: Antigravity:gemini-3.1-pro
Fixes: 361c99a661a7 ("perf evsel: Introduce perf_evlist")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-stat.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 3f897b2e8638..7cb6ea937e87 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2718,7 +2718,8 @@ int cmd_stat(int argc, const char **argv)
 		output = fopen(output_name, mode);
 		if (!output) {
 			perror("failed to create output file");
-			return -1;
+			status = -1;
+			goto out;
 		}
 		if (!stat_config.json_output) {
 			clock_gettime(CLOCK_REALTIME, &tm);
@@ -2729,7 +2730,8 @@ int cmd_stat(int argc, const char **argv)
 		output = fdopen(output_fd, mode);
 		if (!output) {
 			perror("Failed opening logfd");
-			return -errno;
+			status = -errno;
+			goto out;
 		}
 	}
 
@@ -2738,7 +2740,8 @@ int cmd_stat(int argc, const char **argv)
 		parse_options_usage(stat_usage, stat_options, "o", 1);
 		parse_options_usage(NULL, stat_options, "log-fd", 0);
 		parse_options_usage(NULL, stat_options, "interval-clear", 0);
-		return -1;
+		status = -1;
+		goto out;
 	}
 
 	stat_config.output = output;
-- 
2.55.0.rc0.786.g65d90a0328-goog

[PATCH v1 03/13] perf top: Fix sb_evlist leak

[Ian Rogers]

Fix a memory leak in cmd_top() where top.sb_evlist was not freed if
evlist__add_bpf_sb_event() fails. Note that evlist__start_sb_thread() and
evlist__stop_sb_thread() take ownership of the evlist and free it, so
we must only free it if we fail before starting the thread.

Assisted-by: Antigravity:gemini-3.1-pro
Fixes: b38d85ef49cf ("perf bpf: Decouple creating the evlist from adding the SB event")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-top.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 5933c46ee137..abc74b8ab759 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1881,6 +1881,7 @@ int cmd_top(int argc, const char **argv)
 		if (evlist__add_bpf_sb_event(top.sb_evlist, &host_env)) {
 			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
 			status = -EINVAL;
+			evlist__put(top.sb_evlist);
 			goto out_put_evlist;
 		}
 	}
-- 
2.55.0.rc0.786.g65d90a0328-goog

Re: [PATCH v1 03/13] perf top: Fix sb_evlist leak

[sashiko-bot]

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

Pre-existing issues:
- [High] Error path in `record__setup_sb_evlist()` bypasses state update, leading to a segmentation fault from an uninitialized `pthread_join()`.
- [High] Sideband thread and `sb_evlist` are leaked when using `--switch-output-event` together with `--no-bpf-event`.
--

commit 9bc374ffd1b7ae00f5342c051ca479b32bc92942
Author: Ian Rogers <irogers@google.com>

perf top: Fix sb_evlist leak

Fix a memory leak in cmd_top() where top.sb_evlist was not freed if
evlist__add_bpf_sb_event() fails. Note that evlist__start_sb_thread() and
evlist__stop_sb_thread() take ownership of the evlist and free it, so
we must only free it if we fail before starting the thread.

> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 5933c46ee137e..abc74b8ab7599 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1881,6 +1881,7 @@ int cmd_top(int argc, const char **argv)
>  		if (evlist__add_bpf_sb_event(top.sb_evlist, &host_env)) {
>  			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
>  			status = -EINVAL;
> +			evlist__put(top.sb_evlist);
>  			goto out_put_evlist;
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does tools/perf/builtin-record.c have a
similar problem where the error path in record__setup_sb_evlist() bypasses
updating the state?

If evlist__add_bpf_sb_event() fails in record__setup_sb_evlist(), it
returns -1 without updating the opts->no_bpf_event flag:

tools/perf/builtin-record.c:record__setup_sb_evlist() {
    ...
	if (evlist__add_bpf_sb_event(rec->sb_evlist, perf_session__env(rec->session))) {
		pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
		return -1;
	}
    ...
}

During cleanup in __cmd_record(), the unmodified no_bpf_event flag causes
evlist__stop_sb_thread() to be unconditionally invoked:

tools/perf/builtin-record.c:__cmd_record() {
    ...
	if (!opts->no_bpf_event)
		evlist__stop_sb_thread(rec->sb_evlist);
    ...
}

This could blindly call pthread_join() on the uninitialized thread ID in
evlist__stop_sb_thread():

tools/perf/util/sideband_evlist.c:evlist__stop_sb_thread() {
    ...
	pthread_join(*evlist__sb_thread_th(evlist), NULL);
    ...
}

Could this trigger undefined behavior and a crash?

[Severity: High]
This is also a pre-existing issue, but is there a leak of the sideband
thread and sb_evlist in tools/perf/builtin-record.c when using
--switch-output-event together with --no-bpf-event?

In record__setup_sb_evlist(), the sideband thread is started when using
--switch-output-event, even if BPF events are disabled:

tools/perf/builtin-record.c:record__setup_sb_evlist() {
    ...
	if (evlist__start_sb_thread(rec->sb_evlist, &rec->opts.target)) {
    ...
}

However, cleanup in __cmd_record() is bypassed because opts->no_bpf_event
evaluates to true:

tools/perf/builtin-record.c:__cmd_record() {
    ...
	if (!opts->no_bpf_event)
		evlist__stop_sb_thread(rec->sb_evlist);
    ...
}

Does this permanently strand the background poll thread and leak the
allocated evlist during process teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=3

[PATCH v1 04/13] perf python: Fix memory leak in pyrf_evlist__get_pollfd

[Ian Rogers]

Fix a Python list object leak in pyrf_evlist__get_pollfd() by adding
a missing Py_DECREF on the error exit path.

Assisted-by: Antigravity:gemini-3.1-pro
Fixes: 877108e42b1b ("perf tools: Initial python binding")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 11e1f39c1bea..21e8b5a23a1c 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -2843,6 +2843,7 @@ static PyObject *pyrf_evlist__get_pollfd(struct pyrf_evlist *pevlist,
 
 	return list;
 free_list:
+	Py_DECREF(list);
 	return PyErr_NoMemory();
 }
 
-- 
2.55.0.rc0.786.g65d90a0328-goog

Re: [PATCH v1 04/13] perf python: Fix memory leak in pyrf_evlist__get_pollfd

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added `Py_DECREF(list)` introduces a segmentation fault on the error path if the initial `PyList_New(0)` allocation fails.
--

commit dfef1d27b0ba6fba485792f67daeb68ccdaac6cd
Author: Ian Rogers <irogers@google.com>

perf python: Fix memory leak in pyrf_evlist__get_pollfd

Fix a Python list object leak in pyrf_evlist__get_pollfd() by adding
a missing Py_DECREF on the error exit path.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 11e1f39c1bea9..21e8b5a23a1c9 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -2843,6 +2843,7 @@ static PyObject *pyrf_evlist__get_pollfd(struct pyrf_evlist *pevlist,
>  
>  	return list;
>  free_list:
> +	Py_DECREF(list);

[Severity: High]
If PyList_New(0) fails and returns NULL, this will attempt to decrement the
reference count of a NULL pointer.

Because Py_DECREF unconditionally dereferences its argument, will this result
in a segmentation fault on the error path?

Should we check list for NULL earlier, or use Py_XDECREF(list) instead?

>  	return PyErr_NoMemory();
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=4

[PATCH v1 05/13] perf synthetic-events: Fix uninitialized pthread_join

[Ian Rogers]

In perf_event__synthesize_threads(), fix an uninitialized pthread_join()
call when thread creation fails by only joining the successfully
created threads.

Assisted-by: Antigravity:gemini-3.1-pro
Fixes: 340b47f510bb ("perf top: Implement multithreading for perf_event__synthesize_threads")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/synthetic-events.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index b75f9dcf4dbf..35de41ad14d3 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1104,8 +1104,8 @@ int perf_event__synthesize_threads(const struct perf_tool *tool,
 	}
 	err = 0;
 out_join:
-	for (i = 0; i < thread_nr; i++)
-		pthread_join(synthesize_threads[i], NULL);
+	for (j = 0; j < i; j++)
+		pthread_join(synthesize_threads[j], NULL);
 	free(args);
 free_threads:
 	free(synthesize_threads);
-- 
2.55.0.rc0.786.g65d90a0328-goog

Re: [PATCH v1 05/13] perf synthetic-events: Fix uninitialized pthread_join

[sashiko-bot]

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

Pre-existing issues:
- [High] Divide by zero crash in perf_event__synthesize_threads when no tasks are found.
--

commit 30888fa2ce27193736ae38293c89b11a5e47a514
Author: Ian Rogers <irogers@google.com>

perf synthetic-events: Fix uninitialized pthread_join

This commit fixes an uninitialized pthread_join() call when thread creation
fails in perf_event__synthesize_threads() by ensuring only successfully
created threads are joined.

> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index b75f9dcf4dbfe..35de41ad14d33 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -1104,8 +1104,8 @@ int perf_event__synthesize_threads(const struct perf_tool *tool,

[Severity: High]
This is a pre-existing issue, but does this code suffer from a fatal
division by zero when no tasks are found?

If scandir() finds no matching tasks in /proc (which can happen in an empty
chroot or isolated container), n is 0. If the system has multiple CPU cores
(thread_nr > 1), we bypass the single-thread fast path.

perf_event__synthesize_threads() then clamps thread_nr:

    if (thread_nr > n)
        thread_nr = n;

And shortly after evaluates:

    num_per_thread = n / thread_nr;

Does this divide by zero (0 / 0) and cause the perf tool to crash with a
fatal SIGFPE? Could we handle the n == 0 case earlier to prevent this
regression?

>  	}
>  	err = 0;
>  out_join:
> -	for (i = 0; i < thread_nr; i++)
> -		pthread_join(synthesize_threads[i], NULL);
> +	for (j = 0; j < i; j++)
> +		pthread_join(synthesize_threads[j], NULL);
>  	free(args);
>  free_threads:
>  	free(synthesize_threads);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=5

[PATCH v1 06/13] perf data: Fix directory file descriptor leak in perf_data__switch

[Ian Rogers]

Fix a file descriptor leak in perf_data__switch() when running in
directory mode. By substituting perf_data__close() for
perf_data_file__close(), any thread-local directory files are properly
closed during file rotation.

Assisted-by: Antigravity:gemini-3.1-pro
Fixes: 040f9915e99e ("perf data: Add perf_data_file__switch() helper")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/data.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 33fd1b82019e..32d4d908ad38 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -472,8 +472,7 @@ int perf_data__switch(struct perf_data *data,
 		pr_warning("Failed to rename %s to %s\n", data->path, *new_filepath);
 
 	if (!at_exit) {
-		perf_data_file__close(&data->file);
-		data->open = false;
+		perf_data__close(data);
 		ret = perf_data__open(data);
 		if (ret < 0)
 			goto out;
-- 
2.55.0.rc0.786.g65d90a0328-goog

[PATCH v1 07/13] perf test: Fix skiplist leak in cmd_test

[Ian Rogers]

Fix a memory leak in cmd_test() where skiplist was not freed on
exit paths.

Assisted-by: Antigravity:gemini-3.1-pro
Fixes: 2ae828786c65 ("perf test: Allow skipping tests")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 7e75f590f225..cf5625cf4f11 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -1781,9 +1781,12 @@ int cmd_test(int argc, const char **argv)
 	rlimit__bump_memlock();
 
 	suites = build_suites();
-	if (!suites)
+	if (!suites) {
+		intlist__delete(skiplist);
 		return errno ? -errno : -ENOMEM;
+	}
 	ret = __cmd_test(suites, argc, argv, skiplist);
 	free(suites);
+	intlist__delete(skiplist);
 	return ret;
 }
-- 
2.55.0.rc0.786.g65d90a0328-goog

[PATCH v1 08/13] perf python: Check counts_values size in set_values

[Ian Rogers]

The set_values function incorrectly assumed the list contained exactly
5 elements. Add a check to prevent out-of-bounds access.

Fixes: 877108e42b1b ("perf tools: Initial python binding")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 21e8b5a23a1c..f9398bc30a80 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1913,6 +1913,11 @@ static int pyrf_counts_values_set_values(struct pyrf_counts_values *self, PyObje
 	}
 
 	size = PyList_Size(list);
+	if (size != 5) {
+		PyErr_SetString(PyExc_ValueError, "List must have exactly 5 entries");
+		return -1;
+	}
+
 	for (Py_ssize_t i = 0; i < size; i++) {
 		item = PyList_GetItem(list, i);
 		if (!PyLong_Check(item)) {
-- 
2.55.0.rc0.786.g65d90a0328-goog

Re: [PATCH v1 08/13] perf python: Check counts_values size in set_values

[sashiko-bot]

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

Pre-existing issues:
- [High] The Python bindings for `perf_counts_values` incorrectly use 32-bit-limited C `long` functions and types for `uint64_t` data, leading to data truncation on 32-bit platforms, incorrect negative values, and unhandled `OverflowError` exceptions causing fatal `SystemError` crashes.
- [High] Deleting the `.values` attribute from Python causes a segmentation fault due to a NULL pointer dereference in the property setter.
--

commit a4cb87d6d79ff9be6c884154fa1d1f913fe545d5
Author: Ian Rogers <irogers@google.com>

perf python: Check counts_values size in set_values

The set_values function incorrectly assumed the list contained exactly
5 elements. Add a check to prevent out-of-bounds access.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 21e8b5a23a1c9..f9398bc30a80b 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -1913,6 +1913,11 @@ static int pyrf_counts_values_set_values(struct pyrf_counts_values *self, PyObje
>  	}

[Severity: High]
This is a pre-existing issue, but does this function cause a NULL pointer 
dereference when the attribute is deleted?

Just above this context in pyrf_counts_values_set_values():

	if (!PyList_Check(list)) {
		PyErr_SetString(PyExc_TypeError, "Value assigned must be a list");
		return -1;
	}

If a user executes del obj.values from Python code, the Python C API calls 
the property setter with list set to NULL. Passing a NULL list directly into 
PyList_Check(list) will dereference the object to check its type, which 
causes an immediate segmentation fault.

>  
>  	size = PyList_Size(list);
> +	if (size != 5) {
> +		PyErr_SetString(PyExc_ValueError, "List must have exactly 5 entries");
> +		return -1;
> +	}
> +
>  	for (Py_ssize_t i = 0; i < size; i++) {
>  		item = PyList_GetItem(list, i);
>  		if (!PyLong_Check(item)) {

[Severity: High]
This is a pre-existing issue, but can this loop lead to data truncation or
crash the interpreter?

Just below this context in pyrf_counts_values_set_values():

		self->values.values[i] = PyLong_AsLong(item);
	}

Since self->values.values stores uint64_t data, assigning the result of
PyLong_AsLong (which returns a signed C long) can truncate data on 32-bit
platforms or produce incorrect negative values.

Additionally, if PyLong_AsLong raises an OverflowError for very large
integers, the error is silently ignored and 0 is returned to the interpreter,
which triggers a fatal SystemError crash.

The same underlying issue appears in other places where we expose uint64_t
data:

tools/perf/util/python.c:pyrf_counts_values_get_values()
	for (int i = 0; i < 5; i++)
		PyList_SetItem(vals, i, PyLong_FromLong(self->values.values[i]));

tools/perf/util/python.c:pyrf_counts_values_members
	counts_values_member_def(val, T_ULONG, "Value of event"),

Should we be using 64-bit functions like PyLong_AsUnsignedLongLong() and
PyLong_FromUnsignedLongLong(), and ensuring exceptions are caught?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=8

[PATCH v1 09/13] perf python: Validate CPU and thread maps in pyrf_evsel__open

[Ian Rogers]

Add explicit Py_TYPE checks to ensure the arguments passed are
actually of the correct pyrf_thread_map and pyrf_cpu_map types.

Fixes: 877108e42b1b ("perf tools: Initial python binding")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index f9398bc30a80..62f404c40d8d 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -2096,11 +2096,21 @@ static PyObject *pyrf_evsel__open(struct pyrf_evsel *pevsel,
 					 &pcpus, &pthreads, &group, &inherit))
 		return NULL;
 
-	if (pthreads != NULL && pthreads != Py_None)
+	if (pthreads != NULL && pthreads != Py_None) {
+		if (Py_TYPE(pthreads) != &pyrf_thread_map__type) {
+			PyErr_SetString(PyExc_TypeError, "threads must be a thread_map");
+			return NULL;
+		}
 		threads = ((struct pyrf_thread_map *)pthreads)->threads;
+	}
 
-	if (pcpus != NULL && pcpus != Py_None)
+	if (pcpus != NULL && pcpus != Py_None) {
+		if (Py_TYPE(pcpus) != &pyrf_cpu_map__type) {
+			PyErr_SetString(PyExc_TypeError, "cpus must be a cpu_map");
+			return NULL;
+		}
 		cpus = ((struct pyrf_cpu_map *)pcpus)->cpus;
+	}
 
 	evsel->core.attr.inherit = inherit;
 	/*
-- 
2.55.0.rc0.786.g65d90a0328-goog

Re: [PATCH v1 09/13] perf python: Validate CPU and thread maps in pyrf_evsel__open

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The explicit `Py_TYPE()` checks added to `pyrf_evsel__open` break Python subclassing for `perf.thread_map` and `perf.cpu_map` objects.
--

commit 909b91c5a1d04af18afad351b78395740e856d0b
Author: Ian Rogers <irogers@google.com>

perf python: Validate CPU and thread maps in pyrf_evsel__open

This commit adds explicit Py_TYPE checks to ensure the cpu and thread map
arguments passed to pyrf_evsel__open are of the correct types.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index f9398bc30a80b..62f404c40d8dc 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> @@ -2096,11 +2096,21 @@ static PyObject *pyrf_evsel__open(struct pyrf_evsel *pevsel,
>  					 &pcpus, &pthreads, &group, &inherit))
>  		return NULL;
>  
> -	if (pthreads != NULL && pthreads != Py_None)
> +	if (pthreads != NULL && pthreads != Py_None) {
> +		if (Py_TYPE(pthreads) != &pyrf_thread_map__type) {

[Severity: Medium]
Will this strict equality check break valid Python subclasses of thread_map?

Because pyrf_thread_map__type sets Py_TPFLAGS_BASETYPE, Python scripts are
permitted to subclass it, but Py_TYPE() will point to the subclass type and
fail this exact-type check.

Should this use PyObject_TypeCheck() instead to safely support subclassing?

> +			PyErr_SetString(PyExc_TypeError, "threads must be a thread_map");
> +			return NULL;
> +		}
>  		threads = ((struct pyrf_thread_map *)pthreads)->threads;
> +	}
>  
> -	if (pcpus != NULL && pcpus != Py_None)
> +	if (pcpus != NULL && pcpus != Py_None) {
> +		if (Py_TYPE(pcpus) != &pyrf_cpu_map__type) {

[Severity: Medium]
Similarly, does this strict equality block valid Python subclasses of cpu_map?

> +			PyErr_SetString(PyExc_TypeError, "cpus must be a cpu_map");
> +			return NULL;
> +		}
>  		cpus = ((struct pyrf_cpu_map *)pcpus)->cpus;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=9

[PATCH v1 10/13] perf python: Validate attribute setters in pyrf_evsel

[Ian Rogers]

If val is NULL when setting an attribute, PyErr_SetString should be
called as deleting the attribute isn't supported. In addition, ensure
PyErr_Occurred is checked before setting the attribute to avoid setting
a garbage value.

Fixes: 877108e42b1b ("perf tools: Initial python binding")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 89 ++++++++++++++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 62f404c40d8d..e277c89b8347 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -2284,6 +2284,11 @@ static int pyrf_evsel__set_tracking(PyObject *self, PyObject *val, void *closure
 
 	CHECK_INITIALIZED_INT(pevsel->evsel, "evsel");
 
+	if (val == NULL) {
+		PyErr_SetString(PyExc_TypeError, "cannot delete attribute");
+		return -1;
+	}
+
 	is_true = PyObject_IsTrue(val);
 	if (is_true < 0)
 		return -1;
@@ -2295,11 +2300,21 @@ static int pyrf_evsel__set_tracking(PyObject *self, PyObject *val, void *closure
 static int pyrf_evsel__set_attr_config(PyObject *self, PyObject *val, void *closure __maybe_unused)
 {
 	struct pyrf_evsel *pevsel = (void *)self;
+	unsigned long long new_val;
 
 	CHECK_INITIALIZED_INT(pevsel->evsel, "evsel");
 
-	pevsel->evsel->core.attr.config = PyLong_AsUnsignedLongLong(val);
-	return PyErr_Occurred() ? -1 : 0;
+	if (val == NULL) {
+		PyErr_SetString(PyExc_TypeError, "cannot delete attribute");
+		return -1;
+	}
+
+	new_val = PyLong_AsUnsignedLongLong(val);
+	if (PyErr_Occurred())
+		return -1;
+
+	pevsel->evsel->core.attr.config = new_val;
+	return 0;
 }
 
 static PyObject *pyrf_evsel__get_attr_config(PyObject *self, void *closure __maybe_unused)
@@ -2314,11 +2329,21 @@ static PyObject *pyrf_evsel__get_attr_config(PyObject *self, void *closure __may
 static int pyrf_evsel__set_attr_read_format(PyObject *self, PyObject *val, void *closure __maybe_unused)
 {
 	struct pyrf_evsel *pevsel = (void *)self;
+	unsigned long long new_val;
 
 	CHECK_INITIALIZED_INT(pevsel->evsel, "evsel");
 
-	pevsel->evsel->core.attr.read_format = PyLong_AsUnsignedLongLong(val);
-	return PyErr_Occurred() ? -1 : 0;
+	if (val == NULL) {
+		PyErr_SetString(PyExc_TypeError, "cannot delete attribute");
+		return -1;
+	}
+
+	new_val = PyLong_AsUnsignedLongLong(val);
+	if (PyErr_Occurred())
+		return -1;
+
+	pevsel->evsel->core.attr.read_format = new_val;
+	return 0;
 }
 
 static PyObject *pyrf_evsel__get_attr_read_format(PyObject *self, void *closure __maybe_unused)
@@ -2333,11 +2358,21 @@ static PyObject *pyrf_evsel__get_attr_read_format(PyObject *self, void *closure
 static int pyrf_evsel__set_attr_sample_period(PyObject *self, PyObject *val, void *closure __maybe_unused)
 {
 	struct pyrf_evsel *pevsel = (void *)self;
+	unsigned long long new_val;
 
 	CHECK_INITIALIZED_INT(pevsel->evsel, "evsel");
 
-	pevsel->evsel->core.attr.sample_period = PyLong_AsUnsignedLongLong(val);
-	return PyErr_Occurred() ? -1 : 0;
+	if (val == NULL) {
+		PyErr_SetString(PyExc_TypeError, "cannot delete attribute");
+		return -1;
+	}
+
+	new_val = PyLong_AsUnsignedLongLong(val);
+	if (PyErr_Occurred())
+		return -1;
+
+	pevsel->evsel->core.attr.sample_period = new_val;
+	return 0;
 }
 
 static PyObject *pyrf_evsel__get_attr_sample_period(PyObject *self, void *closure __maybe_unused)
@@ -2352,11 +2387,21 @@ static PyObject *pyrf_evsel__get_attr_sample_period(PyObject *self, void *closur
 static int pyrf_evsel__set_attr_sample_type(PyObject *self, PyObject *val, void *closure __maybe_unused)
 {
 	struct pyrf_evsel *pevsel = (void *)self;
+	unsigned long long new_val;
 
 	CHECK_INITIALIZED_INT(pevsel->evsel, "evsel");
 
-	pevsel->evsel->core.attr.sample_type = PyLong_AsUnsignedLongLong(val);
-	return PyErr_Occurred() ? -1 : 0;
+	if (val == NULL) {
+		PyErr_SetString(PyExc_TypeError, "cannot delete attribute");
+		return -1;
+	}
+
+	new_val = PyLong_AsUnsignedLongLong(val);
+	if (PyErr_Occurred())
+		return -1;
+
+	pevsel->evsel->core.attr.sample_type = new_val;
+	return 0;
 }
 
 static PyObject *pyrf_evsel__get_attr_sample_type(PyObject *self, void *closure __maybe_unused)
@@ -2380,11 +2425,21 @@ static PyObject *pyrf_evsel__get_attr_size(PyObject *self, void *closure __maybe
 static int pyrf_evsel__set_attr_type(PyObject *self, PyObject *val, void *closure __maybe_unused)
 {
 	struct pyrf_evsel *pevsel = (void *)self;
+	unsigned long new_val;
 
 	CHECK_INITIALIZED_INT(pevsel->evsel, "evsel");
 
-	pevsel->evsel->core.attr.type = PyLong_AsUnsignedLong(val);
-	return PyErr_Occurred() ? -1 : 0;
+	if (val == NULL) {
+		PyErr_SetString(PyExc_TypeError, "cannot delete attribute");
+		return -1;
+	}
+
+	new_val = PyLong_AsUnsignedLong(val);
+	if (PyErr_Occurred())
+		return -1;
+
+	pevsel->evsel->core.attr.type = new_val;
+	return 0;
 }
 
 static PyObject *pyrf_evsel__get_attr_type(PyObject *self, void *closure __maybe_unused)
@@ -2399,11 +2454,21 @@ static PyObject *pyrf_evsel__get_attr_type(PyObject *self, void *closure __maybe
 static int pyrf_evsel__set_attr_wakeup_events(PyObject *self, PyObject *val, void *closure __maybe_unused)
 {
 	struct pyrf_evsel *pevsel = (void *)self;
+	unsigned long new_val;
 
 	CHECK_INITIALIZED_INT(pevsel->evsel, "evsel");
 
-	pevsel->evsel->core.attr.wakeup_events = PyLong_AsUnsignedLong(val);
-	return PyErr_Occurred() ? -1 : 0;
+	if (val == NULL) {
+		PyErr_SetString(PyExc_TypeError, "cannot delete attribute");
+		return -1;
+	}
+
+	new_val = PyLong_AsUnsignedLong(val);
+	if (PyErr_Occurred())
+		return -1;
+
+	pevsel->evsel->core.attr.wakeup_events = new_val;
+	return 0;
 }
 
 static PyObject *pyrf_evsel__get_attr_wakeup_events(PyObject *self, void *closure __maybe_unused)
-- 
2.55.0.rc0.786.g65d90a0328-goog

[PATCH v1 11/13] perf python: Zero initialize perf_data in pyrf_data__init

[Ian Rogers]

Replace path clearing with memset so the entire struct is zeroed,
preventing uninitialized fields from causing errors later.

Fixes: 4cd0142f7dec ("perf python: Add wrapper for perf_data file abstraction")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index e277c89b8347..03b4e8809107 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -3607,7 +3607,7 @@ static int pyrf_data__init(struct pyrf_data *pdata, PyObject *args, PyObject *kw
 	if (pdata->data.open)
 		perf_data__close(&pdata->data);
 	free((char *)pdata->data.path);
-	pdata->data.path = NULL;
+	memset(&pdata->data, 0, sizeof(pdata->data));
 
 	if (fd != -1) {
 		struct stat st;
-- 
2.55.0.rc0.786.g65d90a0328-goog

Re: [PATCH v1 11/13] perf python: Zero initialize perf_data in pyrf_data__init

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing explicit inclusion of POSIX standard header `<string.h>` for newly added `memset`.
--

commit 84d9f7832c422ee6951987ed0cdd0d5717fcc7d2
Author: Ian Rogers <irogers@google.com>

perf python: Zero initialize perf_data in pyrf_data__init

This commit replaces clearing the path with memset to zero out the entire
perf_data struct in pyrf_data__init.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index e277c89b83477..03b4e88091074 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -3607,7 +3607,7 @@ static int pyrf_data__init(struct pyrf_data *pdata, PyObject *args, PyObject *kw
>  	if (pdata->data.open)
>  		perf_data__close(&pdata->data);
>  	free((char *)pdata->data.path);
> -	pdata->data.path = NULL;
> +	memset(&pdata->data, 0, sizeof(pdata->data));

[Severity: Medium]
Will this cause a build failure on musl libc systems? It looks like
memset() is added here but <string.h> is not explicitly included in
tools/perf/util/python.c.

Musl libc enforces strict POSIX namespace separation and does not implicitly
include headers. Could we add an explicit #include <string.h> at the top
of the file to ensure the build succeeds in environments using musl?

>  
>  	if (fd != -1) {
>  		struct stat st;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=11

[PATCH v1 12/13] perf python: Add thread uninitialized checks

[Ian Rogers]

Add CHECK_INITIALIZED checks to the thread attribute getters
(get_pid, get_tid, get_ppid) to prevent crashes if they are accessed
before being properly initialized.

Fixes: 3b96bf7af60d ("perf python: Add python session abstraction wrapping perf's session")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 03b4e8809107..70f985ad3dd2 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -3743,16 +3743,19 @@ static PyMethodDef pyrf_thread__methods[] = {
 
 static PyObject *pyrf_thread__get_pid(struct pyrf_thread *pthread, void *closure __maybe_unused)
 {
+	CHECK_INITIALIZED(pthread->thread, "thread");
 	return PyLong_FromLong(thread__pid(pthread->thread));
 }
 
 static PyObject *pyrf_thread__get_tid(struct pyrf_thread *pthread, void *closure __maybe_unused)
 {
+	CHECK_INITIALIZED(pthread->thread, "thread");
 	return PyLong_FromLong(thread__tid(pthread->thread));
 }
 
 static PyObject *pyrf_thread__get_ppid(struct pyrf_thread *pthread, void *closure __maybe_unused)
 {
+	CHECK_INITIALIZED(pthread->thread, "thread");
 	return PyLong_FromLong(thread__ppid(pthread->thread));
 }
 
-- 
2.55.0.rc0.786.g65d90a0328-goog

Re: [PATCH v1 12/13] perf python: Add thread uninitialized checks

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch incompletely fixes the missing initialization checks by omitting `pyrf_thread__get_cpu`, leaving a NULL pointer dereference vulnerability.
- [High] The `perf.pmu` type methods lack initialization guards, allowing uninitialized objects to cause NULL pointer dereferences.
- [Medium] The Python object `count_values` is leaked in multiple error paths within `pyrf_evsel__read`.
--

commit 95a2c7473298664fd72c7086443470ff19559438
Author: Ian Rogers <irogers@google.com>

perf python: Add thread uninitialized checks

The patch adds CHECK_INITIALIZED checks to the thread attribute getters
(get_pid, get_tid, get_ppid) in the perf python module to prevent crashes
when accessing uninitialized thread objects.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 03b4e88091074..70f985ad3dd20 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -3743,16 +3743,19 @@ static PyMethodDef pyrf_thread__methods[] = {
>  
>  static PyObject *pyrf_thread__get_pid(struct pyrf_thread *pthread, void *closure __maybe_unused)
>  {
> +	CHECK_INITIALIZED(pthread->thread, "thread");
>  	return PyLong_FromLong(thread__pid(pthread->thread));
>  }
>  
>  static PyObject *pyrf_thread__get_tid(struct pyrf_thread *pthread, void *closure __maybe_unused)
>  {
> +	CHECK_INITIALIZED(pthread->thread, "thread");
>  	return PyLong_FromLong(thread__tid(pthread->thread));
>  }
>  
>  static PyObject *pyrf_thread__get_ppid(struct pyrf_thread *pthread, void *closure __maybe_unused)
>  {
> +	CHECK_INITIALIZED(pthread->thread, "thread");
>  	return PyLong_FromLong(thread__ppid(pthread->thread));
>  }
>  

[Severity: High]
This isn't a bug introduced by this patch, but pyrf_thread__get_cpu() is
missing a similar check:

tools/perf/util/python.c:pyrf_thread__get_cpu() {
    return PyLong_FromLong(thread__cpu(pthread->thread));
}

Could accessing the .cpu property on an uninitialized thread object cause a
NULL pointer dereference? Does it make sense to add a CHECK_INITIALIZED
guard here as well?

[Severity: High]
This isn't a bug introduced by this patch, but looking at the perf.pmu
methods, they also appear to lack initialization checks:

tools/perf/util/python.c:pyrf_pmu__name() {
    return PyUnicode_FromString(ppmu->pmu->name);
}

tools/perf/util/python.c:pyrf_pmu__events() {
    ...
    ret = perf_pmu__for_each_event(ppmu->pmu,
                                   false,
                                   py_list,
                                   pyrf_pmu__events_cb);
    ...
}

Since perf.pmu uses PyType_GenericNew, could a script instantiate an
uninitialized PMU and trigger a NULL pointer dereference when calling .name()
or .events()?

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed a leak in
pyrf_evsel__read() on its error paths:

tools/perf/util/python.c:pyrf_evsel__read() {
    count_values = PyObject_New(struct pyrf_counts_values,
                                &pyrf_counts_values__type);
    if (!count_values)
        return NULL;

    if (!PyArg_ParseTuple(args, "ii", &cpu, &thread))
        return NULL;

    cpu_idx = perf_cpu_map__idx(evsel->core.cpus, (struct perf_cpu){.cpu = cpu});
    if (cpu_idx < 0) {
        PyErr_Format(PyExc_TypeError, "CPU %d is not part of evsel's CPUs", cpu);
        return NULL;
    }
    ...
}

Does returning NULL directly on argument parsing or bounds checking failures
leak the newly allocated count_values object? Should we call
Py_DECREF(count_values) before returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=12

[PATCH v1 13/13] perf python: Fix MetricGroup return type in perf.pyi

[Ian Rogers]

The metrics() function can return a dictionary where the value is either
a string or a list of strings, so the type signature in the stub file
should be Union[str, List[str]].

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/python/perf.pyi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/python/perf.pyi b/tools/perf/python/perf.pyi
index f266c9baffd0..58b0b3ed819d 100644
--- a/tools/perf/python/perf.pyi
+++ b/tools/perf/python/perf.pyi
@@ -1,5 +1,5 @@
 """Type stubs for the perf Python module."""
-from typing import Callable, Dict, List, Optional, Any, Iterator
+from typing import Callable, Dict, List, Optional, Any, Iterator, Union
 
 def config_get(name: str) -> Optional[str]:
     """Get a configuration value from perf config.
@@ -12,7 +12,7 @@ def config_get(name: str) -> Optional[str]:
     """
     ...
 
-def metrics() -> List[Dict[str, str]]:
+def metrics() -> List[Dict[str, Union[str, List[str]]]]:
     """Get a list of available metrics.
 
     Returns:
-- 
2.55.0.rc0.786.g65d90a0328-goog

Re: [PATCH v1 13/13] perf python: Fix MetricGroup return type in perf.pyi

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but local references to the Python objects 'key' and 'value' in `pyrf__metrics_cb` are leaked on the success path.
--

commit 0b60b233b211850d4730f46ab4faf2da85366d5f
Author: Ian Rogers <irogers@google.com>

perf python: Fix MetricGroup return type in perf.pyi

The metrics() function can return a dictionary where the value is either
a string or a list of strings, so the type signature in the stub file
should be Union[str, List[str]].

> diff --git a/tools/perf/python/perf.pyi b/tools/perf/python/perf.pyi
> index f266c9baffd01..58b0b3ed819d8 100644
> --- a/tools/perf/python/perf.pyi
> +++ b/tools/perf/python/perf.pyi
[ ... ]
> @@ -12,7 +12,7 @@ def config_get(name: str) -> Optional[str]:
>      """
>      ...
>  
> -def metrics() -> List[Dict[str, str]]:
> +def metrics() -> List[Dict[str, Union[str, List[str]]]]:

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the return
type update for metrics(), I noticed a potential resource leak in its C
implementation.

Looking at tools/perf/util/python.c:pyrf__metrics_cb():

	if (!value || PyDict_SetItem(dict, key, value) != 0) {
		Py_XDECREF(key);
		Py_XDECREF(value);
		Py_XDECREF(dict);
		return -ENOMEM;
	}

	if (!add_to_dict(dict, "MetricName", pm->metric_name) ||

Does this leak the local references to the Python objects 'key' and 'value'
on the success path?

Since PyDict_SetItem() does not steal references, it appears that 'key' and
'value' might need a Py_DECREF() after a successful insertion. Without it,
a script repeatedly calling perf.metrics() could continually leak memory.

>      """Get a list of available metrics.
>  
>      Returns:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623053515.2677888-1-irogers@google.com?part=13