lkml.org 
[lkml]   [2020]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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
>

\
 
 \ /
  Last update: 2020-01-06 13:55    [W:0.105 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site