Messages in this thread | | | Date | Tue, 7 May 2019 10:51:53 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH] fix use-after-free in perf_sched__lat |
| |
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@huawei.com> > Signed-off-by: Wei Li <liwei391@huawei.com> > --- > 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 >
| |