Re: [PATCH v11 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:
- [High] This problem wasn't introduced by this patch, but a NULL pointer dereference occurs in the error path if `intlist__findnew()` fails.
- [High] This problem wasn't introduced by this patch, but the `reallocarray` error path in `cs_etm__etmq_get_traceid_queue` leaks memory and thread references.
--

commit 7d838026b9aad27b43e259b084c832b06d7fc96a
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 thread
reference that was already acquired.

Release both thread pointers with thread__zput() on the error path, so
does not leak thread references or leave stale pointers behind.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 2284cda78abe1..deca07d57282e 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -645,6 +645,8 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
>  					       queue->tid);
>  	tidq->decode_thread = machine__findnew_thread(&etm->session->machines.host, -1,
>  					       queue->tid);
> +	if (!tidq->frontend_thread || !tidq->decode_thread)
> +		goto out;
>  
>  	tidq->packet = zalloc(sizeof(struct cs_etm_packet));
>  	if (!tidq->packet)
[ ... ]
> @@ -679,6 +681,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: High]
This isn't a bug introduced by this patch, but there appears to be a
pre-existing NULL pointer dereference in the error path of the caller
cs_etm__etmq_get_traceid_queue() in tools/perf/util/cs-etm.c.

If intlist__findnew() fails and returns NULL:

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;
}

Can this crash? Passing a NULL inode to intlist__remove() eventually
leads to rblist__remove_node(), which will try to dereference the pointer.

[Severity: High]
This is also a pre-existing issue, but does this same error path leak the
internal resources of tidq?

If reallocarray() fails after cs_etm__init_traceid_queue() succeeds:

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;

	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;
}

Since cs_etm__init_traceid_queue() successfully allocated dynamic fields
like packet and prev_packet, and acquired thread references that are stored
in tidq, wouldn't free(tidq) leak these allocations and references?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-b4-arm_cs_callchain_support_v1-v11-0-846abbf600d7@arm.com?part=1