Re: [PATCH] perf tui: avoid crash when annotating recursive functions

From: Arnaldo Carvalho de Melo
Date: Mon Jan 10 2022 - 13:51:15 EST


Em Mon, Jan 10, 2022 at 12:44:41AM +0100, Dario Petrillo escreveu:
> In perf report, entering a recursive function from inside of itself
> (either directly of indirectly through some other function) results in
> calling symbol__annotate2 multiple times, and freeing the whole disassembly
> when exiting from the innermost instance. The first issue causes the
> function's disassembly to be duplicated, and the latter a heap
> use-after-free (and crash) when trying to access the disassembly again.
>
> I reproduced the bug on perf 5.11.22 (Ubuntu 20.04.3 LTS) and 5.16.rc8
> with the following testcase (compile with gcc recursive.c -o recursive).
> To reproduce:
> - perf record ./recursive
> - perf report
> - enter fibonacci and annotate it
> - move the cursor on one of the "callq fibonacci" instructions and press enter
> - at this point there will be two copies of the function in the disassembly
> - go back by pressing q, and perf will crash
>
> #include <stdio.h>
>
> int fibonacci(int n)
> {
> if(n <= 2) return 1;
> return fibonacci(n-1) + fibonacci(n-2);
> }
>
> int main()
> {
> printf("%d\n", fibonacci(40));
> }
>
> This patch addresses the issue by annotating a function and freeing the
> associated memory on exit only if no annotation is already present,
> so that a recursive function is only annotated on entry.

Problem reproduced, patch applied, problem fixed.

Thanks, applied.

Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>

> Signed-off-by: Dario Petrillo <dario.pk1@xxxxxxxxx>
>
> ---
> tools/perf/ui/browsers/annotate.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index e81c2493efdf..44ba900828f6 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -966,6 +966,7 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> .opts = opts,
> };
> int ret = -1, err;
> + int not_annotated = list_empty(&notes->src->source);
>
> if (sym == NULL)
> return -1;
> @@ -973,13 +974,15 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> if (ms->map->dso->annotate_warned)
> return -1;
>
> - err = symbol__annotate2(ms, evsel, opts, &browser.arch);
> - if (err) {
> - char msg[BUFSIZ];
> - ms->map->dso->annotate_warned = true;
> - symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
> - ui__error("Couldn't annotate %s:\n%s", sym->name, msg);
> - goto out_free_offsets;
> + if (not_annotated) {
> + err = symbol__annotate2(ms, evsel, opts, &browser.arch);
> + if (err) {
> + char msg[BUFSIZ];
> + ms->map->dso->annotate_warned = true;
> + symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
> + ui__error("Couldn't annotate %s:\n%s", sym->name, msg);
> + goto out_free_offsets;
> + }
> }
>
> ui_helpline__push("Press ESC to exit");
> @@ -994,9 +997,11 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>
> ret = annotate_browser__run(&browser, evsel, hbt);
>
> - annotated_source__purge(notes->src);
> + if(not_annotated)
> + annotated_source__purge(notes->src);
>
> out_free_offsets:
> - zfree(&notes->offsets);
> + if(not_annotated)
> + zfree(&notes->offsets);
> return ret;
> }
> --
> 2.25.1

--

- Arnaldo