Re: [PATCH 2/2] perf: do not call list_for_each_entry_safe() on uninitalized list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf tests: uninitialized err list in test__pmu_config_helpers()
--

commit 779f6d6b7e775ad26417497420edb0cda7bafa6c
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>

perf: do not call list_for_each_entry_safe() on uninitalized list

This commit adds an err_out_pmu cleanup label in test__pmu_config_helpers()
so that an early evlist__new() allocation failure skips calling
parse_events_terms__exit(&terms) before parse_events_terms__init(&terms) has
run.

> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index 13e8d7fa80af2..be984e1385df2 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -223,7 +223,7 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
>  	evlist = evlist__new();
>  	if (evlist == NULL) {
>  		pr_err("Failed allocation");
> -		goto err_out;
> +		goto err_out_pmu;
>  	}
>  
>  	parse_events_terms__init(&terms);
> @@ -295,6 +295,7 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
>  err_out:
>  	parse_events_terms__exit(&terms);
>  	evlist__put(evlist);
> +err_out_pmu:
>  	test_pmu_put(dir, pmu);
>  	return ret;
>  }

[Severity: Medium]
Should test__pmu_config_helpers() in tools/perf/tests/pmu.c be initializing
and cleaning up err instead of terms?

Looking at test__pmu_config_helpers(), both terms and config_terms appear to
be unused remnants, whereas err is passed to parse_events(),
parse_events_error__print(), and parse_events_error__contains() without
parse_events_error__init(&err) or parse_events_error__exit(&err) ever being
called:

	struct parse_events_terms terms;
	struct parse_events_error err;
	LIST_HEAD(config_terms);
	...
	parse_events_terms__init(&terms);
	ret = parse_events(evlist, event, &err);
	if (ret) {
		pr_debug("failed to parse event '%s', err %d\n", event, ret);
		parse_events_error__print(&err, event);
		if (parse_events_error__contains(&err, "can't access trace events"))
			ret = TEST_SKIP;
		goto err_out;
	}
	...
err_out:
	parse_events_terms__exit(&terms);
	evlist__put(evlist);

When parse_events() fails and records an error via
parse_events_error__handle(), list_add(&entry->list, &err->list) and the
subsequent list_for_each_entry() walks in parse_events_error__print() and
parse_events_error__contains() will dereference the uninitialized err.list
pointers on the stack, and any allocated parse_events_error_entry objects
will be leaked.

Instead of keeping the unused terms variable, should terms and config_terms
be removed and parse_events_terms__init(&terms) /
parse_events_terms__exit(&terms) be replaced with
parse_events_error__init(&err) and parse_events_error__exit(&err)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104100802.539266-1-jirislaby@kernel.org?part=2