Re: [PATCH 1/5] perf config: Move perf_config__set_variable() to util/config.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: data race in perf_config__set_variable() on static path buffer
--

commit 6edfbfca7e25b1f211f3d43320a648de89590467
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf config: Move perf_config__set_variable() to util/config.c

This commit moves perf_config__set_variable() out of the perf config builtin
so that opt-in features can persist their choice from outside it. It also
adds mutexes to serialize parsing and rewriting since config file access can
now run on more than one thread.

> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
[ ... ]
> +int perf_config__set_variable(const char *var, const char *value)
> +{
> +	/*
> +	 * Not on the stack: the parser publishes this buffer as
> +	 * config_file_name, which another thread may still be reading.
> +	 */
> +	static char path[PATH_MAX];
> +	char *user_config = mkpath(path, sizeof(path), "%s/.perfconfig", getenv("HOME"));

[Severity: Medium]
Could the use of a static buffer here cause a data race if two threads
concurrently call perf_config__set_variable()?

Since mkpath() uses vsnprintf() to format the file path directly into the
shared static path array concurrently, couldn't this corrupt the resulting
file path and cause perf to read or write its configuration to an invalid
or garbage location?

> +	const char *config_filename = config_exclusive_filename ?: user_config;
> +		/*
> +	 * When rewriting the system wide file all entries are marked as coming
> +	 * from it and must be kept, or it would be truncated down to its
> +	 * header.
> +	 */
> +	bool system_config = strcmp(config_filename, perf_etc_perfconfig()) == 0;
> +	struct perf_config_set *set;
> +	int ret = -1;
> +
> +	pthread_mutex_lock(&config_update_mutex);

[Severity: Medium]
The lock intended to serialize the read-modify-write operation is acquired
here, but the static buffer was already mutated in the mkpath() call above.
Does this leave the static path array unprotected against the concurrent
race?

> +	set = perf_config_set__new();
> +	if (!set)
> +		goto out_err;

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