Re: [PATCH] fix use-after-free in perf_sched__lat

[Jiri Olsa]

On Fri, May 03, 2019 at 10:35:55AM +0800, Wei Li wrote:
> After thread is added to machine->threads[i].dead in
> __machine__remove_thread, the machine->threads[i].dead is freed
> when calling free(session) in perf_session__delete(). So it get a
> Segmentation fault when accessing it in thread__put().
> 
> In this patch, we delay the perf_session__delete until all threads
> have been deleted.
> 
> This can be reproduced by following steps:
> 	ulimit -c unlimited
> 	export MALLOC_MMAP_THRESHOLD_=0

what's this for?

> 	perf sched record sleep 10
> 	perf sched latency --sort max
> 	Segmentation fault (core dumped)
> 
> Signed-off-by: Zhipeng Xie <xiezhipeng1@xxxxxxxxxx>
> Signed-off-by: Wei Li <liwei391@xxxxxxxxxx>
> ---
>  tools/perf/builtin-sched.c | 44 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index cbf39dab19c1..17849ae2eb1e 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3130,11 +3130,48 @@ static void perf_sched__merge_lat(struct perf_sched *sched)
>  static int perf_sched__lat(struct perf_sched *sched)
>  {
>  	struct rb_node *next;
> +	const struct perf_evsel_str_handler handlers[] = {
> +		{ "sched:sched_switch",	      process_sched_switch_event, },
> +		{ "sched:sched_stat_runtime", process_sched_runtime_event, },
> +		{ "sched:sched_wakeup",	      process_sched_wakeup_event, },
> +		{ "sched:sched_wakeup_new",   process_sched_wakeup_event, },
> +		{ "sched:sched_migrate_task", process_sched_migrate_task_event, },
> +	};
> +	struct perf_session *session;
> +	struct perf_data data = {
> +		.file      = {
> +			.path = input_name,
> +		},

I can't compile this:

builtin-sched.c: In function âperf_sched__latâ:
builtin-sched.c:3144:12: error: initialization discards âconstâ qualifier from pointer target type [-Werror=discarded-qualifiers]
    .path = input_name,


> +		.mode      = PERF_DATA_MODE_READ,
> +		.force     = sched->force,
> +	};
> +	int rc = -1;
>  
>  	setup_pager();
>  
> -	if (perf_sched__read_events(sched))

so it's basically perf_sched__read_events code in here now, right?

might be better to add __perf_sched__read_events function
that would take session argument, something like:

        session = perf_session__new(&data, false, &sched->tool);
	...
	__perf_sched__read_events(sched, session)
	...
	perf_session__delete(session);

to avoid the code ducplication

thanks,
jirka

> +	session = perf_session__new(&data, false, &sched->tool);
> +	if (session == NULL) {
> +		pr_debug("No Memory for session\n");
>  		return -1;
> +	}
> +
> +	symbol__init(&session->header.env);
> +
> +	if (perf_session__set_tracepoints_handlers(session, handlers))
> +		goto out_delete;
> +
> +	if (perf_session__has_traces(session, "record -R")) {
> +		int err = perf_session__process_events(session);
> +
> +		if (err) {
> +			pr_err("Failed to process events, error %d", err);
> +			goto out_delete;
> +		}
> +
> +		sched->nr_events      = session->evlist->stats.nr_events[0];
> +		sched->nr_lost_events = session->evlist->stats.total_lost;
> +		sched->nr_lost_chunks = session->evlist->stats.nr_events[PERF_RECORD_LOST];
> +	}
>  
>  	perf_sched__merge_lat(sched);
>  	perf_sched__sort_lat(sched);
> @@ -3163,7 +3200,10 @@ static int perf_sched__lat(struct perf_sched *sched)
>  	print_bad_events(sched);
>  	printf("\n");
>  
> -	return 0;
> +	rc = 0;
> +out_delete:
> +	perf_session__delete(session);
> +	return rc;
>  }
>  
>  static int setup_map_cpus(struct perf_sched *sched)
> -- 
> 2.17.1
>