Messages in this thread | | | Date | Mon, 6 Jan 2020 12:36:10 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH] perf parse: Copy string to perf_evsel_config_term |
| |
On Thu, Jan 02, 2020 at 11:13:26PM +0800, Leo Yan wrote: > perf with CoreSight fails to record trace data with command: > > perf record -e cs_etm/@tmc_etr0/u --per-thread ls > failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a > directory)/perf/ > > This failure is root caused with the commit 1dc925568f01 ("perf > parse: Add a deep delete for parse event terms"). > > The log shows, cs_etm fails to parse the sink attribution; cs_etm event > relies on the event configuration to pass sink name, but the event > specific configuration data cannot be passed properly with flow: > > get_config_terms() > ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str); > __t->val.drv_cfg = term->val.str; > `> __t->val.drv_cfg is assigned to term->val.str; > > parse_events_terms__purge() > parse_events_term__delete() > zfree(&term->val.str); > `> term->val.str is freed and assigned to NULL pointer; > > cs_etm_set_sink_attr() > sink = __t->val.drv_cfg; > `> sink string has been freed.
so your problem is that the data is freed before you use it?
> > To fix this issue, in the function get_config_terms(), this patch > changes from directly assignment pointer value for the strings to > use strdup() for allocation a new duplicate string for the cases: > > perf_evsel_config_term::val.callgraph > perf_evsel_config_term::val.branch > perf_evsel_config_term::val.drv_cfg. > > Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms") > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > tools/perf/util/parse-events.c | 49 ++++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 20 deletions(-) > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index ed7c008b9c8b..5972acdd40d6 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -1220,7 +1220,6 @@ static int get_config_terms(struct list_head *head_config, > struct list_head *head_terms __maybe_unused) > { > #define ADD_CONFIG_TERM(__type, __name, __val) \ > -do { \ > struct perf_evsel_config_term *__t; \ > \ > __t = zalloc(sizeof(*__t)); \ > @@ -1229,9 +1228,19 @@ do { \ > \ > INIT_LIST_HEAD(&__t->list); \ > __t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \ > - __t->val.__name = __val; \ > __t->weak = term->weak; \ > - list_add_tail(&__t->list, head_terms); \ > + list_add_tail(&__t->list, head_terms) > + > +#define ADD_CONFIG_TERM_VAL(__type, __name, __val) \ > +do { \ > + ADD_CONFIG_TERM(__type, __name, __val); \ > + __t->val.__name = __val; \ > +} while (0) > + > +#define ADD_CONFIG_TERM_STR(__type, __name, __val) \ > +do { \ > + ADD_CONFIG_TERM(__type, __name, __val); \ > + __t->val.__name = strdup(__val); \ > } while (0)
the term->val.str is supposed to be already strdup-ed, so this seems wrong and leaking mem
jirka
| |