Re: [PATCH 2/2] perf data convert ctf: Initial babeltrace2 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Dead code in `ctf_writer__init` masks initialization errors and causes a NULL pointer dereference.
- [High] Broken `CLEANUP` macro in `ctf_writer__cleanup_data` leaks Babeltrace data types and causes double-frees.
- [High] Queue state corruption in `queue_push` silently drops all queued messages if flushing fails.
- [High] Infinite loop (hang) occurs in `ctf_writer__flush_streams` if the Babeltrace2 graph fails.
- [Medium] Missing feature test source file makes Babeltrace 2 support impossible to enable.
- [Medium] Unchecked allocation and memory leak of the message queue array `c.queue`.
- [Medium] Redundant double-cleanup in `bt_convert__perf2ctf` when Babeltrace 2 is disabled.
--

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

perf data convert ctf: Initial babeltrace2 support

This commit adds experimental support for libbabeltrace2 to the perf data
CTF conversion tool. It introduces a message iterator, graph, and component
classes to replace the deprecated libbabeltrace 1 stream writer APIs.

> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index 333ddd0e4bd81..3e3c4f240ee8f 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
[ ... ]
> @@ -1067,7 +1069,16 @@ ifndef NO_LIBBABELTRACE
>      EXTLIBS += -lbabeltrace-ctf
>      $(call detected,CONFIG_LIBBABELTRACE)
>    else
> -    $(warning No libbabeltrace found, disables 'perf data' CTF format support, please install libbabeltrace-dev[el]/libbabeltrace-ctf-dev)
> +    $(call feature_check,libbabeltrace2)

Is there a missing feature test source file for libbabeltrace2?
The build system's feature_check relies on compiling a matching test file in
tools/build/feature/test-libbabeltrace2.c. Because this test file is
missing, the compilation test will permanently fail, preventing
HAVE_LIBBABELTRACE2_SUPPORT from being enabled.

[ ... ]
> diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
> index 3b8f2df823a9b..b16544f8b9b9a 100644
> --- a/tools/perf/util/data-convert-bt.c
> +++ b/tools/perf/util/data-convert-bt.c
[ ... ]
> +static void queue_push(struct convert *c, bt_message *msg)
> +{
> +	unsigned int next_head = (c->queue_head + 1) & c->queue_mask;
> +
> +	while (next_head == c->queue_tail) {
> +		/* Queue is full, drive the graph to consume */
> +		if (ctf_writer__flush_queue(c) < 0)
> +			break;
> +		next_head = (c->queue_head + 1) & c->queue_mask;
> +	}
> +
> +	c->queue[c->queue_head] = msg;
> +	c->queue_head = next_head;

Does this code corrupt the queue state if flushing fails?
If ctf_writer__flush_queue() returns < 0, the loop breaks and assigns
next_head to c->queue_head. Since next_head still equals c->queue_tail,
the head and tail pointers become identical. This effectively marks the
ring buffer as empty, silently dropping all queued messages.

[ ... ]
>  static void ctf_writer__cleanup_data(struct ctf_writer *cw)
>  {
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(cw->data.array); i++)
> -		bt_ctf_field_type_put(cw->data.array[i]);
> +#ifdef HAVE_LIBBABELTRACE2_SUPPORT
> +#define CLEANUP(type)					\
> +	if (cw->data.type) {				\
> +		bt_field_class_put_ref(cw->data.s64);	\
> +		cw->data.s64 = NULL;			\
> +	}

Could this macro cause memory leaks and double-frees?
It hardcodes cw->data.s64 instead of operating on the provided type
parameter. When expanded for subsequent types (e.g., u64, s32), it checks
their presence but exclusively frees and nullifies s64, leaving the other
field classes leaked and potentially double-freeing s64.

[ ... ]
>  static int ctf_writer__init(struct ctf_writer *cw, const char *path,
> -			    struct perf_session *session, bool tod)
> +			    struct perf_session *session __maybe_unused,
> +			    bool tod __maybe_unused, struct convert *c __maybe_unused)
>  {
> +#ifdef HAVE_LIBBABELTRACE2_SUPPORT
[ ... ]
> +	err = setup_streams(cw, session);
> +err:
> +	if (sink_comp_class)
> +		bt_component_class_sink_put_ref(sink_comp_class);
> +	if (src_comp_class)
> +		bt_component_class_source_put_ref(src_comp_class);
> +	if (iter_class)
> +		bt_message_iterator_class_put_ref(iter_class);
> +	return 0;
> +
> +	ctf_writer__cleanup(cw);
> +	if (err)
> +		pr_err("Failed to setup CTF writer.\n");
> +	return err;

Does the unconditional return 0 here mask initialization errors?
It leaves the actual error reporting and cleanup logic unreachable.
If graph creation or component initialization fails, the function will falsely
report success, which could lead to a NULL pointer dereference later when
bt_graph_run_once() is called on the uninitialized graph.

[ ... ]
> -static int ctf_writer__flush_streams(struct ctf_writer *cw)
> +static int ctf_writer__flush_streams(struct convert *c)
>  {
[ ... ]
> +#ifdef HAVE_LIBBABELTRACE2_SUPPORT
> +	while (c->queue_head != c->queue_tail)
> +		ctf_writer__flush_queue(c);

Could this result in an infinite loop?
If ctf_writer__flush_queue() encounters an error and returns -1, it does
not advance the queue pointers. Since the return value is ignored, the loop
will spin indefinitely without consuming any messages.

[ ... ]
>  int bt_convert__perf2ctf(const char *input, const char *path,
>  			 struct perf_data_convert_opts *opts)
>  {
[ ... ]
> -	struct convert c = {};
> -	struct ctf_writer *cw = &c.writer;
> -	int err;
> +	struct convert c = {0};
> +	int err = -1;
> +
> +	dump_trace = 1;
> +	debug_data_convert = 3;
> +	/* Setup queue */
> +	c.queue_size = QUEUE_SIZE;
> +#ifdef HAVE_LIBBABELTRACE2_SUPPORT
> +	c.queue = zalloc(sizeof(bt_message *) * c.queue_size);
> +	c.queue_mask = c.queue_size - 1;
> +#endif

Is there a missing allocation check and free for c.queue?
The dynamically allocated c.queue is never checked for failure, which would
cause a NULL pointer dereference in queue_push() under memory pressure.
Additionally, there appears to be no corresponding free() in the cleanup
path, leading to a memory leak.

[ ... ]
>  	cleanup_events(session);
>  	perf_session__delete(session);
> -	ctf_writer__cleanup(cw);
> -
> -	return err;
> -
> -free_writer:
> -	ctf_writer__cleanup(cw);
> -free_session:
> -	if (c.ptime_range)
> -		zfree(&c.ptime_range);
> -
> -	perf_session__delete(session);
> -	pr_err("Error during conversion setup.\n");
> +	ctf_writer__cleanup(&c.writer);
> +#ifndef HAVE_LIBBABELTRACE2_SUPPORT
> +	ctf_writer__cleanup(&c.writer);
> +#endif
>  	return err;
>  }

Is the double-cleanup intentional when Babeltrace 2 is disabled?
ctf_writer__cleanup() is called unconditionally, and then a second time inside
the #ifndef block. While the first call zeroes the structure, feeding a
zeroed state back into the cleanup function might be unsafe for older
libbabeltrace1 APIs.

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