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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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@redhat.com>
 
> Signed-off-by: Dario Petrillo <dario.pk1@gmail.com>
> 
> ---
>  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