Re: [PATCH v5 01/58] perf inject: Fix itrace branch stack synthesis

sashiko-bot
2026-04-24T17:32:46Z

Sashiko AI review found 5 potential issue(s):
- [High] Uninitialized `header.misc` field causes heap memory leak and data corruption.
- [High] The `last_branch` condition bypasses `cut_auxtrace_sample`, leaking AUX data and corrupting the `perf.data` format.
- [High] Hardcoded `branch_sample_type=0` in injection callbacks omits `hw_idx`, misaligning synthetic event layouts.
- [High] Integer truncation of event size at `PERF_SAMPLE_MAX_SIZE` causes 0-size records and parser DoS.
- [High] Double-counting callchain size leaks uninitialized heap data into output file.
--

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

perf inject: Fix itrace branch stack synthesis

When using "perf inject --itrace=L" to synthesize branch stacks from
AUX data, several issues caused failures:

1. The synthesized samples were delivered without the
PERF_SAMPLE_BRANCH_STACK flag if it was not in the original event's
sample_type. Fixed by using sample_type | evsel->synth_sample_type
in intel_pt_deliver_synth_event.

2. The record layout was misaligned because of inconsistent handling
of PERF_SAMPLE_BRANCH_HW_INDEX. Fixed by explicitly writing nr and
hw_idx in perf_event__synthesize_sample.

3. Modifying evsel->core.attr.sample_type early in __cmd_inject caused
parse failures for subsequent records in the input file. Fixed by
moving this modification to just before writing the header.

4. perf_event__repipe_sample was narrowed to only synthesize samples
when branch stack injection was requested, and restored the use of
perf_inject__cut_auxtrace_sample as a fallback to preserve
functionality.

Assisted-by: Gemini:gemini-3.1-pro-preview
Signed-off-by: Ian Rogers <irogers@google.com>

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index f174bc69cec453..88c0ef4f5ff198 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -375,7 +375,59 @@ static int perf_event__repipe_sample(const struct perf_tool *tool,
>
> build_id__mark_dso_hit(tool, event, sample, evsel, machine);
>
> - if (inject->itrace_synth_opts.set && sample->aux_sample.size) {
> + if (inject->itrace_synth_opts.set &&
> + (inject->itrace_synth_opts.last_branch ||
> + inject->itrace_synth_opts.add_last_branch)) {

Can this bypass the perf_inject__cut_auxtrace_sample() call and corrupt
the perf.data format?

If an event has AUX data and last_branch is true but add_last_branch is
false, it skips the block that temporarily clears PERF_SAMPLE_AUX.

The event is then synthesized with its full AUX payload, bypassing the
else if block intended to cut the AUX data.

Later, __cmd_inject() unconditionally strips PERF_SAMPLE_AUX from the
file's global header. Does this leave unadvertised AUX payloads inside
sample records, causing parsers to miscalculate boundaries?

> + union perf_event *event_copy = (void *)inject->event_copy;
> + struct branch_stack dummy_bs = { .nr = 0 };
> + int err;

[ ... ]

> + if (sz > PERF_SAMPLE_MAX_SIZE) {
> + pr_err("Sample size %zu exceeds max size %d\n", sz, PERF_SAMPLE_MAX_SIZE);
> + return -EFAULT;
> + }

Does this allow integer truncation leading to zero-sized records?

If sz evaluates to exactly 65536 (PERF_SAMPLE_MAX_SIZE), the condition
passes. Since event_copy->header.size is a __u16, assigning 65536 silently
truncates it to 0.

Could this cause downstream parsers relying on event->header.size to enter
an infinite loop?

> +
> + event_copy->header.type = PERF_RECORD_SAMPLE;
> + event_copy->header.size = sz;

Does this leak uninitialized heap memory and corrupt metadata?

The event_copy buffer is allocated via malloc() which does not zero out
memory.

The header.type and header.size fields are set here, but the header.misc
field is omitted. perf_event__synthesize_sample() writes the payload but
does not touch the header.

Could this cause the output perf.data file to contain garbage for critical
flags like the CPU mode (PERF_RECORD_MISC_USER or KERNEL) and exact IP
flags?

> +
> + err = perf_event__synthesize_sample(event_copy, evsel->core.attr.sample_type,
> + evsel->core.attr.read_format,
> + evsel->core.attr.branch_sample_type, sample);
> +
> + evsel->core.attr.sample_type = orig_type;

[ ... ]

> @@ -464,7 +516,8 @@ static int perf_event__convert_sample_callchain(const struct perf_tool *tool,
> sample_type &= ~(PERF_SAMPLE_STACK_USER | PERF_SAMPLE_REGS_USER);
>
> perf_event__synthesize_sample(event_copy, sample_type,
> - evsel->core.attr.read_format, sample);
> + evsel->core.attr.read_format,
> + evsel->core.attr.branch_sample_type, sample);
> return perf_event__repipe_synth(tool, event_copy);
> }

This isn't a regression from this commit, but does
perf_event__convert_sample_callchain() double-count the callchain size?

It calculates the new sample_size by taking the original event->header.size,
subtracting stack and regs sizes, and adding the new callchain size, but it
never subtracts the old callchain size.

Does this cause event_copy->header.size to be larger than the actual
synthesized payload, leaking uninitialized heap memory into the output file?

> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index fc9eec8b54b824..2dce6106c038d7 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -1731,8 +1731,12 @@ static void intel_pt_prep_b_sample(struct intel_pt *pt,
> static int intel_pt_inject_event(union perf_event *event,
> struct perf_sample *sample, u64 type)
> {
> - event->header.size = perf_event__sample_event_size(sample, type, 0);
> - return perf_event__synthesize_sample(event, type, 0, sample);
> + event->header.type = PERF_RECORD_SAMPLE;
> + event->header.size = perf_event__sample_event_size(sample, type, /*read_format=*/0,
> + /*branch_sample_type=*/0);
> +
> + return perf_event__synthesize_sample(event, type, /*read_format=*/0,
> + /*branch_sample_type=*/0, sample);
> }

Does passing 0 for branch_sample_type misalign the synthetic event layouts?

If the corresponding synthetic evsel header natively has
PERF_SAMPLE_BRANCH_HW_INDEX set, the output file will contain an evsel
claiming hw_idx exists.

However, because the payload is synthesized here with 0, it will lack the
8-byte hw_idx field. Could this misalign the reading of branch entries and
subsequent fields?

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