Messages in this thread Patch in this message | | | Date | Mon, 6 Jan 2020 20:55:06 +0800 | From | Leo Yan <> | Subject | Re: [PATCH] perf parse: Copy string to perf_evsel_config_term |
| |
Hi Jiri,
On Mon, Jan 06, 2020 at 12:36:10PM +0100, Jiri Olsa wrote: > 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?
Yes, exactly.
> > > > 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
Though term->val.str is strdup-ed, after merged the commit 1dc925568f01 ("perf parse: Add a deep delete for parse event terms"), term->val.str is freed in the code [1].
If term->val.str should not be freed, how about to rollback the code as below:
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index 5972acdd40d6..f3211cde0d9f 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -2955,7 +2955,12 @@ void parse_events_terms__purge(struct list_head *terms) list_for_each_entry_safe(term, h, terms, list) { list_del_init(&term->list); - parse_events_term__delete(term); + + if (term->array.nr_ranges) + zfree(&term->array.ranges); + + zfree(&term->config); + free(term); } }
Thanks, Leo [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/parse-events.c?h=v5.5-rc5#n2911
> > jirka >
| |