Re: [PATCH v7 12/59] perf evlist: Add reference count checking

sashiko-bot
2026-04-25T23:28:34Z

Sashiko AI review found 3 potential issue(s):
- [High] TOCTOU race in `evlist__put()` cyclic reference detection causes a double free.
- [High] Missing `evsel__put()` on `sample->evsel` causes massive memory leaks for synthesized and deferred events.
- [Low] Missing NULL pointer check after `evsel__new()` allocation in sample parsing test.
--

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

perf evlist: Add reference count checking

Now the evlist is reference counted, add reference count checking so
that gets and puts are paired and easy to debug.

> diff --git a/tools/perf/tests/sample-parsing.c b/tools/perf/tests/sample-parsing.c
> index 55f0b73ca20e..6db717e562d5 100644
> --- a/tools/perf/tests/sample-parsing.c
> +++ b/tools/perf/tests/sample-parsing.c
[ ... ]
> @@ -287,16 +283,17 @@ static int do_test(u64 sample_type, u64 sample_regs, u64 read_format)
> size_t i, sz, bufsz;
> int err, ret = -1;
>
> + evsel = evsel__new(&attr);
> perf_sample__init(&sample_out, /*all=*/false);
> perf_sample__init(&sample_out_endian, /*all=*/false);
> if (sample_type & PERF_SAMPLE_REGS_USER)
> - evsel.core.attr.sample_regs_user = sample_regs;
> + evsel->core.attr.sample_regs_user = sample_regs;

Could this dereference a NULL pointer if evsel__new() fails?

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index a362f338f104..29588af735e5 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
[ ... ]
> @@ -220,13 +227,33 @@ static void evlist__exit(struct evlist *evlist)
>
> void evlist__put(struct evlist *evlist)
> {
> + struct evsel *evsel;
> + unsigned int count;
> +
> if (evlist == NULL)
> return;
>
> - if (!refcount_dec_and_test(&evlist->refcnt))
> - return;
> + if (refcount_dec_and_test(evlist__refcnt(evlist)))
> + goto out_delete;
>
> + count = refcount_read(evlist__refcnt(evlist));
> + evlist__for_each_entry(evlist, evsel) {
> + if (RC_CHK_EQUAL(evsel->evlist, evlist) && count)
> + count--;
> + }
> + if (count != 0) {
> + /*
> + * Not the last reference except for back references from
> + * evsels.
> + */
> + RC_CHK_PUT(evlist);
> + return;
> + }
> +out_delete:

Is there a race condition here if two threads concurrently call evlist__put()
and drop the final two external references?

If they both execute refcount_dec_and_test() and it evaluates to false, both
threads could read the updated refcount that matches the back-references, and
both would jump to out_delete, causing a double free.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index deb5b9dfe44c..f9fafbb80a9d 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
[ ... ]
> @@ -1464,22 +1470,24 @@ static int machines__deliver_event(struct machines *machines,
>
> dump_event(evlist, event, file_offset, sample, file_path);
>
> - if (!sample->evsel)
> + if (!sample->evsel) {
> sample->evsel = evlist__id2evsel(evlist, sample->id);
> - else
> + if (sample->evsel)
> + sample->evsel = evsel__get(sample->evsel);
> + } else {
> assert(sample->evsel == evlist__id2evsel(evlist, sample->id));

Will this leak the evsel reference?

We acquire a strong reference to the evsel here using evsel__get(), but it
doesn't appear that perf_sample__exit() or other paths in the lifecycle of
the sample object are updated to release it via evsel__put().

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