Re: [PATCH v10 1/9] perf cs-etm: Fix thread leaks on trace queue init failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Crash via NULL pointer dereference in `cs_etm__etmq_get_traceid_queue()` when `intlist__findnew()` fails.
- [Medium] Memory and thread reference leak in `cs_etm__etmq_get_traceid_queue()` when `reallocarray()` fails.
--

commit 81959854a0401183b992f3f15f791657d4e73afb
Author: Leo Yan <leo.yan@arm.com>

perf cs-etm: Fix thread leaks on trace queue init failure

cs_etm__init_traceid_queue() allocates the frontend and decode threads.
If a later allocation fails, the error path does not drop the thread
references that were already acquired. This patch releases both thread
pointers on the error path to avoid leaking thread references.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 0927b0b9c06b1..d484a6155c2c2 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -661,6 +663,8 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
>  	zfree(&tidq->prev_packet);
>  	zfree(&tidq->packet);
>  out:
> +	thread__zput(tidq->frontend_thread);
> +	thread__zput(tidq->decode_thread);
>  	return rc;
>  }

[Severity: Medium]
This isn't a regression introduced by this patch, but while looking at the
thread and memory leaks fixed here, I noticed a pre-existing regression in
the caller cs_etm__etmq_get_traceid_queue() which calls
cs_etm__init_traceid_queue():

tools/perf/util/cs-etm.c:cs_etm__etmq_get_traceid_queue() {
...
	if (cs_etm__init_traceid_queue(etmq, tidq, trace_chan_id))
		goto out_free;

	/* Grow the traceid_queues array by one unit */
	traceid_queues = etmq->traceid_queues;
	traceid_queues = reallocarray(traceid_queues,
				      idx + 1,
				      sizeof(*traceid_queues));

	if (!traceid_queues)
		goto out_free;
...
out_free:
	intlist__remove(traceid_queues_list, inode);
	free(tidq);

	return NULL;
}

If reallocarray() fails, the code jumps to out_free. Does this leak the
inner allocations and thread references of tidq? 

It appears free(tidq) is called, but the nested allocations (like
tidq->packet, tidq->last_branch) and the thread references acquired in
cs_etm__init_traceid_queue() are never cleaned up here.


[Severity: Medium]
This is another pre-existing regression in the same function. Looking
slightly higher up in cs_etm__etmq_get_traceid_queue():

tools/perf/util/cs-etm.c:cs_etm__etmq_get_traceid_queue() {
...
	inode = intlist__findnew(traceid_queues_list, trace_chan_id);
	if (!inode)
		goto out_free;
...
out_free:
	intlist__remove(traceid_queues_list, inode);
	free(tidq);

	return NULL;
}

If intlist__findnew() fails, inode will be NULL. Could calling
intlist__remove() with a NULL inode cause a crash? 

It looks like intlist__remove() unconditionally dereferences the node passed
to it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-b4-arm_cs_callchain_support_v1-v10-0-e8b6e5d63db5@arm.com?part=1