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 <[email protected]>
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/[email protected]?part=8
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.