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

[sashiko-bot@kernel.org]

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