Re: [RFC PATCH v1 8/8] perf tests: Add build_id_offset test coverage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] Heap Use-After-Free of `thread` reference handle in `mmap_events`.
- [Medium] Raw pointer comparison of reference-counted `struct map` handles causes spurious test failures.
- [Medium] Silent test bypass due to missing validation of `PERF_SAMPLE_BUILD_ID_OFFSET` fields.
- [Medium] Stack Out-Of-Bounds Read when synthesizing `PERF_SAMPLE_CALLCHAIN_BUILD_ID_OFFSET`.
- [Low] Missing explicit `<string.h>` inclusion causes compilation failures on musl libc.
--

--- Patch [8]: [RFC PATCH v1 8/8] perf tests: Add build_id_offset test coverage ---
commit 2b1a4526386e7ebd56cc41f452ac5f172c32e199
Author: Ian Rogers <irogers@google.com>

perf tests: Add build_id_offset test coverage

Test the kernel interface using sys_perf_event_open, test perf inject
--sample-buildids with pipe mode and callchains, and test perf record
--buildid-offset across multiple sampling modes.

> diff --git a/tools/perf/tests/mmap-thread-lookup.c b/tools/perf/tests/mmap-thread-lookup.c
> index dca8bbfe780a7..65c72ed896ac2 100644
> --- a/tools/perf/tests/mmap-thread-lookup.c
> +++ b/tools/perf/tests/mmap-thread-lookup.c
[ ... ]
> @@ -211,6 +213,40 @@ static int mmap_events(synth_cb synth)
>  
>  		pr_debug("map %p, addr %" PRIx64 "\n", al.map, map__start(al.map));
>  
> +		/* Verify lookup by build ID and offset */
> +		{
> +			struct dso *dso = dso__get(map__dso(al.map));
> +
> +			if (dso) {
> +				const struct build_id *bid = dso__bid(dso);
> +
> +				if (bid && bid->size > 0) {
> +					struct addr_location al2;
> +					struct perf_build_id pbid = { .size = bid->size };
> +					u64 offset = map__dso_map_ip(al.map,
> +								     (unsigned long)(td->map + 1));
> +
> +					memcpy(pbid.data, bid->data, bid->size);

[Severity: Low]
Does this code need to explicitly include <string.h>?

Without the explicit include, calling memcpy() causes compilation failures on
systems using musl libc.

> +					addr_location__init(&al2);
> +					thread__find_map(thread, &(struct perf_sample){

[Severity: Medium]
Can this use-after-free the thread reference handle?

Looking at mmap_events(), thread__put(thread) is called right after the first
thread__find_map() call.

This means the newly added test code dereferences the freed thread pointer
here. Under REFCNT_CHECKING (ASAN), this invalidates the handle and poisons
the memory.

> +								.cpumode = PERF_RECORD_MISC_USER,
> +								.ip = 0,
> +								.bid = {
> +									.bid = &pbid,
> +									.offset = offset,
> +								},
> +							 }, &al2);
> +
> +					if (al2.map != al.map) {

[Severity: Medium]
Will this raw pointer comparison of reference-counted struct map handles cause
spurious test failures?

When perf is compiled with REFCNT_CHECKING, map__get() creates a newly
allocated wrapper struct for each reference, meaning two handles to the same
underlying map will have different pointer addresses.

Should this use RC_CHK_EQUAL(al2.map, al.map)?

> +						pr_debug("failed, couldn't find map by build id and offset\n");
> +						err = -1;
> +					}
> +					addr_location__exit(&al2);
> +				}
> +				dso__put(dso);
> +			}
> +		}
> +
>  		addr_location__exit(&al);
>  		if (err)
>  			break;

> diff --git a/tools/perf/tests/sample-parsing.c b/tools/perf/tests/sample-parsing.c
> index 20cab91ceaeb2..b49b6dcf10cdd 100644
> --- a/tools/perf/tests/sample-parsing.c
> +++ b/tools/perf/tests/sample-parsing.c
[ ... ]
> @@ -229,6 +229,36 @@ static int do_test(u64 sample_type, u64 sample_regs, u64 read_format)
>  	const u32 raw_data[] = {0x12345678, 0x0a0b0c0d, 0x11020304, 0x05060708, 0 };
>  	const u64 data[] = {0x2211443366558877ULL, 0, 0xaabbccddeeff4321ULL};
>  	const u64 aux_data[] = {0xa55a, 0, 0xeeddee, 0x0282028202820282};
> +	const struct perf_build_id bid = {
> +		.size = 20,
> +		.data = {
> +			'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
> +			'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
> +		}
> +	};
> +	const struct perf_build_id callchain_bids[] = {

[Severity: Medium]
Will this result in a stack out-of-bounds read when synthesizing
PERF_SAMPLE_CALLCHAIN_BUILD_ID_OFFSET?

callchain_bids is declared as an array of 3 struct perf_build_id elements
(3 * 24 = 72 bytes).

However, perf_event__synthesize_sample() expects this array to contain
elements of size sizeof(struct perf_build_id) + sizeof(u64) (32 bytes).
It calculates the size as 96 bytes and calls memcpy(), reading past the end
of the stack array.

> +		{
> +			.size = 20,
> +			.data = {
> +				'9', '8', '7', '6', '5', '4', '3', '2', '1', '0',
> +				'9', '8', '7', '6', '5', '4', '3', '2', '1', '0',
> +			}
> +		},
[ ... ]
> @@ -277,6 +307,11 @@ static int do_test(u64 sample_type, u64 sample_regs, u64 read_format)
>  			.size	= sizeof(aux_data),
>  			.data	= (void *)aux_data,
>  		},
> +		.bid		= {
> +			.bid = (struct perf_build_id *)&bid,
> +			.offset = 0,
> +		},
> +		.callchain_bids = (struct perf_build_id *)callchain_bids,

[Severity: Medium]
Are the new PERF_SAMPLE_BUILD_ID_OFFSET fields actually being validated?

do_test() synthesizes events with bid and callchain_bids data here, but
the samples_same() validation function completely omits the COMP() checks
for these new flags.

Does this mean the parsing is never actually verified?

>  	};
>  	struct sample_read_value values[] = {{1, 5, 0}, {9, 3, 0}, {2, 7, 0}, {6, 4, 1},};
>  	struct perf_sample sample_out, sample_out_endian;

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