Re: [PATCH v8 15/17] perf env: Add helper to lazily compute the os_release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incomplete refactoring: The JSON data converter was missed and does not use the new lazy initialization helper.
- [Medium] Context leak: Assuming uninitialized `os_release` always implies live mode incorrectly pollutes offline trace conversions with the host's kernel version.
--

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

    perf env: Add helper to lazily compute the os_release

This commit introduces a helper function, perf_env__os_release(), to lazily
initialize the OS release string when it is not populated. It updates the CTF
data converter and other sites to use this helper. The helper assumes that if
os_release isn't initialized, perf is running in live mode and uses uname()
as a fallback.

> diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
> index 3b8f2df823a9..2c88420fe33e 100644
> --- a/tools/perf/util/data-convert-bt.c
> +++ b/tools/perf/util/data-convert-bt.c
> @@ -1414,7 +1414,7 @@ do {									\
>  
>  	ADD("host",    env->hostname);
>  	ADD("sysname", "Linux");
> -	ADD("release", env->os_release);
> +	ADD("release", perf_env__os_release(env));

Was the JSON data converter missed in this refactoring?

The CTF converter was updated to use the new helper, but the JSON converter
in tools/perf/util/data-convert-json.c still accesses the field directly:

tools/perf/util/data-convert-json.c:
    ...
    output_json_key_string(out, true, 2, "hostname", env->hostname);
    output_json_key_string(out, true, 2, "os-release", env->os_release);
    output_json_key_string(out, true, 2, "arch", env->arch);
    ...

This leads to inconsistent behavior where the JSON converter outputs an empty
string or null if os_release is uninitialized, while the CTF converter
outputs the host's release string.

>  	ADD("version", env->version);
>  	ADD("machine", env->arch);
>  	ADD("domain", "kernel");

[ ... ]

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 5944acd28996..1090aaa2985f 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -339,6 +339,27 @@ int perf_env__kernel_is_64_bit(struct perf_env *env)
>  	return env->kernel_is_64_bit;
>  }
>  
> +const char *perf_env__os_release(struct perf_env *env)
> +{
> +	struct utsname uts;
> +	int ret;
> +
> +	if (!env)
> +		return perf_version_string;
> +
> +	if (env->os_release)
> +		return env->os_release;
> +
> +	/*
> +	 * The os_release is being accessed but wasn't initialized from a data
> +	 * file, assume this is 'live' mode and use the release from uname. If
> +	 * uname or strdup fails then use the current perf tool version.
> +	 */
> +	ret = uname(&uts);
> +	env->os_release = strdup(ret < 0 ? perf_version_string : uts.release);
> +	return env->os_release ?: perf_version_string;
> +}

Does this fallback to uname() incorrectly pollute offline trace conversions
with the host's kernel version?

If a perf.data file was recorded without the HEADER_OSRELEASE feature,
env->os_release will be NULL.

When running perf data convert --to-ctf on such a file, this helper will
execute uname() and write the host machine's OS release into the trace
environment.

Prior to this patch, a NULL os_release was safely omitted from the CTF
export.

Injecting the converter's host kernel version into offline trace metadata
falsifies the environment in which the trace was originally recorded.

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