Re: [PATCH v6 1/3] perf env: Add perf_env__e_machine helper and use in perf_env__arch

sashiko-bot
2026-04-09T23:37:32Z

Sashiko AI review found 4 potential issue(s):
- [High] perf_session__e_machine() bypasses lazy evaluation of e_machine from env->arch when env->e_machine is EM_NONE, breaking cross-architecture analysis of legacy perf.data files.
- [High] Uninitialized env->kernel_is_64_bit coerces to true, causing 32-bit architectures to be parsed as 64-bit.
- [High] write_arch() silently caches EM_HOST, permanently bypassing accurate thread-based architecture detection during perf record.
- [Medium] Unhandled OOM in process_arch() leaves env->arch as NULL, leading to a subsequent NULL pointer dereference.
--

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

perf env: Add perf_env__e_machine helper and use in perf_env__arch

Add a helper that lazily computes the e_machine and falls back of
EM_HOST. Use the perf_env's arch to compute the e_machine if
available. Use a binary search for some efficiency in this, but handle
somewhat complex duplicate rules.

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 1e54e2c863605..339d62ca37bb8 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
[ ... ]
> @@ -588,51 +590,160 @@ void cpu_cache_level__free(struct cpu_cache_level *cache)
[ ... ]
> +uint16_t perf_env__e_machine(struct perf_env *env, uint32_t *e_flags)
> +{
> + if (!env) {
> + if (e_flags)
> + *e_flags = EF_HOST;
> +
> + return EM_HOST;
> + }
> + if (env->e_machine == EM_NONE) {
> + env->e_machine = perf_arch_to_e_machine(env->arch, env->kernel_is_64_bit);

Is env->kernel_is_64_bit guaranteed to be a boolean here?

If env->kernel_is_64_bit is uninitialized, it defaults to -1 in
perf_env__init(). In C, -1 evaluates as true, which would cause 32-bit
architectures to be parsed as 64-bit (e.g., returning EM_X86_64 instead
of EM_386).

Should this use the lazy evaluator perf_env__kernel_is_64_bit(env) instead?

[ ... ]
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index c6efddb70aee2..9bb4a271b4f8a 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -370,21 +370,25 @@ static int write_osrelease(struct feat_fd *ff,
[ ... ]
> -static int write_arch(struct feat_fd *ff,
> - struct evlist *evlist __maybe_unused)
> +static int write_arch(struct feat_fd *ff, struct evlist *evlist)
> {
> struct utsname uts;
> - int ret;
> + const char *arch = NULL;
>
> - ret = uname(&uts);
> - if (ret < 0)
> - return -1;
> + if (evlist->session)
> + arch = perf_env__arch(perf_session__env(evlist->session));

When write_arch() is called during perf record, it invokes perf_env__arch().
Since env->arch is NULL and env->e_machine is EM_NONE, this falls back to
perf_arch_to_e_machine(NULL, ...), locking env->e_machine to EM_HOST.

Later, when write_e_machine() calls perf_session__e_machine(), the check for
env->e_machine != EM_NONE will evaluate to true.

Will this permanently bypass the accurate thread-based architecture detection
(machines__for_each_thread) designed to handle mixed 32/64-bit threads?

[ ... ]
> @@ -2675,10 +2679,30 @@ static int process_##__feat(struct feat_fd *ff, void *data __maybe_unused) \
[ ... ]
> +static int process_arch(struct feat_fd *ff, void *data __maybe_unused)
> +{
> + uint16_t saved_e_machine = ff->ph->env.e_machine;
> +
> + free(ff->ph->env.arch);
> + ff->ph->env.arch = do_read_string(ff);
> + if (!ff->ph->env.arch)
> + return -ENOMEM;
> + /*
> + * Make the arch string canonical by computing the e_machine from it,
> + * then turning the e_machine back into an arch string.
> + */
> + ff->ph->env.e_machine = EM_NONE;
> + if (perf_env__e_machine(&ff->ph->env, /*e_flags=*/NULL) != EM_NONE) {
> + zfree(&ff->ph->env.arch);
> + perf_env__arch(&ff->ph->env);

If the strdup() inside perf_env__arch() fails due to memory constraints,
it returns NULL and env->arch remains NULL.

Since process_arch() unconditionally returns 0 here, could this lead to a
NULL pointer dereference later if strncmp() is called on the architecture
string?

> + }
> + ff->ph->env.e_machine = saved_e_machine;
> + return 0;
> +}
> +
> static int process_e_machine(struct feat_fd *ff, void *data __maybe_unused)
> {
> int ret;
[ ... ]
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index fe0de2a0277f0..726568b888039 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -3023,14 +3023,16 @@ uint16_t perf_session__e_machine(struct perf_session *session, uint32_t *e_flags
> return EM_HOST;
> }
>
> + /* Is the env caching an e_machine? */
> env = perf_session__env(session);
> - if (env && env->e_machine != EM_NONE) {
> - if (e_flags)
> - *e_flags = env->e_flags;
> -
> - return env->e_machine;
> - }
> + if (env && env->e_machine != EM_NONE)
> + return perf_env__e_machine(env, e_flags);

For legacy perf.data files without HEADER_E_MACHINE, process_arch() restores
env->e_machine to EM_NONE.

Since the condition here explicitly requires env->e_machine != EM_NONE, won't
this completely bypass the new lazy evaluator and fall back to EM_HOST,
potentially breaking cross-architecture analysis for legacy files?

>
> + /*
> + * Compute from threads, note this is more accurate than
> + * perf_env__e_machine that falls back on EM_HOST and doesn't consider
> + * mixed 32-bit and 64-bit threads.
> + */
> machines__for_each_thread(&session->machines,
> perf_session__e_machine_cb,
> &args);

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