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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: hardcoded system_config=false wipes out system config file
- [Low] perf: missing limits.h inclusion for PATH_MAX causes musl build failure
--

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

    perf config: Move perf_config__set_variable() to util/config.c
    
    It is code from the 'perf config' builtin to set a variable in the
    configuration file perf is using, the user's ~/.perfconfig, or the file
    named by the PERF_CONFIG environment variable when it is set, rewriting
    that file from the config set, the same rewrite 'perf config' does,
    i.e. the comments in it are not preserved, as the config set carries
    just the key-value pairs.  It has to honour PERF_CONFIG, that makes perf
    read only that file: the config set is loaded from it, so writing
    ~/.perfconfig would replace it with the exclusive file's entries.

> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 31c6618d3b3da..3f9753b2a7bdf 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c

[ ... ]

> @@ -876,6 +876,76 @@ void perf_config__exit(void)
>  	config_set = NULL;
>  }
>  
> +int perf_config_set__write(struct perf_config_set *set,
> +			   const char *file_name, bool system_config)
> +{
> +	struct perf_config_section *section = NULL;
> +	struct perf_config_item *item = NULL;
> +	FILE *fp;
> +
> +	fp = fopen(file_name, "w");
> +	if (!fp)
> +		return -1;
> +
> +	fprintf(fp, "# this file is auto-generated.\n");
> +
> +	/* overwrite configvariables */
> +	perf_config_sections__for_each_entry(&set->sections, section) {
> +		if (!system_config && section->from_system_config)
> +			continue;
> +		fprintf(fp, "[%s]\n", section->name);
> +
> +		perf_config_items__for_each_entry(&section->items, item) {
> +			if (!system_config && item->from_system_config)
> +				continue;
> +			if (item->value)
> +				fprintf(fp, "\t%s = %s\n",
> +					item->name, item->value);
> +		}
> +	}
> +	fclose(fp);
> +
> +	return 0;
> +}
> +
> +/*
> + * Set @var=@value in the configuration file perf is using: the user's
> + * ~/.perfconfig, or the file named by the PERF_CONFIG environment
> + * variable.  The latter has to be honoured as it makes perf read only
> + * that file, so the config set came from it and writing ~/.perfconfig
> + * would replace it with the exclusive file's entries.  Rewriting the
> + * file from the config set is the same rewrite 'perf config' does: the
> + * comments are not preserved, as the config set carries just the
> + * key-value pairs.
> + */
> +int perf_config__set_variable(const char *var, const char *value)
> +{
> +	char path[PATH_MAX];

[Severity: Low]
Does this file need an explicit #include <limits.h>? The function uses the
PATH_MAX macro, and while glibc may implicitly include it through other
headers, strict musl compilation might fail without the required
POSIX-specified header.

> +	char *user_config = mkpath(path, sizeof(path), "%s/.perfconfig", getenv("HOME"));
> +	const char *config_filename = config_exclusive_filename ?: user_config;
> +	struct perf_config_set *set;
> +	int ret = -1;
> +
> +	set = perf_config_set__new();
> +	if (!set)
> +		goto out_err;
> +
> +	if (perf_config_set__collect(set, config_filename, var, value) < 0) {
> +		pr_err("Failed to add '%s=%s'\n", var, value);
> +		goto out_err;
> +	}
> +
> +	if (perf_config_set__write(set, config_filename, /*system_config=*/false) < 0) {

[Severity: High]
Could hardcoding system_config to false here risk wiping out the system
configuration file?

If a user sets PERF_CONFIG=/etc/perfconfig (setting config_exclusive_filename),
this call to perf_config__set_variable() will read that file. Because the file
path matches perf_etc_perfconfig(), all loaded items are flagged with
from_system_config = true.

When perf_config_set__write() is then called with system_config=false, it
checks:

tools/perf/util/config.c:perf_config_set__write() {
...
		if (!system_config && section->from_system_config)
			continue;
...
}

This actively filters out items that originated from the system config, which
could effectively truncate the file and cause data loss for the system-wide
configuration.

> +		pr_err("Failed to set the configs on %s\n", config_filename);
> +		goto out_err;
> +	}
> +
> +	ret = 0;
> +out_err:
> +	perf_config_set__delete(set);
> +	return ret;
> +}

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