Re: [PATCH v1 12/58] perf evlist: Add reference count checking

sashiko-bot
2026-04-20T00:54:58Z

Sashiko AI review found 4 potential issue(s):
- [High] Missing NULL checks on the return values of `from_list_start()` and `evlist__get()` lead to NULL pointer dereferences on OOM.
- [High] Dangling back-pointers causing Use-After-Free (UAF) due to independent wrapper lifetimes in `REFCNT_CHECKING` mode.
- [Medium] Memory leak of the core `evlist` structure in `evlist__new()` if the RC wrapper allocation fails.
- [Low] Incorrect use of `TEST_ASSERT_VAL` macro in a boolean-returning test function masks test failures.
--

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

perf evlist: Add reference count checking

This commit adds reference count checking to the perf evlist structure to
ensure gets and puts are paired correctly. It introduces new accessors for
evlist functions and updates the subsystem to use these checked variants.

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
[ ... ]
> struct evlist *evlist__new(void)
> {
> - struct evlist *evlist = zalloc(sizeof(*evlist));
> -
> - if (evlist != NULL)
> - evlist__init(evlist, NULL, NULL);
> + struct evlist *result;
> + RC_STRUCT(evlist) *evlist;
>
> - return evlist;
> + evlist = zalloc(sizeof(*evlist));
> + if (ADD_RC_CHK(result, evlist)) {
> + perf_evlist__init(evlist__core(result));

If the ADD_RC_CHK allocation fails internally, it returns a NULL pointer for
the wrapper. Does this code leak the original evlist allocated via zalloc
since the function will return without freeing it?

[ ... ]
> +static struct evlist *from_list_start(struct perf_evlist *core)
> +{
> +#ifdef REFCNT_CHECKING
> + RC_STRUCT(evlist) *core_evlist = container_of(core, RC_STRUCT(evlist), core);
> + struct evlist *evlist;
> +
> + if (ADD_RC_CHK(evlist, core_evlist))
> + refcount_inc(evlist__refcnt(evlist));
> +
> + return evlist;
> +#else
> + return container_of(core, struct evlist, core);
> +#endif
> +}
[ ... ]
> static struct perf_mmap*
> perf_evlist__mmap_cb_get(struct perf_evlist *_evlist, bool overwrite, int idx)
> {
> - struct evlist *evlist = container_of(_evlist, struct evlist, core);
> + struct evlist *evlist = from_list_start(_evlist);
> struct mmap *maps;
>
> - maps = overwrite ? evlist->overwrite_mmap : evlist->mmap;
> + maps = overwrite ? evlist__overwrite_mmap(evlist) : evlist__mmap(evlist);

Since from_list_start can return a NULL pointer under memory pressure when
REFCNT_CHECKING is enabled, could this lead to a NULL pointer dereference
when evlist__overwrite_mmap subsequently attempts to access the pointer?

Other functions like evlist__get also appear to return NULL on allocation
failure without subsequent checks by callers.

[ ... ]
> void evlist__add(struct evlist *evlist, struct evsel *entry)
> {
> - perf_evlist__add(&evlist->core, &entry->core);
> + perf_evlist__add(evlist__core(evlist), &entry->core);
> entry->evlist = evlist;
> entry->tracking = !entry->core.idx;

With the new reference count checking, evlist__get creates a new wrapper and
evlist__put frees it via RC_CHK_PUT, even if the underlying shared object
is still active.

If entry->evlist stores the specific wrapper pointer passed to evlist__add,
and that wrapper is later freed while the underlying list remains active,
could accessing entry->evlist elsewhere trigger a use-after-free regression?

> diff --git a/tools/perf/tests/time-utils-test.c b/tools/perf/tests/time-utils-test.c
> --- a/tools/perf/tests/time-utils-test.c
> +++ b/tools/perf/tests/time-utils-test.c
[ ... ]
> static bool test__perf_time__parse_for_ranges(struct test_data *d)
> {
> - struct evlist evlist = {
> - .first_sample_time = d->first,
> - .last_sample_time = d->last,
> - };
> - struct perf_session session = { .evlist = &evlist };
> + struct evlist *evlist = evlist__new();
> + struct perf_session session = { .evlist = evlist };
> struct perf_time_interval *ptime = NULL;
> int range_size, range_num;
> bool pass = false;
> int i, err;
>
> + TEST_ASSERT_VAL("Missing evlist", evlist);

TEST_ASSERT_VAL returns TEST_FAIL (-1) if the condition is false. Since
this function returns a boolean, -1 evaluates to true. Does this regression
inadvertently mask the error and cause a failing test to report as passing?

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