Messages in this thread | | | Date | Thu, 7 Nov 2019 10:42:26 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH v2] perf tests: Fix out of bounds memory access |
| |
On Thu, Nov 07, 2019 at 10:02:44AM +0800, Leo Yan wrote: > The test case 'Read backward ring buffer' failed on 32-bit architectures > which were found by LKFT perf testing. The test failed on arm32 x15 > device, qemu_arm32, qemu_i386, and found intermittent failure on i386; > the failure log is as below: > > 50: Read backward ring buffer : > --- start --- > test child forked, pid 510 > Using CPUID GenuineIntel-6-9E-9 > mmap size 1052672B > mmap size 8192B > Finished reading overwrite ring buffer: rewind > free(): invalid next size (fast) > test child interrupted > ---- end ---- > Read backward ring buffer: FAILED! > > The log hints there have issue for memory usage, thus free() reports > error 'invalid next size' and directly exit for the case. Finally, this > issue is root caused as out of bounds memory access for the data array > 'evsel->id'. > > The backward ring buffer test invokes do_test() twice. 'evsel->id' is > allocated at the first call with the flow: > > test__backward_ring_buffer() > `-> do_test() > `-> evlist__mmap() > `-> evlist__mmap_ex() > `-> perf_evsel__alloc_id() > > So 'evsel->id' is allocated with one item, and it will be used in > function perf_evlist__id_add(): > > evsel->id[0] = id > evsel->ids = 1 > > At the second call for do_test(), it skips to initialize 'evsel->id' > and reuses the array which is allocated in the first call. But > 'evsel->ids' contains the stale value. Thus: > > evsel->id[1] = id -> out of bound access > evsel->ids = 2 > > To fix this issue, we will use evlist__open() and evlist__close() pair > functions to prepare and cleanup context for evlist; so 'evsel->id' and > 'evsel->ids' can be initialized properly when invoke do_test() and avoid > the out of bounds memory access.
right, we need to solve this on libperf level, so it's possible to call mmap/munmap multiple time without close/open.. I'll try to send something, but meanwhile this is good workaround
Reviewed-by: Jiri Olsa <jolsa@kernel.org>
thanks, jirka
> > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > tools/perf/tests/backward-ring-buffer.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/perf/tests/backward-ring-buffer.c b/tools/perf/tests/backward-ring-buffer.c > index 338cd9faa835..5128f727c0ef 100644 > --- a/tools/perf/tests/backward-ring-buffer.c > +++ b/tools/perf/tests/backward-ring-buffer.c > @@ -147,6 +147,15 @@ int test__backward_ring_buffer(struct test *test __maybe_unused, int subtest __m > goto out_delete_evlist; > } > > + evlist__close(evlist); > + > + err = evlist__open(evlist); > + if (err < 0) { > + pr_debug("perf_evlist__open: %s\n", > + str_error_r(errno, sbuf, sizeof(sbuf))); > + goto out_delete_evlist; > + } > + > err = do_test(evlist, 1, &sample_count, &comm_count); > if (err != TEST_OK) > goto out_delete_evlist; > -- > 2.17.1 >
| |