lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 6 Jan 2020 20:55:06 +0800 From: Leo Yan <leo.yan@...aro.org> To: Jiri Olsa <jolsa@...hat.com> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, Mark Rutland <mark.rutland@....com>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>, Andi Kleen <ak@...ux.intel.com>, linux-kernel@...r.kernel.org 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/@..._etr0/u --per-thread ls > > failed to set sink "" on event cs_etm/@..._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@...aro.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 >
Powered by blists - more mailing lists