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

[liwei (GF)]

Hi Jiriï
Thanks for your reply.

On 2019/5/7 16:51, Jiri Olsa wrote:
> 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?

When we set env "MALLOC_MMAP_THRESHOLD_=0"ïthe memory-allocation functions employ
mmap instead of increasing the program break using sbrk what may be maintained
with cache. Thus it can report "Segmentation fault" immediately when going into
this use-after-free code.

> 
>> 	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,
> 
Sorry, this place has been changed recently in mainline code, i will update to
the latest code.

> 
>> +		.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

Yes, what you suggest is reasonable, i will send a v2 with your suggestion soon.

Thanksï
Wei