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

[sashiko-bot@kernel.org]

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