[PATCH] perf tools: avoid reading out of scope array

0 views
Skip to first unread message

Ian Rogers

unread,
Oct 17, 2019, 1:05:39 PM10/17/19
to He Kuang, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Andi Kleen, linux-...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Modify tracepoint name into 2 sys components and assemble at use. This
avoids the sys_name array being out of scope at the point of use.
Bug caught with LLVM's address sanitizer with fuzz generated input of
":cs\1" to parse_events.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 36 +++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 48126ae4cd13..28be39a703c9 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -104,7 +104,8 @@ static void inc_group_count(struct list_head *list,
struct list_head *head;
struct parse_events_term *term;
struct tracepoint_name {
- char *sys;
+ char *sys1;
+ char *sys2;
char *event;
} tracepoint_name;
struct parse_events_array array;
@@ -425,9 +426,19 @@ tracepoint_name opt_event_config
if (error)
error->idx = @1.first_column;

- if (parse_events_add_tracepoint(list, &parse_state->idx, $1.sys, $1.event,
- error, $2))
- return -1;
+ if ($1.sys2) {
+ char sys_name[128];
+ snprintf(&sys_name, sizeof(sys_name), "%s-%s",
+ $1.sys1, $1.sys2);
+ if (parse_events_add_tracepoint(list, &parse_state->idx,
+ sys_name, $1.event,
+ error, $2))
+ return -1;
+ } else
+ if (parse_events_add_tracepoint(list, &parse_state->idx,
+ $1.sys1, $1.event,
+ error, $2))
+ return -1;

$$ = list;
}
@@ -435,19 +446,22 @@ tracepoint_name opt_event_config
tracepoint_name:
PE_NAME '-' PE_NAME ':' PE_NAME
{
- char sys_name[128];
- struct tracepoint_name tracepoint;
-
- snprintf(&sys_name, 128, "%s-%s", $1, $3);
- tracepoint.sys = &sys_name;
- tracepoint.event = $5;
+ struct tracepoint_name tracepoint = {
+ .sys1 = $1,
+ .sys2 = $3,
+ .event = $5,
+ };

$$ = tracepoint;
}
|
PE_NAME ':' PE_NAME
{
- struct tracepoint_name tracepoint = {$1, $3};
+ struct tracepoint_name tracepoint = {
+ .sys1 = $1,
+ .sys2 = NULL,
+ .event = $3,
+ };

$$ = tracepoint;
}
--
2.23.0.700.g56cf767bdb-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:00 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
The parse events parser leaks memory for certain expressions as well as
allowing a char* to reference stack, heap or .rodata. This series of patches
improves the hygeine and adds free-ing operations to reclaim memory in
the parser in error and non-error situations.

The series of patches was generated with LLVM's address sanitizer and
libFuzzer:
https://llvm.org/docs/LibFuzzer.html
called on the parse_events function with randomly generated input. With
the patches no leaks or memory corruption issues were present.

These patches are preferable to an earlier proposed patch:
perf tools: avoid reading out of scope array

Ian Rogers (9):
perf tools: add parse events append error
perf tools: splice events onto evlist even on error
perf tools: ensure config and str in terms are unique
perf tools: move ALLOC_LIST into a function
perf tools: avoid a malloc for array events
perf tools: add destructors for parse event terms
perf tools: before yyabort-ing free components
perf tools: if pmu configuration fails free terms
perf tools: add a deep delete for parse event terms

tools/perf/util/parse-events.c | 177 ++++++++++-----
tools/perf/util/parse-events.h | 3 +
tools/perf/util/parse-events.y | 388 ++++++++++++++++++++++++---------
tools/perf/util/pmu.c | 38 ++--
4 files changed, 431 insertions(+), 175 deletions(-)

--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:02 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Parse event error handling may overwrite one error string with another
creating memory leaks and masking errors. Introduce a helper routine
that appends error messages and avoids the memory leak.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 102 ++++++++++++++++++++++-----------
tools/perf/util/parse-events.h | 2 +
tools/perf/util/pmu.c | 36 ++++++------
3 files changed, 89 insertions(+), 51 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index db882f630f7e..4d42344698b8 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -182,6 +182,34 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)

#define MAX_EVENT_LENGTH 512

+void parse_events__append_error(struct parse_events_error *err, int idx,
+ char *str, char *help)
+{
+ char *new_str = NULL;
+
+ WARN(!str, "WARNING: failed to provide error string");
+ if (err->str) {
+ int ret;
+
+ if (err->help)
+ ret = asprintf(&new_str,
+ "%s (previous error: %s(help: %s))",
+ str, err->str, err->help);
+ else
+ ret = asprintf(&new_str,
+ "%s (previous error: %s)",
+ str, err->str);
+ if (ret < 0)
+ new_str = NULL;
+ else
+ zfree(&str);
+ }
+ err->idx = idx;
+ free(err->str);
+ err->str = new_str ?: str;
+ free(err->help);
+ err->help = help;
+}

struct tracepoint_path *tracepoint_id_to_path(u64 config)
{
@@ -931,13 +959,12 @@ static int check_type_val(struct parse_events_term *term,
if (type == term->type_val)
return 0;

- if (err) {
- err->idx = term->err_val;
- if (type == PARSE_EVENTS__TERM_TYPE_NUM)
- err->str = strdup("expected numeric value");
- else
- err->str = strdup("expected string value");
- }
+ if (err)
+ parse_events__append_error(err, term->err_val,
+ type == PARSE_EVENTS__TERM_TYPE_NUM
+ ? strdup("expected numeric value")
+ : strdup("expected string value"),
+ NULL);
return -EINVAL;
}

@@ -972,8 +999,11 @@ static bool config_term_shrinked;
static bool
config_term_avail(int term_type, struct parse_events_error *err)
{
+ char *err_str;
+
if (term_type < 0 || term_type >= __PARSE_EVENTS__TERM_TYPE_NR) {
- err->str = strdup("Invalid term_type");
+ parse_events__append_error(err, -1,
+ strdup("Invalid term_type"), NULL);
return false;
}
if (!config_term_shrinked)
@@ -992,9 +1022,9 @@ config_term_avail(int term_type, struct parse_events_error *err)
return false;

/* term_type is validated so indexing is safe */
- if (asprintf(&err->str, "'%s' is not usable in 'perf stat'",
- config_term_names[term_type]) < 0)
- err->str = NULL;
+ if (asprintf(&err_str, "'%s' is not usable in 'perf stat'",
+ config_term_names[term_type]) >= 0)
+ parse_events__append_error(err, -1, err_str, NULL);
return false;
}
}
@@ -1036,17 +1066,20 @@ do { \
case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
CHECK_TYPE_VAL(STR);
if (strcmp(term->val.str, "no") &&
- parse_branch_str(term->val.str, &attr->branch_sample_type)) {
- err->str = strdup("invalid branch sample type");
- err->idx = term->err_val;
+ parse_branch_str(term->val.str,
+ &attr->branch_sample_type)) {
+ parse_events__append_error(err, term->err_val,
+ strdup("invalid branch sample type"),
+ NULL);
return -EINVAL;
}
break;
case PARSE_EVENTS__TERM_TYPE_TIME:
CHECK_TYPE_VAL(NUM);
if (term->val.num > 1) {
- err->str = strdup("expected 0 or 1");
- err->idx = term->err_val;
+ parse_events__append_error(err, term->err_val,
+ strdup("expected 0 or 1"),
+ NULL);
return -EINVAL;
}
break;
@@ -1080,8 +1113,9 @@ do { \
case PARSE_EVENTS__TERM_TYPE_PERCORE:
CHECK_TYPE_VAL(NUM);
if ((unsigned int)term->val.num > 1) {
- err->str = strdup("expected 0 or 1");
- err->idx = term->err_val;
+ parse_events__append_error(err, term->err_val,
+ strdup("expected 0 or 1"),
+ NULL);
return -EINVAL;
}
break;
@@ -1089,9 +1123,9 @@ do { \
CHECK_TYPE_VAL(NUM);
break;
default:
- err->str = strdup("unknown term");
- err->idx = term->err_term;
- err->help = parse_events_formats_error_string(NULL);
+ parse_events__append_error(err, term->err_term,
+ strdup("unknown term"),
+ parse_events_formats_error_string(NULL));
return -EINVAL;
}

@@ -1141,11 +1175,10 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
return config_term_common(attr, term, err);
default:
- if (err) {
- err->idx = term->err_term;
- err->str = strdup("unknown term");
- err->help = strdup("valid terms: call-graph,stack-size\n");
- }
+ if (err)
+ parse_events__append_error(err, term->err_term,
+ strdup("unknown term"),
+ strdup("valid terms: call-graph,stack-size\n"));
return -EINVAL;
}

@@ -1323,10 +1356,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,

pmu = perf_pmu__find(name);
if (!pmu) {
- if (asprintf(&err->str,
+ char *err_str;
+
+ if (asprintf(&err_str,
"Cannot find PMU `%s'. Missing kernel support?",
- name) < 0)
- err->str = NULL;
+ name) >= 0)
+ parse_events__append_error(err, -1, err_str, NULL);
return -EINVAL;
}

@@ -2797,13 +2832,10 @@ void parse_events__clear_array(struct parse_events_array *a)
void parse_events_evlist_error(struct parse_events_state *parse_state,
int idx, const char *str)
{
- struct parse_events_error *err = parse_state->error;
-
- if (!err)
+ if (!parse_state->error)
return;
- err->idx = idx;
- err->str = strdup(str);
- WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
+
+ parse_events__append_error(parse_state->error, idx, strdup(str), NULL);
}

static void config_terms_list(char *buf, size_t buf_sz)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 769e07cddaa2..a7d42faeab0c 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -124,6 +124,8 @@ struct parse_events_state {
struct list_head *terms;
};

+void parse_events__append_error(struct parse_events_error *err, int idx,
+ char *str, char *help);
void parse_events__shrink_config_terms(void);
int parse_events__is_hardcoded_term(struct parse_events_term *term);
int parse_events_term__num(struct parse_events_term **term,
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index adbe97e941dd..0fc2a51bb953 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1050,9 +1050,9 @@ static int pmu_config_term(struct list_head *formats,
if (err) {
char *pmu_term = pmu_formats_string(formats);

- err->idx = term->err_term;
- err->str = strdup("unknown term");
- err->help = parse_events_formats_error_string(pmu_term);
+ parse_events__append_error(err, term->err_term,
+ strdup("unknown term"),
+ parse_events_formats_error_string(pmu_term));
free(pmu_term);
}
return -EINVAL;
@@ -1079,10 +1079,10 @@ static int pmu_config_term(struct list_head *formats,
if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) {
if (term->no_value &&
bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) {
- if (err) {
- err->idx = term->err_val;
- err->str = strdup("no value assigned for term");
- }
+ if (err)
+ parse_events__append_error(err, term->err_val,
+ strdup("no value assigned for term"),
+ NULL);
return -EINVAL;
}

@@ -1093,10 +1093,10 @@ static int pmu_config_term(struct list_head *formats,
pr_info("Invalid sysfs entry %s=%s\n",
term->config, term->val.str);
}
- if (err) {
- err->idx = term->err_val;
- err->str = strdup("expected numeric value");
- }
+ if (err)
+ parse_events__append_error(err, term->err_val,
+ strdup("expected numeric value"),
+ NULL);
return -EINVAL;
}

@@ -1108,11 +1108,15 @@ static int pmu_config_term(struct list_head *formats,
max_val = pmu_format_max_value(format->bits);
if (val > max_val) {
if (err) {
- err->idx = term->err_val;
- if (asprintf(&err->str,
- "value too big for format, maximum is %llu",
- (unsigned long long)max_val) < 0)
- err->str = strdup("value too big for format");
+ char *err_str;
+
+ parse_events__append_error(err, term->err_val,
+ asprintf(&err_str,
+ "value too big for format, maximum is %llu",
+ (unsigned long long)max_val) < 0
+ ? strdup("value too big for format")
+ : err_str,
+ NULL);
return -EINVAL;
}
/*
--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:05 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
If event parsing fails the event list is leaked, instead splice the list
onto the out result and let the caller cleanup.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 4d42344698b8..a8f8801bd127 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1962,15 +1962,20 @@ int parse_events(struct evlist *evlist, const char *str,

ret = parse_events__scanner(str, &parse_state, PE_START_EVENTS);
perf_pmu__parse_cleanup();
+
+ if (list_empty(&parse_state.list)) {
+ WARN_ONCE(true, "WARNING: event parser found nothing\n");
+ return -1;
+ }
+
+ /*
+ * Add list to the evlist even with errors to allow callers to clean up.
+ */
+ perf_evlist__splice_list_tail(evlist, &parse_state.list);
+
if (!ret) {
struct evsel *last;

- if (list_empty(&parse_state.list)) {
- WARN_ONCE(true, "WARNING: event parser found nothing\n");
- return -1;
- }
-
- perf_evlist__splice_list_tail(evlist, &parse_state.list);
evlist->nr_groups += parse_state.nr_groups;
last = evlist__last(evlist);
last->cmdline_group_boundary = true;
--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:07 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Make it easier to release memory associated with parse event terms by
duplicating the string for the config name and ensuring the val string
is a duplicate.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 33 ++++++++++++++++++++++++---------
tools/perf/util/parse-events.y | 4 +++-
2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index a8f8801bd127..f7c8d0853d71 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1424,7 +1424,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
char *str, struct list_head **listp)
{
- struct list_head *head;
struct parse_events_term *term;
struct list_head *list;
struct perf_pmu *pmu = NULL;
@@ -1441,19 +1440,30 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,

list_for_each_entry(alias, &pmu->aliases, list) {
if (!strcasecmp(alias->name, str)) {
+ struct list_head *head;
+ char *config;
+
head = malloc(sizeof(struct list_head));
if (!head)
return -1;
INIT_LIST_HEAD(head);
- if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
- str, 1, false, &str, NULL) < 0)
+ config = strdup(str);
+ if (!config)
+ return -1;
+ if (parse_events_term__num(&term,
+ PARSE_EVENTS__TERM_TYPE_USER,
+ config, 1, false, &config,
+ NULL) < 0) {
+ free(list);
+ free(config);
return -1;
+ }
list_add_tail(&term->list, head);

if (!parse_events_add_pmu(parse_state, list,
pmu->name, head,
true, true)) {
- pr_debug("%s -> %s/%s/\n", str,
+ pr_debug("%s -> %s/%s/\n", config,
pmu->name, alias->str);
ok++;
}
@@ -1462,8 +1472,10 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
}
}
}
- if (!ok)
+ if (!ok) {
+ free(list);
return -1;
+ }
*listp = list;
return 0;
}
@@ -2761,13 +2773,13 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
struct parse_events_term temp = {
.type_val = PARSE_EVENTS__TERM_TYPE_STR,
.type_term = PARSE_EVENTS__TERM_TYPE_USER,
- .config = config ?: (char *) "event",
+ .config = config ?: strdup("event"),
};

BUG_ON(idx >= PERF_COUNT_HW_MAX);
sym = &event_symbols_hw[idx];

- return new_term(term, &temp, (char *) sym->symbol, 0);
+ return new_term(term, &temp, strdup(sym->symbol), 0);
}

int parse_events_term__clone(struct parse_events_term **new,
@@ -2776,12 +2788,15 @@ int parse_events_term__clone(struct parse_events_term **new,
struct parse_events_term temp = {
.type_val = term->type_val,
.type_term = term->type_term,
- .config = term->config,
+ .config = term->config ? strdup(term->config) : NULL,
.err_term = term->err_term,
.err_val = term->err_val,
};

- return new_term(new, &temp, term->val.str, term->val.num);
+ if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
+ return new_term(new, &temp, NULL, term->val.num);
+ else
+ return new_term(new, &temp, strdup(term->val.str), 0);
}

int parse_events_copy_term_list(struct list_head *old,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 48126ae4cd13..27d6b187c9b1 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -644,9 +644,11 @@ PE_NAME array '=' PE_VALUE
PE_DRV_CFG_TERM
{
struct parse_events_term *term;
+ char *config = strdup($1);

+ ABORT_ON(!config);
ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
- $1, $1, &@1, NULL));
+ config, $1, &@1, NULL));
$$ = term;
}

--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:10 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Having a YYABORT in a macro makes it hard to free memory for components
of a rule. Separate the logic out.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 65 ++++++++++++++++++++++------------
1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 27d6b187c9b1..26cb65798522 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -25,12 +25,17 @@ do { \
YYABORT; \
} while (0)

-#define ALLOC_LIST(list) \
-do { \
- list = malloc(sizeof(*list)); \
- ABORT_ON(!list); \
- INIT_LIST_HEAD(list); \
-} while (0)
+static struct list_head* alloc_list()
+{
+ struct list_head *list;
+
+ list = malloc(sizeof(*list));
+ if (!list)
+ return NULL;
+
+ INIT_LIST_HEAD(list);
+ return list;
+}

static void inc_group_count(struct list_head *list,
struct parse_events_state *parse_state)
@@ -238,7 +243,8 @@ PE_NAME opt_pmu_config
if (error)
error->idx = @1.first_column;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
struct perf_pmu *pmu = NULL;
int ok = 0;
@@ -306,7 +312,8 @@ value_sym '/' event_config '/'
int type = $1 >> 16;
int config = $1 & 255;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_numeric(_parse_state, list, type, config, $3));
parse_events_terms__delete($3);
$$ = list;
@@ -318,7 +325,8 @@ value_sym sep_slash_slash_dc
int type = $1 >> 16;
int config = $1 & 255;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_numeric(_parse_state, list, type, config, NULL));
$$ = list;
}
@@ -327,7 +335,8 @@ PE_VALUE_SYM_TOOL sep_slash_slash_dc
{
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_tool(_parse_state, list, $1));
$$ = list;
}
@@ -339,7 +348,8 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT '-' PE_NAME_CACHE_OP_RESULT opt_e
struct parse_events_error *error = parse_state->error;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, $3, $5, error, $6));
parse_events_terms__delete($6);
$$ = list;
@@ -351,7 +361,8 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT opt_event_config
struct parse_events_error *error = parse_state->error;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, $3, NULL, error, $4));
parse_events_terms__delete($4);
$$ = list;
@@ -363,7 +374,8 @@ PE_NAME_CACHE_TYPE opt_event_config
struct parse_events_error *error = parse_state->error;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, NULL, NULL, error, $2));
parse_events_terms__delete($2);
$$ = list;
@@ -375,7 +387,8 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
(void *) $2, $6, $4));
$$ = list;
@@ -386,7 +399,8 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE sep_dc
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
(void *) $2, NULL, $4));
$$ = list;
@@ -397,7 +411,8 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
(void *) $2, $4, 0));
$$ = list;
@@ -408,7 +423,8 @@ PE_PREFIX_MEM PE_VALUE sep_dc
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
(void *) $2, NULL, 0));
$$ = list;
@@ -421,7 +437,8 @@ tracepoint_name opt_event_config
struct parse_events_error *error = parse_state->error;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
if (error)
error->idx = @1.first_column;

@@ -457,7 +474,8 @@ PE_VALUE ':' PE_VALUE opt_event_config
{
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_numeric(_parse_state, list, (u32)$1, $3, $4));
parse_events_terms__delete($4);
$$ = list;
@@ -468,7 +486,8 @@ PE_RAW opt_event_config
{
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, $1, $2));
parse_events_terms__delete($2);
$$ = list;
@@ -480,7 +499,8 @@ PE_BPF_OBJECT opt_event_config
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_load_bpf(parse_state, list, $1, false, $2));
parse_events_terms__delete($2);
$$ = list;
@@ -490,7 +510,8 @@ PE_BPF_SOURCE opt_event_config
{
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_load_bpf(_parse_state, list, $1, true, $2));
parse_events_terms__delete($2);
$$ = list;
--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:12 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Use realloc rather than malloc+memcpy to possibly avoid a memory
allocation when appending array elements.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 26cb65798522..545ab7cefc20 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -691,14 +691,12 @@ array_terms ',' array_term
struct parse_events_array new_array;

new_array.nr_ranges = $1.nr_ranges + $3.nr_ranges;
- new_array.ranges = malloc(sizeof(new_array.ranges[0]) *
- new_array.nr_ranges);
+ new_array.ranges = realloc($1.ranges,
+ sizeof(new_array.ranges[0]) *
+ new_array.nr_ranges);
ABORT_ON(!new_array.ranges);
- memcpy(&new_array.ranges[0], $1.ranges,
- $1.nr_ranges * sizeof(new_array.ranges[0]));
memcpy(&new_array.ranges[$1.nr_ranges], $3.ranges,
$3.nr_ranges * sizeof(new_array.ranges[0]));
- free($1.ranges);
free($3.ranges);
$$ = new_array;
}
--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:15 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
If parsing fails then destructors are ran to clean the up the stack.
Rename the head union member to make the term and evlist use cases more
distinct, this simplifies matching the correct destructor.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 69 +++++++++++++++++++++++-----------
1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 545ab7cefc20..4725b14b9db4 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -12,6 +12,7 @@
#include <stdio.h>
#include <linux/compiler.h>
#include <linux/types.h>
+#include <linux/zalloc.h>
#include "pmu.h"
#include "evsel.h"
#include "parse-events.h"
@@ -37,6 +38,25 @@ static struct list_head* alloc_list()
return list;
}

+static void free_list_evsel(struct list_head* list_evsel)
+{
+ struct perf_evsel *pos, *tmp;
+
+ list_for_each_entry_safe(pos, tmp, list_evsel, node) {
+ list_del_init(&pos->node);
+ perf_evsel__delete(pos);
+ }
+ free(list_evsel);
+}
+
+static void free_term(struct parse_events_term *term)
+{
+ if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
+ free(term->val.str);
+ zfree(&term->array.ranges);
+ free(term);
+}
+
static void inc_group_count(struct list_head *list,
struct parse_events_state *parse_state)
{
@@ -66,6 +86,7 @@ static void inc_group_count(struct list_head *list,
%type <num> PE_VALUE_SYM_TOOL
%type <num> PE_RAW
%type <num> PE_TERM
+%type <num> value_sym
%type <str> PE_NAME
%type <str> PE_BPF_OBJECT
%type <str> PE_BPF_SOURCE
@@ -76,37 +97,43 @@ static void inc_group_count(struct list_head *list,
%type <str> PE_EVENT_NAME
%type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
%type <str> PE_DRV_CFG_TERM
-%type <num> value_sym
-%type <head> event_config
-%type <head> opt_event_config
-%type <head> opt_pmu_config
+%destructor { free ($$); } <str>
%type <term> event_term
-%type <head> event_pmu
-%type <head> event_legacy_symbol
-%type <head> event_legacy_cache
-%type <head> event_legacy_mem
-%type <head> event_legacy_tracepoint
+%destructor { free_term ($$); } <term>
+%type <list_terms> event_config
+%type <list_terms> opt_event_config
+%type <list_terms> opt_pmu_config
+%destructor { parse_events_terms__delete ($$); } <list_terms>
+%type <list_evsel> event_pmu
+%type <list_evsel> event_legacy_symbol
+%type <list_evsel> event_legacy_cache
+%type <list_evsel> event_legacy_mem
+%type <list_evsel> event_legacy_tracepoint
+%type <list_evsel> event_legacy_numeric
+%type <list_evsel> event_legacy_raw
+%type <list_evsel> event_bpf_file
+%type <list_evsel> event_def
+%type <list_evsel> event_mod
+%type <list_evsel> event_name
+%type <list_evsel> event
+%type <list_evsel> events
+%type <list_evsel> group_def
+%type <list_evsel> group
+%type <list_evsel> groups
+%destructor { free_list_evsel ($$); } <list_evsel>
%type <tracepoint_name> tracepoint_name
-%type <head> event_legacy_numeric
-%type <head> event_legacy_raw
-%type <head> event_bpf_file
-%type <head> event_def
-%type <head> event_mod
-%type <head> event_name
-%type <head> event
-%type <head> events
-%type <head> group_def
-%type <head> group
-%type <head> groups
+%destructor { free ($$.sys); free ($$.event); } <tracepoint_name>
%type <array> array
%type <array> array_term
%type <array> array_terms
+%destructor { free ($$.ranges); } <array>

%union
{
char *str;
u64 num;
- struct list_head *head;
+ struct list_head *list_evsel;
+ struct list_head *list_terms;
struct parse_events_term *term;
struct tracepoint_name {
char *sys;
--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:18 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Yyabort doesn't destruct inputs and so this must be done manually before
using yyabort.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 250 +++++++++++++++++++++++++--------
1 file changed, 195 insertions(+), 55 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 4725b14b9db4..d1cceb3bc620 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -152,6 +152,7 @@ start_events: groups
{
struct parse_events_state *parse_state = _parse_state;

+ /* frees $1 */
parse_events_update_lists($1, &parse_state->list);
}

@@ -161,6 +162,7 @@ groups ',' group
struct list_head *list = $1;
struct list_head *group = $3;

+ /* frees $3 */
parse_events_update_lists(group, list);
$$ = list;
}
@@ -170,6 +172,7 @@ groups ',' event
struct list_head *list = $1;
struct list_head *event = $3;

+ /* frees $3 */
parse_events_update_lists(event, list);
$$ = list;
}
@@ -182,8 +185,14 @@ group:
group_def ':' PE_MODIFIER_EVENT
{
struct list_head *list = $1;
+ int err;

- ABORT_ON(parse_events__modifier_group(list, $3));
+ err = parse_events__modifier_group(list, $3);
+ free($3);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -196,6 +205,7 @@ PE_NAME '{' events '}'

inc_group_count(list, _parse_state);
parse_events__set_leader($1, list, _parse_state);
+ free($1);
$$ = list;
}
|
@@ -214,6 +224,7 @@ events ',' event
struct list_head *event = $3;
struct list_head *list = $1;

+ /* frees $3 */
parse_events_update_lists(event, list);
$$ = list;
}
@@ -226,13 +237,19 @@ event_mod:
event_name PE_MODIFIER_EVENT
{
struct list_head *list = $1;
+ int err;

/*
* Apply modifier on all events added by single event definition
* (there could be more events added for multiple tracepoint
* definitions via '*?'.
*/
- ABORT_ON(parse_events__modifier_event(list, $2, false));
+ err = parse_events__modifier_event(list, $2, false);
+ free($2);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -241,8 +258,14 @@ event_name
event_name:
PE_EVENT_NAME event_def
{
- ABORT_ON(parse_events_name($2, $1));
+ int err;
+
+ err = parse_events_name($2, $1);
free($1);
+ if (err) {
+ free_list_evsel($2);
+ YYABORT;
+ }
$$ = $2;
}
|
@@ -262,23 +285,32 @@ PE_NAME opt_pmu_config
{
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
- struct list_head *list, *orig_terms, *terms;
+ struct list_head *list = NULL, *orig_terms = NULL, *terms= NULL;
+ char *pattern = NULL;
+
+#define CLEANUP_YYABORT \
+ do { \
+ parse_events_terms__delete($2); \
+ parse_events_terms__delete(orig_terms); \
+ free($1); \
+ YYABORT; \
+ } while(0)

if (parse_events_copy_term_list($2, &orig_terms))
- YYABORT;
+ CLEANUP_YYABORT;

if (error)
error->idx = @1.first_column;

list = alloc_list();
- ABORT_ON(!list);
+ if (!list)
+ CLEANUP_YYABORT;
if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
struct perf_pmu *pmu = NULL;
int ok = 0;
- char *pattern;

if (asprintf(&pattern, "%s*", $1) < 0)
- YYABORT;
+ CLEANUP_YYABORT;

while ((pmu = perf_pmu__scan(pmu)) != NULL) {
char *name = pmu->name;
@@ -287,31 +319,32 @@ PE_NAME opt_pmu_config
strncmp($1, "uncore_", 7))
name += 7;
if (!fnmatch(pattern, name, 0)) {
- if (parse_events_copy_term_list(orig_terms, &terms)) {
- free(pattern);
- YYABORT;
- }
+ if (parse_events_copy_term_list(orig_terms, &terms))
+ CLEANUP_YYABORT;
if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
ok++;
parse_events_terms__delete(terms);
}
}

- free(pattern);
-
if (!ok)
- YYABORT;
+ CLEANUP_YYABORT;
}
parse_events_terms__delete($2);
parse_events_terms__delete(orig_terms);
+ free($1);
$$ = list;
+#undef CLEANUP_YYABORT
}
|
PE_KERNEL_PMU_EVENT sep_dc
{
struct list_head *list;
+ int err;

- if (parse_events_multi_pmu_add(_parse_state, $1, &list) < 0)
+ err = parse_events_multi_pmu_add(_parse_state, $1, &list);
+ free($1);
+ if (err < 0)
YYABORT;
$$ = list;
}
@@ -322,6 +355,8 @@ PE_PMU_EVENT_PRE '-' PE_PMU_EVENT_SUF sep_dc
char pmu_name[128];

snprintf(&pmu_name, 128, "%s-%s", $1, $3);
+ free($1);
+ free($3);
if (parse_events_multi_pmu_add(_parse_state, pmu_name, &list) < 0)
YYABORT;
$$ = list;
@@ -338,11 +373,16 @@ value_sym '/' event_config '/'
struct list_head *list;
int type = $1 >> 16;
int config = $1 & 255;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_numeric(_parse_state, list, type, config, $3));
+ err = parse_events_add_numeric(_parse_state, list, type, config, $3);
parse_events_terms__delete($3);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -374,11 +414,19 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT '-' PE_NAME_CACHE_OP_RESULT opt_e
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, $3, $5, error, $6));
+ err = parse_events_add_cache(list, &parse_state->idx, $1, $3, $5, error, $6);
parse_events_terms__delete($6);
+ free($1);
+ free($3);
+ free($5);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -387,11 +435,18 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT opt_event_config
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, $3, NULL, error, $4));
+ err = parse_events_add_cache(list, &parse_state->idx, $1, $3, NULL, error, $4);
parse_events_terms__delete($4);
+ free($1);
+ free($3);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -400,11 +455,17 @@ PE_NAME_CACHE_TYPE opt_event_config
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, NULL, NULL, error, $2));
+ err = parse_events_add_cache(list, &parse_state->idx, $1, NULL, NULL, error, $2);
parse_events_terms__delete($2);
+ free($1);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -413,11 +474,17 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
{
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
- (void *) $2, $6, $4));
+ err = parse_events_add_breakpoint(list, &parse_state->idx,
+ (void *) $2, $6, $4);
+ free($6);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -428,8 +495,11 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE sep_dc

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
- (void *) $2, NULL, $4));
+ if (parse_events_add_breakpoint(list, &parse_state->idx,
+ (void *) $2, NULL, $4)) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -437,11 +507,17 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
{
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
- (void *) $2, $4, 0));
+ err = parse_events_add_breakpoint(list, &parse_state->idx,
+ (void *) $2, $4, 0);
+ free($4);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -452,8 +528,11 @@ PE_PREFIX_MEM PE_VALUE sep_dc

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
- (void *) $2, NULL, 0));
+ if (parse_events_add_breakpoint(list, &parse_state->idx,
+ (void *) $2, NULL, 0)) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -463,29 +542,35 @@ tracepoint_name opt_event_config
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
if (error)
error->idx = @1.first_column;

- if (parse_events_add_tracepoint(list, &parse_state->idx, $1.sys, $1.event,
- error, $2))
- return -1;
+ err = parse_events_add_tracepoint(list, &parse_state->idx, $1.sys, $1.event,
+ error, $2);

+ parse_events_terms__delete($2);
+ free($1.sys);
+ free($1.event);
+ if (err) {
+ free(list);
+ return -1;
+ }
$$ = list;
}

tracepoint_name:
PE_NAME '-' PE_NAME ':' PE_NAME
{
- char sys_name[128];
struct tracepoint_name tracepoint;

- snprintf(&sys_name, 128, "%s-%s", $1, $3);
- tracepoint.sys = &sys_name;
+ ABORT_ON(asprintf(&tracepoint.sys, "%s-%s", $1, $3) < 0);
tracepoint.event = $5;
-
+ free($1);
+ free($3);
$$ = tracepoint;
}
|
@@ -500,11 +585,16 @@ event_legacy_numeric:
PE_VALUE ':' PE_VALUE opt_event_config
{
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_numeric(_parse_state, list, (u32)$1, $3, $4));
+ err = parse_events_add_numeric(_parse_state, list, (u32)$1, $3, $4);
parse_events_terms__delete($4);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -512,11 +602,16 @@ event_legacy_raw:
PE_RAW opt_event_config
{
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, $1, $2));
+ err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, $1, $2);
parse_events_terms__delete($2);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -525,22 +620,33 @@ PE_BPF_OBJECT opt_event_config
{
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_load_bpf(parse_state, list, $1, false, $2));
+ err = parse_events_load_bpf(parse_state, list, $1, false, $2);
parse_events_terms__delete($2);
+ free($1);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}
|
PE_BPF_SOURCE opt_event_config
{
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_load_bpf(_parse_state, list, $1, true, $2));
+ err = parse_events_load_bpf(_parse_state, list, $1, true, $2);
parse_events_terms__delete($2);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -573,6 +679,10 @@ opt_pmu_config:
start_terms: event_config
{
struct parse_events_state *parse_state = _parse_state;
+ if (parse_state->terms) {
+ parse_events_terms__delete ($1);
+ YYABORT;
+ }
parse_state->terms = $1;
}

@@ -582,7 +692,10 @@ event_config ',' event_term
struct list_head *head = $1;
struct parse_events_term *term = $3;

- ABORT_ON(!head);
+ if (!head) {
+ free_term(term);
+ YYABORT;
+ }
list_add_tail(&term->list, head);
$$ = $1;
}
@@ -603,8 +716,12 @@ PE_NAME '=' PE_NAME
{
struct parse_events_term *term;

- ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_USER,
- $1, $3, &@1, &@3));
+ if (parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_USER,
+ $1, $3, &@1, &@3)) {
+ free($1);
+ free($3);
+ YYABORT;
+ }
$$ = term;
}
|
@@ -612,8 +729,11 @@ PE_NAME '=' PE_VALUE
{
struct parse_events_term *term;

- ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
- $1, $3, false, &@1, &@3));
+ if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
+ $1, $3, false, &@1, &@3)) {
+ free($1);
+ YYABORT;
+ }
$$ = term;
}
|
@@ -622,7 +742,10 @@ PE_NAME '=' PE_VALUE_SYM_HW
struct parse_events_term *term;
int config = $3 & 255;

- ABORT_ON(parse_events_term__sym_hw(&term, $1, config));
+ if (parse_events_term__sym_hw(&term, $1, config)) {
+ free($1);
+ YYABORT;
+ }
$$ = term;
}
|
@@ -630,8 +753,11 @@ PE_NAME
{
struct parse_events_term *term;

- ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
- $1, 1, true, &@1, NULL));
+ if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
+ $1, 1, true, &@1, NULL)) {
+ free($1);
+ YYABORT;
+ }
$$ = term;
}
|
@@ -648,7 +774,10 @@ PE_TERM '=' PE_NAME
{
struct parse_events_term *term;

- ABORT_ON(parse_events_term__str(&term, (int)$1, NULL, $3, &@1, &@3));
+ if (parse_events_term__str(&term, (int)$1, NULL, $3, &@1, &@3)) {
+ free($3);
+ YYABORT;
+ }
$$ = term;
}
|
@@ -672,9 +801,13 @@ PE_NAME array '=' PE_NAME
{
struct parse_events_term *term;

- ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_USER,
- $1, $4, &@1, &@4));
-
+ if (parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_USER,
+ $1, $4, &@1, &@4)) {
+ free($1);
+ free($4);
+ free($2.ranges);
+ YYABORT;
+ }
term->array = $2;
$$ = term;
}
@@ -683,8 +816,12 @@ PE_NAME array '=' PE_VALUE
{
struct parse_events_term *term;

- ABORT_ON(parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
- $1, $4, false, &@1, &@4));
+ if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
+ $1, $4, false, &@1, &@4)) {
+ free($1);
+ free($2.ranges);
+ YYABORT;
+ }
term->array = $2;
$$ = term;
}
@@ -695,8 +832,11 @@ PE_DRV_CFG_TERM
char *config = strdup($1);

ABORT_ON(!config);
- ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
- config, $1, &@1, NULL));
+ if (parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
+ config, $1, &@1, NULL)) {
+ free($1);
+ YYABORT;
+ }

Ian Rogers

unread,
Oct 22, 2019, 8:54:21 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Avoid a memory leak when the configuration fails.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f7c8d0853d71..6bf64b3767cc 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1400,8 +1400,15 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
if (get_config_terms(head_config, &config_terms))
return -ENOMEM;

- if (perf_pmu__config(pmu, &attr, head_config, parse_state->error))
+ if (perf_pmu__config(pmu, &attr, head_config, parse_state->error)) {
+ struct perf_evsel_config_term *pos, *tmp;
+
+ list_for_each_entry_safe(pos, tmp, &config_terms, list) {
+ list_del_init(&pos->list);
+ free(pos);
+ }
return -EINVAL;
+ }

evsel = __add_event(list, &parse_state->idx, &attr,
get_config_name(head_config), pmu,
--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 22, 2019, 8:54:23 PM10/22/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Add a parse_events_term deep delete function so that owned strings and
arrays are freed.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 16 +++++++++++++---
tools/perf/util/parse-events.h | 1 +
tools/perf/util/parse-events.y | 12 ++----------
tools/perf/util/pmu.c | 2 +-
4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 6bf64b3767cc..5e7bebc8fad4 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2806,6 +2806,18 @@ int parse_events_term__clone(struct parse_events_term **new,
return new_term(new, &temp, strdup(term->val.str), 0);
}

+void parse_events_term__delete(struct parse_events_term *term)
+{
+ if (term->array.nr_ranges)
+ zfree(&term->array.ranges);
+
+ if (term->type_val != PARSE_EVENTS__TERM_TYPE_NUM)
+ zfree(&term->val.str);
+
+ zfree(&term->config);
+ free(term);
+}
+
int parse_events_copy_term_list(struct list_head *old,
struct list_head **new)
{
@@ -2836,10 +2848,8 @@ void parse_events_terms__purge(struct list_head *terms)
struct parse_events_term *term, *h;

list_for_each_entry_safe(term, h, terms, list) {
- if (term->array.nr_ranges)
- zfree(&term->array.ranges);
list_del_init(&term->list);
- free(term);
+ parse_events_term__delete(term);
}
}

diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index a7d42faeab0c..d1ade97e2357 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -139,6 +139,7 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
char *config, unsigned idx);
int parse_events_term__clone(struct parse_events_term **new,
struct parse_events_term *term);
+void parse_events_term__delete(struct parse_events_term *term);
void parse_events_terms__delete(struct list_head *terms);
void parse_events_terms__purge(struct list_head *terms);
void parse_events__clear_array(struct parse_events_array *a);
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d1cceb3bc620..649c63809bad 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -49,14 +49,6 @@ static void free_list_evsel(struct list_head* list_evsel)
free(list_evsel);
}

-static void free_term(struct parse_events_term *term)
-{
- if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
- free(term->val.str);
- zfree(&term->array.ranges);
- free(term);
-}
-
static void inc_group_count(struct list_head *list,
struct parse_events_state *parse_state)
{
@@ -99,7 +91,7 @@ static void inc_group_count(struct list_head *list,
%type <str> PE_DRV_CFG_TERM
%destructor { free ($$); } <str>
%type <term> event_term
-%destructor { free_term ($$); } <term>
+%destructor { parse_events_term__delete ($$); } <term>
%type <list_terms> event_config
%type <list_terms> opt_event_config
%type <list_terms> opt_pmu_config
@@ -693,7 +685,7 @@ event_config ',' event_term
struct parse_events_term *term = $3;

if (!head) {
- free_term(term);
+ parse_events_term__delete(term);
YYABORT;
}
list_add_tail(&term->list, head);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 0fc2a51bb953..081e55300237 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1258,7 +1258,7 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
info->metric_name = alias->metric_name;

list_del_init(&term->list);
- free(term);
+ parse_events_term__delete(term);
}

/*
--
2.23.0.866.gb869b98d4c-goog

Jiri Olsa

unread,
Oct 23, 2019, 4:29:21 AM10/23/19
to Ian Rogers, He Kuang, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Andi Kleen, linux-...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
nice catch, please enclose all multiline condition legs with {}

other than that

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka

Jiri Olsa

unread,
Oct 23, 2019, 4:36:20 AM10/23/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Tue, Oct 22, 2019 at 05:53:29PM -0700, Ian Rogers wrote:
> Parse event error handling may overwrite one error string with another
> creating memory leaks and masking errors. Introduce a helper routine
> that appends error messages and avoids the memory leak.

good idea, it became little messy with time ;-)
some comments below

thanks,
jirka


>
> Signed-off-by: Ian Rogers <iro...@google.com>
> ---
> tools/perf/util/parse-events.c | 102 ++++++++++++++++++++++-----------
> tools/perf/util/parse-events.h | 2 +
> tools/perf/util/pmu.c | 36 ++++++------
> 3 files changed, 89 insertions(+), 51 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index db882f630f7e..4d42344698b8 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -182,6 +182,34 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)
>
> #define MAX_EVENT_LENGTH 512
>
> +void parse_events__append_error(struct parse_events_error *err, int idx,
> + char *str, char *help)
> +{
> + char *new_str = NULL;
> +
> + WARN(!str, "WARNING: failed to provide error string");

should we also bail out if str is NULL?

> + if (err->str) {
> + int ret;
> +
> + if (err->help)
> + ret = asprintf(&new_str,
> + "%s (previous error: %s(help: %s))",
> + str, err->str, err->help);
> + else

please use {} for multiline condition legs

> + ret = asprintf(&new_str,
> + "%s (previous error: %s)",
> + str, err->str);

does this actualy happen? could you please provide output
of this in the changelog?

> + if (ret < 0)
> + new_str = NULL;
> + else
> + zfree(&str);
> + }
> + err->idx = idx;
> + free(err->str);
> + err->str = new_str ?: str;
> + free(err->help);
> + err->help = help;
> +}
>

SNIP

Jiri Olsa

unread,
Oct 23, 2019, 4:40:49 AM10/23/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Tue, Oct 22, 2019 at 05:53:30PM -0700, Ian Rogers wrote:
> If event parsing fails the event list is leaked, instead splice the list
> onto the out result and let the caller cleanup.
>
> Signed-off-by: Ian Rogers <iro...@google.com>
> ---
> tools/perf/util/parse-events.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 4d42344698b8..a8f8801bd127 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1962,15 +1962,20 @@ int parse_events(struct evlist *evlist, const char *str,
>
> ret = parse_events__scanner(str, &parse_state, PE_START_EVENTS);
> perf_pmu__parse_cleanup();
> +

I dont understand.. is there something on the list in case we fail?

> + if (list_empty(&parse_state.list)) {
> + WARN_ONCE(true, "WARNING: event parser found nothing\n");
> + return -1;
> + }

this will display extra warning message for fail case:

[jolsa@krava perf]$ ./perf record -e krava ls
WARNING: event parser found nothing
event syntax error: 'krava'
\___ parser error

we don't want that

jirka

Jiri Olsa

unread,
Oct 23, 2019, 4:50:58 AM10/23/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Tue, Oct 22, 2019 at 05:53:31PM -0700, Ian Rogers wrote:

SNIP

> return -1;
> + }
> list_add_tail(&term->list, head);
>
> if (!parse_events_add_pmu(parse_state, list,
> pmu->name, head,
> true, true)) {
> - pr_debug("%s -> %s/%s/\n", str,
> + pr_debug("%s -> %s/%s/\n", config,
> pmu->name, alias->str);
> ok++;
> }
> @@ -1462,8 +1472,10 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
> }
> }
> }
> - if (!ok)
> + if (!ok) {
> + free(list);
> return -1;
> + }
> *listp = list;
> return 0;
> }
> @@ -2761,13 +2773,13 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
> struct parse_events_term temp = {
> .type_val = PARSE_EVENTS__TERM_TYPE_STR,
> .type_term = PARSE_EVENTS__TERM_TYPE_USER,
> - .config = config ?: (char *) "event",
> + .config = config ?: strdup("event"),

there's no check if this succeeds

> };
>
> BUG_ON(idx >= PERF_COUNT_HW_MAX);
> sym = &event_symbols_hw[idx];
>
> - return new_term(term, &temp, (char *) sym->symbol, 0);
> + return new_term(term, &temp, strdup(sym->symbol), 0);
> }
>
> int parse_events_term__clone(struct parse_events_term **new,
> @@ -2776,12 +2788,15 @@ int parse_events_term__clone(struct parse_events_term **new,
> struct parse_events_term temp = {
> .type_val = term->type_val,
> .type_term = term->type_term,
> - .config = term->config,
> + .config = term->config ? strdup(term->config) : NULL,

ditto

also how is this released when term is freed?

thanks,
jirka

Jiri Olsa

unread,
Oct 23, 2019, 4:56:10 AM10/23/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Tue, Oct 22, 2019 at 05:53:32PM -0700, Ian Rogers wrote:
> Having a YYABORT in a macro makes it hard to free memory for components
> of a rule. Separate the logic out.

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka

>

Jiri Olsa

unread,
Oct 23, 2019, 4:58:42 AM10/23/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Tue, Oct 22, 2019 at 05:53:33PM -0700, Ian Rogers wrote:
> Use realloc rather than malloc+memcpy to possibly avoid a memory
> allocation when appending array elements.
>
> Signed-off-by: Ian Rogers <iro...@google.com>

Acked-by: Jiri Olsa <jo...@kernel.org>

thanks,
jirka

Jiri Olsa

unread,
Oct 23, 2019, 5:01:51 AM10/23/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Tue, Oct 22, 2019 at 05:53:34PM -0700, Ian Rogers wrote:
> If parsing fails then destructors are ran to clean the up the stack.
> Rename the head union member to make the term and evlist use cases more
> distinct, this simplifies matching the correct destructor.

I'm getting compilation fail:

CC util/parse-events-bison.o
util/parse-events.y: In function ‘yydestruct’:
util/parse-events.y:125:45: error: ‘struct tracepoint_name’ has no member named ‘sys’; did you mean ‘sys1’?
125 | %destructor { free ($$.sys); free ($$.event); } <tracepoint_name>

jirka

Arnaldo Carvalho de Melo

unread,
Oct 23, 2019, 8:37:36 AM10/23/19
to Jiri Olsa, Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
Em Wed, Oct 23, 2019 at 10:55:59AM +0200, Jiri Olsa escreveu:
> On Tue, Oct 22, 2019 at 05:53:32PM -0700, Ian Rogers wrote:
> > Having a YYABORT in a macro makes it hard to free memory for components
> > of a rule. Separate the logic out.
>
> Acked-by: Jiri Olsa <jo...@kernel.org>

Thanks, applied.

- Arnaldo

Arnaldo Carvalho de Melo

unread,
Oct 23, 2019, 8:38:05 AM10/23/19
to Jiri Olsa, Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
Em Wed, Oct 23, 2019 at 10:58:30AM +0200, Jiri Olsa escreveu:
> On Tue, Oct 22, 2019 at 05:53:33PM -0700, Ian Rogers wrote:
> > Use realloc rather than malloc+memcpy to possibly avoid a memory
> > allocation when appending array elements.
> >
> > Signed-off-by: Ian Rogers <iro...@google.com>
>
> Acked-by: Jiri Olsa <jo...@kernel.org>

Thanks, applied,

- Arnaldo

Ian Rogers

unread,
Oct 24, 2019, 2:49:21 PM10/24/19
to Jiri Olsa, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, LKML, net...@vger.kernel.org, b...@vger.kernel.org, clang-built-linux, Stephane Eranian

Sorry, the intent here is that patch v2 be used in preference to the 1st patch, it looks like you've applied both. The first patch split apart tracepoint_name to avoid accessing out of scope stack memory, the second patch allocates heap memory that is correctly destructed (and consequently needs 1 fewer struct tracepoint_name member). Please disregard the 1st patch and just apply the second series.

Thanks,
Ian

Ian Rogers

unread,
Oct 24, 2019, 3:02:09 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
The parse events parser leaks memory for certain expressions as well
as allowing a char* to reference stack, heap or .rodata. This series
of patches improves the hygeine and adds free-ing operations to
reclaim memory in the parser in error and non-error situations.

The series of patches was generated with LLVM's address sanitizer and
libFuzzer:
https://llvm.org/docs/LibFuzzer.html
called on the parse_events function with randomly generated input. With
the patches no leaks or memory corruption issues were present.

The v3 patches address review comments from Jiri Olsa improving commit
messages, handling ENOMEM errors from strdup better, and removing a
printed warning if an invalid event is passed.

The v2 patches are preferable to an earlier proposed patch:
perf tools: avoid reading out of scope array

Ian Rogers (9):
perf tools: add parse events append error
perf tools: splice events onto evlist even on error
perf tools: ensure config and str in terms are unique
perf tools: move ALLOC_LIST into a function
perf tools: avoid a malloc for array events
perf tools: add destructors for parse event terms
perf tools: before yyabort-ing free components
perf tools: if pmu configuration fails free terms
perf tools: add a deep delete for parse event terms

tools/perf/util/parse-events.c | 193 +++++++++++-----
tools/perf/util/parse-events.h | 3 +
tools/perf/util/parse-events.y | 388 ++++++++++++++++++++++++---------
tools/perf/util/pmu.c | 32 +--
4 files changed, 449 insertions(+), 167 deletions(-)

--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 24, 2019, 3:02:11 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Parse event error handling may overwrite one error string with another
creating memory leaks and masking errors. Introduce a helper routine
that appends error messages and avoids the memory leak.

A reproduction of this problem can be seen with:
perf stat -e c/c/
After this change this produces:
event syntax error: 'c/c/'
\___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))

valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore
Run 'perf list' for a list of valid events

Usage: perf stat [<options>] [<command>]

-e, --event <event> event selector. use 'perf list' to list available events

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 100 +++++++++++++++++++++++----------
tools/perf/util/parse-events.h | 2 +
tools/perf/util/pmu.c | 30 ++++++----
3 files changed, 89 insertions(+), 43 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index db882f630f7e..edb3ae76777d 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -182,6 +182,38 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)

#define MAX_EVENT_LENGTH 512

+void parse_events__append_error(struct parse_events_error *err, int idx,
+ char *str, char *help)
+{
+ char *new_str = NULL;
+
+ if (WARN(!str, "WARNING: failed to provide error string")) {
+ free(help);
+ return;
+ }
+ if (err->str) {
+ int ret;
+
+ if (err->help) {
+ ret = asprintf(&new_str,
+ "%s (previous error: %s(help: %s))",
+ str, err->str, err->help);
+ } else {
+ ret = asprintf(&new_str,
+ "%s (previous error: %s)",
+ str, err->str);
+ }
+ if (ret < 0)
+ new_str = NULL;
+ else
+ zfree(&str);
+ }
+ err->idx = idx;
+ free(err->str);
+ err->str = new_str ?: str;
+ free(err->help);
+ err->help = help;
+}

struct tracepoint_path *tracepoint_id_to_path(u64 config)
{
@@ -932,11 +964,11 @@ static int check_type_val(struct parse_events_term *term,
return 0;

if (err) {
- err->idx = term->err_val;
- if (type == PARSE_EVENTS__TERM_TYPE_NUM)
- err->str = strdup("expected numeric value");
- else
- err->str = strdup("expected string value");
+ parse_events__append_error(err, term->err_val,
+ type == PARSE_EVENTS__TERM_TYPE_NUM
+ ? strdup("expected numeric value")
+ : strdup("expected string value"),
+ NULL);
}
return -EINVAL;
}
@@ -972,8 +1004,11 @@ static bool config_term_shrinked;
static bool
config_term_avail(int term_type, struct parse_events_error *err)
{
+ char *err_str;
+
if (term_type < 0 || term_type >= __PARSE_EVENTS__TERM_TYPE_NR) {
- err->str = strdup("Invalid term_type");
+ parse_events__append_error(err, -1,
+ strdup("Invalid term_type"), NULL);
return false;
}
if (!config_term_shrinked)
@@ -992,9 +1027,9 @@ config_term_avail(int term_type, struct parse_events_error *err)
return false;

/* term_type is validated so indexing is safe */
- if (asprintf(&err->str, "'%s' is not usable in 'perf stat'",
- config_term_names[term_type]) < 0)
- err->str = NULL;
+ if (asprintf(&err_str, "'%s' is not usable in 'perf stat'",
+ config_term_names[term_type]) >= 0)
+ parse_events__append_error(err, -1, err_str, NULL);
return false;
}
}
@@ -1036,17 +1071,20 @@ do { \
case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
CHECK_TYPE_VAL(STR);
if (strcmp(term->val.str, "no") &&
- parse_branch_str(term->val.str, &attr->branch_sample_type)) {
- err->str = strdup("invalid branch sample type");
- err->idx = term->err_val;
+ parse_branch_str(term->val.str,
+ &attr->branch_sample_type)) {
+ parse_events__append_error(err, term->err_val,
+ strdup("invalid branch sample type"),
+ NULL);
return -EINVAL;
}
break;
case PARSE_EVENTS__TERM_TYPE_TIME:
CHECK_TYPE_VAL(NUM);
if (term->val.num > 1) {
- err->str = strdup("expected 0 or 1");
- err->idx = term->err_val;
+ parse_events__append_error(err, term->err_val,
+ strdup("expected 0 or 1"),
+ NULL);
return -EINVAL;
}
break;
@@ -1080,8 +1118,9 @@ do { \
case PARSE_EVENTS__TERM_TYPE_PERCORE:
CHECK_TYPE_VAL(NUM);
if ((unsigned int)term->val.num > 1) {
- err->str = strdup("expected 0 or 1");
- err->idx = term->err_val;
+ parse_events__append_error(err, term->err_val,
+ strdup("expected 0 or 1"),
+ NULL);
return -EINVAL;
}
break;
@@ -1089,9 +1128,9 @@ do { \
CHECK_TYPE_VAL(NUM);
break;
default:
- err->str = strdup("unknown term");
- err->idx = term->err_term;
- err->help = parse_events_formats_error_string(NULL);
+ parse_events__append_error(err, term->err_term,
+ strdup("unknown term"),
+ parse_events_formats_error_string(NULL));
return -EINVAL;
}

@@ -1142,9 +1181,9 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
return config_term_common(attr, term, err);
default:
if (err) {
- err->idx = term->err_term;
- err->str = strdup("unknown term");
- err->help = strdup("valid terms: call-graph,stack-size\n");
+ parse_events__append_error(err, term->err_term,
+ strdup("unknown term"),
+ strdup("valid terms: call-graph,stack-size\n"));
}
return -EINVAL;
}
@@ -1323,10 +1362,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,

pmu = perf_pmu__find(name);
if (!pmu) {
- if (asprintf(&err->str,
+ char *err_str;
+
+ if (asprintf(&err_str,
"Cannot find PMU `%s'. Missing kernel support?",
- name) < 0)
- err->str = NULL;
+ name) >= 0)
+ parse_events__append_error(err, -1, err_str, NULL);
return -EINVAL;
}

@@ -2797,13 +2838,10 @@ void parse_events__clear_array(struct parse_events_array *a)
void parse_events_evlist_error(struct parse_events_state *parse_state,
int idx, const char *str)
{
- struct parse_events_error *err = parse_state->error;
-
- if (!err)
+ if (!parse_state->error)
return;
- err->idx = idx;
- err->str = strdup(str);
- WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
+
+ parse_events__append_error(parse_state->error, idx, strdup(str), NULL);
}

static void config_terms_list(char *buf, size_t buf_sz)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 769e07cddaa2..a7d42faeab0c 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -124,6 +124,8 @@ struct parse_events_state {
struct list_head *terms;
};

+void parse_events__append_error(struct parse_events_error *err, int idx,
+ char *str, char *help);
void parse_events__shrink_config_terms(void);
int parse_events__is_hardcoded_term(struct parse_events_term *term);
int parse_events_term__num(struct parse_events_term **term,
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index adbe97e941dd..4015ec11944a 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1050,9 +1050,9 @@ static int pmu_config_term(struct list_head *formats,
if (err) {
char *pmu_term = pmu_formats_string(formats);

- err->idx = term->err_term;
- err->str = strdup("unknown term");
- err->help = parse_events_formats_error_string(pmu_term);
+ parse_events__append_error(err, term->err_term,
+ strdup("unknown term"),
+ parse_events_formats_error_string(pmu_term));
free(pmu_term);
}
return -EINVAL;
@@ -1080,8 +1080,9 @@ static int pmu_config_term(struct list_head *formats,
if (term->no_value &&
bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) {
if (err) {
- err->idx = term->err_val;
- err->str = strdup("no value assigned for term");
+ parse_events__append_error(err, term->err_val,
+ strdup("no value assigned for term"),
+ NULL);
}
return -EINVAL;
}
@@ -1094,8 +1095,9 @@ static int pmu_config_term(struct list_head *formats,
term->config, term->val.str);
}
if (err) {
- err->idx = term->err_val;
- err->str = strdup("expected numeric value");
+ parse_events__append_error(err, term->err_val,
+ strdup("expected numeric value"),
+ NULL);
}
return -EINVAL;
}
@@ -1108,11 +1110,15 @@ static int pmu_config_term(struct list_head *formats,

Ian Rogers

unread,
Oct 24, 2019, 3:02:13 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
If event parsing fails the event list is leaked, instead splice the list
onto the out result and let the caller cleanup.

An example input for parse_events found by libFuzzer that reproduces
this memory leak is 'm{'.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index edb3ae76777d..f0d50f079d2f 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1968,15 +1968,20 @@ int parse_events(struct evlist *evlist, const char *str,

ret = parse_events__scanner(str, &parse_state, PE_START_EVENTS);
perf_pmu__parse_cleanup();
+
+ if (!ret && list_empty(&parse_state.list)) {
+ WARN_ONCE(true, "WARNING: event parser found nothing\n");
+ return -1;
+ }

Ian Rogers

unread,
Oct 24, 2019, 3:02:16 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Make it easier to release memory associated with parse event terms by
duplicating the string for the config name and ensuring the val string
is a duplicate.

Currently the parser may memory leak terms and this is addressed in a
later patch.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 51 ++++++++++++++++++++++++++++------
tools/perf/util/parse-events.y | 4 ++-
2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f0d50f079d2f..dc5862a663b5 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1430,7 +1430,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
char *str, struct list_head **listp)
{
- struct list_head *head;
struct parse_events_term *term;
struct list_head *list;
struct perf_pmu *pmu = NULL;
@@ -1447,19 +1446,30 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,

list_for_each_entry(alias, &pmu->aliases, list) {
if (!strcasecmp(alias->name, str)) {
+ struct list_head *head;
+ char *config;
+
head = malloc(sizeof(struct list_head));
if (!head)
return -1;
INIT_LIST_HEAD(head);
- if (parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
- str, 1, false, &str, NULL) < 0)
+ config = strdup(str);
+ if (!config)
+ return -1;
+ if (parse_events_term__num(&term,
+ PARSE_EVENTS__TERM_TYPE_USER,
+ config, 1, false, &config,
+ NULL) < 0) {
+ free(list);
+ free(config);
return -1;
+ }
list_add_tail(&term->list, head);

if (!parse_events_add_pmu(parse_state, list,
pmu->name, head,
true, true)) {
- pr_debug("%s -> %s/%s/\n", str,
+ pr_debug("%s -> %s/%s/\n", config,
pmu->name, alias->str);
ok++;
}
@@ -1468,8 +1478,10 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
}
}
}
- if (!ok)
+ if (!ok) {
+ free(list);
return -1;
+ }
*listp = list;
return 0;
}
@@ -2764,30 +2776,51 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
char *config, unsigned idx)
{
struct event_symbol *sym;
+ char *str;
struct parse_events_term temp = {
.type_val = PARSE_EVENTS__TERM_TYPE_STR,
.type_term = PARSE_EVENTS__TERM_TYPE_USER,
- .config = config ?: (char *) "event",
+ .config = config,
};

+ if (!temp.config) {
+ temp.config = strdup("event");
+ if (!temp.config)
+ return -ENOMEM;
+ }
BUG_ON(idx >= PERF_COUNT_HW_MAX);
sym = &event_symbols_hw[idx];

- return new_term(term, &temp, (char *) sym->symbol, 0);
+ str = strdup(sym->symbol);
+ if (!str)
+ return -ENOMEM;
+ return new_term(term, &temp, str, 0);
}

int parse_events_term__clone(struct parse_events_term **new,
struct parse_events_term *term)
{
+ char *str;
struct parse_events_term temp = {
.type_val = term->type_val,
.type_term = term->type_term,
- .config = term->config,
+ .config = NULL,
.err_term = term->err_term,
.err_val = term->err_val,
};

- return new_term(new, &temp, term->val.str, term->val.num);
+ if (term->config) {
+ temp.config = strdup(term->config);
+ if (!temp.config)
+ return -ENOMEM;
+ }
+ if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
+ return new_term(new, &temp, NULL, term->val.num);
+
+ str = strdup(term->val.str);
+ if (!str)
+ return -ENOMEM;
+ return new_term(new, &temp, str, 0);
}

int parse_events_copy_term_list(struct list_head *old,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 48126ae4cd13..27d6b187c9b1 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -644,9 +644,11 @@ PE_NAME array '=' PE_VALUE
PE_DRV_CFG_TERM
{
struct parse_events_term *term;
+ char *config = strdup($1);

+ ABORT_ON(!config);
ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
- $1, $1, &@1, NULL));
+ config, $1, &@1, NULL));

Ian Rogers

unread,
Oct 24, 2019, 3:02:18 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Having a YYABORT in a macro makes it hard to free memory for components
of a rule. Separate the logic out.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 65 ++++++++++++++++++++++------------
1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 27d6b187c9b1..26cb65798522 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
struct perf_pmu *pmu = NULL;

Ian Rogers

unread,
Oct 24, 2019, 3:02:21 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Use realloc rather than malloc+memcpy to possibly avoid a memory
allocation when appending array elements.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 26cb65798522..545ab7cefc20 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y

Ian Rogers

unread,
Oct 24, 2019, 3:02:24 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
If parsing fails then destructors are ran to clean the up the stack.
Rename the head union member to make the term and evlist use cases more
distinct, this simplifies matching the correct destructor.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 69 +++++++++++++++++++++++-----------
1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 545ab7cefc20..4725b14b9db4 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -12,6 +12,7 @@
#include <stdio.h>
#include <linux/compiler.h>
#include <linux/types.h>
+#include <linux/zalloc.h>
#include "pmu.h"
#include "evsel.h"
#include "parse-events.h"
@@ -37,6 +38,25 @@ static struct list_head* alloc_list()
return list;
}

+static void free_list_evsel(struct list_head* list_evsel)
+{
+ struct perf_evsel *pos, *tmp;
+
+ list_for_each_entry_safe(pos, tmp, list_evsel, node) {
+ list_del_init(&pos->node);
+ perf_evsel__delete(pos);
+ }
+ free(list_evsel);
+}
+
+static void free_term(struct parse_events_term *term)
+{
+ if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
+ free(term->val.str);
+ zfree(&term->array.ranges);
+ free(term);
+}
+
static void inc_group_count(struct list_head *list,
struct parse_events_state *parse_state)
{
@@ -66,6 +86,7 @@ static void inc_group_count(struct list_head *list,
%type <num> PE_VALUE_SYM_TOOL
%type <num> PE_RAW
%type <num> PE_TERM
+%type <num> value_sym
%type <str> PE_NAME
%type <str> PE_BPF_OBJECT
%type <str> PE_BPF_SOURCE
@@ -76,37 +97,43 @@ static void inc_group_count(struct list_head *list,
%type <str> PE_EVENT_NAME
%type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT
%type <str> PE_DRV_CFG_TERM
-%type <num> value_sym
-%type <head> event_config
-%type <head> opt_event_config
-%type <head> opt_pmu_config
+%destructor { free ($$); } <str>
%type <term> event_term
struct parse_events_term *term;
struct tracepoint_name {
char *sys;
--
2.23.0.866.gb869b98d4c-goog

Ian Rogers

unread,
Oct 24, 2019, 3:02:26 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Yyabort doesn't destruct inputs and so this must be done manually before
using yyabort.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 250 +++++++++++++++++++++++++--------
1 file changed, 195 insertions(+), 55 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 4725b14b9db4..d1cceb3bc620 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -152,6 +152,7 @@ start_events: groups
{
struct parse_events_state *parse_state = _parse_state;

+ /* frees $1 */
parse_events_update_lists($1, &parse_state->list);
}

@@ -161,6 +162,7 @@ groups ',' group
struct list_head *list = $1;
struct list_head *group = $3;

+ /* frees $3 */
parse_events_update_lists(group, list);
$$ = list;
}
@@ -170,6 +172,7 @@ groups ',' event
struct list_head *list = $1;
struct list_head *event = $3;

+ /* frees $3 */
parse_events_update_lists(event, list);
$$ = list;
}
@@ -182,8 +185,14 @@ group:
group_def ':' PE_MODIFIER_EVENT
{
struct list_head *list = $1;
+ int err;

- ABORT_ON(parse_events__modifier_group(list, $3));
+ err = parse_events__modifier_group(list, $3);
+ free($3);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
- struct list_head *list, *orig_terms, *terms;
+ struct list_head *list = NULL, *orig_terms = NULL, *terms= NULL;
+ char *pattern = NULL;
+
+#define CLEANUP_YYABORT \
+ do { \
+ parse_events_terms__delete($2); \
+ parse_events_terms__delete(orig_terms); \
+ free($1); \
+ YYABORT; \
+ } while(0)

if (parse_events_copy_term_list($2, &orig_terms))
- YYABORT;
+ CLEANUP_YYABORT;

if (error)
error->idx = @1.first_column;

list = alloc_list();
- ABORT_ON(!list);
+ if (!list)
+ CLEANUP_YYABORT;
if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
struct perf_pmu *pmu = NULL;
int ok = 0;
@@ -338,11 +373,16 @@ value_sym '/' event_config '/'
struct list_head *list;
int type = $1 >> 16;
int config = $1 & 255;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_numeric(_parse_state, list, type, config, $3));
+ err = parse_events_add_numeric(_parse_state, list, type, config, $3);
parse_events_terms__delete($3);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -374,11 +414,19 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT '-' PE_NAME_CACHE_OP_RESULT opt_e
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, $3, $5, error, $6));
+ err = parse_events_add_cache(list, &parse_state->idx, $1, $3, $5, error, $6);
parse_events_terms__delete($6);
+ free($1);
+ free($3);
+ free($5);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -387,11 +435,18 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT opt_event_config
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, $3, NULL, error, $4));
+ err = parse_events_add_cache(list, &parse_state->idx, $1, $3, NULL, error, $4);
parse_events_terms__delete($4);
+ free($1);
+ free($3);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -400,11 +455,17 @@ PE_NAME_CACHE_TYPE opt_event_config
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, NULL, NULL, error, $2));
+ err = parse_events_add_cache(list, &parse_state->idx, $1, NULL, NULL, error, $2);
parse_events_terms__delete($2);
+ free($1);
+ if (err) {
+ free_list_evsel(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -413,11 +474,17 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
{
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
- (void *) $2, $6, $4));
+ err = parse_events_add_breakpoint(list, &parse_state->idx,
+ (void *) $2, $6, $4);
+ free($6);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -428,8 +495,11 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE sep_dc

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
- (void *) $2, NULL, $4));
+ if (parse_events_add_breakpoint(list, &parse_state->idx,
+ (void *) $2, NULL, $4)) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -437,11 +507,17 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
{
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
- (void *) $2, $4, 0));
+ err = parse_events_add_breakpoint(list, &parse_state->idx,
+ (void *) $2, $4, 0);
+ free($4);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}
|
@@ -452,8 +528,11 @@ PE_PREFIX_MEM PE_VALUE sep_dc

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
- (void *) $2, NULL, 0));
+ if (parse_events_add_breakpoint(list, &parse_state->idx,
+ (void *) $2, NULL, 0)) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -463,29 +542,35 @@ tracepoint_name opt_event_config
struct parse_events_state *parse_state = _parse_state;
struct parse_events_error *error = parse_state->error;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
if (error)
error->idx = @1.first_column;

- if (parse_events_add_tracepoint(list, &parse_state->idx, $1.sys, $1.event,
- error, $2))
- return -1;
+ err = parse_events_add_tracepoint(list, &parse_state->idx, $1.sys, $1.event,
+ error, $2);

+ parse_events_terms__delete($2);
+ free($1.sys);
+ free($1.event);
+ if (err) {
+ free(list);
+ return -1;
+ }
$$ = list;
}

tracepoint_name:
PE_NAME '-' PE_NAME ':' PE_NAME
{
- char sys_name[128];
struct tracepoint_name tracepoint;

- snprintf(&sys_name, 128, "%s-%s", $1, $3);
- tracepoint.sys = &sys_name;
+ ABORT_ON(asprintf(&tracepoint.sys, "%s-%s", $1, $3) < 0);
tracepoint.event = $5;
-
+ free($1);
+ free($3);
$$ = tracepoint;
}
|
@@ -500,11 +585,16 @@ event_legacy_numeric:
PE_VALUE ':' PE_VALUE opt_event_config
{
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_numeric(_parse_state, list, (u32)$1, $3, $4));
+ err = parse_events_add_numeric(_parse_state, list, (u32)$1, $3, $4);
parse_events_terms__delete($4);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -512,11 +602,16 @@ event_legacy_raw:
PE_RAW opt_event_config
{
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, $1, $2));
+ err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, $1, $2);
parse_events_terms__delete($2);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -525,22 +620,33 @@ PE_BPF_OBJECT opt_event_config
{
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_load_bpf(parse_state, list, $1, false, $2));
+ err = parse_events_load_bpf(parse_state, list, $1, false, $2);
parse_events_terms__delete($2);
+ free($1);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}
|
PE_BPF_SOURCE opt_event_config
{
struct list_head *list;
+ int err;

list = alloc_list();
ABORT_ON(!list);
- ABORT_ON(parse_events_load_bpf(_parse_state, list, $1, true, $2));
+ err = parse_events_load_bpf(_parse_state, list, $1, true, $2);
parse_events_terms__delete($2);
+ if (err) {
+ free(list);
+ YYABORT;
+ }
$$ = list;
}

@@ -573,6 +679,10 @@ opt_pmu_config:
start_terms: event_config
{
struct parse_events_state *parse_state = _parse_state;
+ if (parse_state->terms) {
+ parse_events_terms__delete ($1);
+ YYABORT;
+ }
parse_state->terms = $1;
}

@@ -582,7 +692,10 @@ event_config ',' event_term
struct list_head *head = $1;
struct parse_events_term *term = $3;

+ config, $1, &@1, NULL)) {
+ free($1);
+ YYABORT;
+ }

Ian Rogers

unread,
Oct 24, 2019, 3:02:30 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Avoid a memory leak when the configuration fails.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index dc5862a663b5..999ea7378969 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1406,8 +1406,15 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
if (get_config_terms(head_config, &config_terms))
return -ENOMEM;

- if (perf_pmu__config(pmu, &attr, head_config, parse_state->error))
+ if (perf_pmu__config(pmu, &attr, head_config, parse_state->error)) {
+ struct perf_evsel_config_term *pos, *tmp;
+

Ian Rogers

unread,
Oct 24, 2019, 3:02:32 PM10/24/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Add a parse_events_term deep delete function so that owned strings and
arrays are freed.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 16 +++++++++++++---
tools/perf/util/parse-events.h | 1 +
tools/perf/util/parse-events.y | 12 ++----------
tools/perf/util/pmu.c | 2 +-
4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 999ea7378969..58322cb3b5df 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2830,6 +2830,18 @@ int parse_events_term__clone(struct parse_events_term **new,
return new_term(new, &temp, str, 0);
}

+void parse_events_term__delete(struct parse_events_term *term)
+{
+ if (term->array.nr_ranges)
+ zfree(&term->array.ranges);
+
+ if (term->type_val != PARSE_EVENTS__TERM_TYPE_NUM)
+ zfree(&term->val.str);
+
+ zfree(&term->config);
+ free(term);
+}
+
int parse_events_copy_term_list(struct list_head *old,
struct list_head **new)
{
@@ -2860,10 +2872,8 @@ void parse_events_terms__purge(struct list_head *terms)
struct parse_events_term *term, *h;

list_for_each_entry_safe(term, h, terms, list) {
- if (term->array.nr_ranges)
- zfree(&term->array.ranges);
list_del_init(&term->list);
- free(term);
+ parse_events_term__delete(term);
}
}

diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index a7d42faeab0c..d1ade97e2357 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -139,6 +139,7 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
char *config, unsigned idx);
int parse_events_term__clone(struct parse_events_term **new,
struct parse_events_term *term);
+void parse_events_term__delete(struct parse_events_term *term);
void parse_events_terms__delete(struct list_head *terms);
void parse_events_terms__purge(struct list_head *terms);
void parse_events__clear_array(struct parse_events_array *a);
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d1cceb3bc620..649c63809bad 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -49,14 +49,6 @@ static void free_list_evsel(struct list_head* list_evsel)
free(list_evsel);
}

-static void free_term(struct parse_events_term *term)
-{
- if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
- free(term->val.str);
- zfree(&term->array.ranges);
- free(term);
-}
-
static void inc_group_count(struct list_head *list,
struct parse_events_state *parse_state)
{
@@ -99,7 +91,7 @@ static void inc_group_count(struct list_head *list,
%type <str> PE_DRV_CFG_TERM
%destructor { free ($$); } <str>
%type <term> event_term
-%destructor { free_term ($$); } <term>
+%destructor { parse_events_term__delete ($$); } <term>
%type <list_terms> event_config
%type <list_terms> opt_event_config
%type <list_terms> opt_pmu_config
@@ -693,7 +685,7 @@ event_config ',' event_term
struct parse_events_term *term = $3;

if (!head) {
- free_term(term);
+ parse_events_term__delete(term);
YYABORT;
}
list_add_tail(&term->list, head);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 4015ec11944a..53af92321693 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1260,7 +1260,7 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,

Ian Rogers

unread,
Oct 24, 2019, 3:03:38 PM10/24/19
to Jiri Olsa, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, LKML, net...@vger.kernel.org, b...@vger.kernel.org, clang-built-linux, Stephane Eranian
Sorry, the intent here is that patch v2 be used in preference to the
1st patch, it looks like you've applied both. The first patch split
apart tracepoint_name to avoid accessing out of scope stack memory,
the second patch allocates heap memory that is correctly destructed
(and consequently needs 1 fewer struct tracepoint_name member). Please
disregard the 1st patch and just apply the second series.

Thanks,
Ian


On Wed, Oct 23, 2019 at 2:01 AM Jiri Olsa <jo...@redhat.com> wrote:
>

Jiri Olsa

unread,
Oct 25, 2019, 3:58:32 AM10/25/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote:
> Parse event error handling may overwrite one error string with another
> creating memory leaks and masking errors. Introduce a helper routine
> that appends error messages and avoids the memory leak.
>
> A reproduction of this problem can be seen with:
> perf stat -e c/c/
> After this change this produces:
> event syntax error: 'c/c/'
> \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))


hum... I'd argue that the previous state was better:

[jolsa@krava perf]$ ./perf stat -e c/c/
event syntax error: 'c/c/'
\___ unknown term


jirka

Jiri Olsa

unread,
Oct 25, 2019, 4:01:54 AM10/25/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
I still dont understand this one.. if there was an error, the list
should be empty, right? also if there's an error and there's something
on the list, what is it? how it gets deleted?

thanks,
jirka

Jiri Olsa

unread,
Oct 25, 2019, 4:10:59 AM10/25/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Thu, Oct 24, 2019 at 12:01:56PM -0700, Ian Rogers wrote:
> Make it easier to release memory associated with parse event terms by
> duplicating the string for the config name and ensuring the val string
> is a duplicate.
>
> Currently the parser may memory leak terms and this is addressed in a
> later patch.

please move that patch before this one

thanks,
jirka

Jiri Olsa

unread,
Oct 25, 2019, 4:27:28 AM10/25/19
to Ian Rogers, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian
On Thu, Oct 24, 2019 at 12:01:59PM -0700, Ian Rogers wrote:
> If parsing fails then destructors are ran to clean the up the stack.
> Rename the head union member to make the term and evlist use cases more
> distinct, this simplifies matching the correct destructor.

nice did not know about this.. looks like it's been in bison for some time, right?
I think you need to iterate 'struct evsel' in here, not 'struct perf_evsel'

should be:

struct evsel *evsel, *tmp;

list_for_each_entry_safe(evsel, tmp, list_evsel, core.node) {
list_del_init(&evsel->core.node);
evsel__delete(evsel);
}

thanks,
jirka

Ian Rogers

unread,
Oct 25, 2019, 11:14:49 AM10/25/19
to Jiri Olsa, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, LKML, net...@vger.kernel.org, b...@vger.kernel.org, clang-built-linux, Stephane Eranian
On Fri, Oct 25, 2019 at 12:58 AM Jiri Olsa <jo...@redhat.com> wrote:
>
> On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote:
> > Parse event error handling may overwrite one error string with another
> > creating memory leaks and masking errors. Introduce a helper routine
> > that appends error messages and avoids the memory leak.
> >
> > A reproduction of this problem can be seen with:
> > perf stat -e c/c/
> > After this change this produces:
> > event syntax error: 'c/c/'
> > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))
>
>
> hum... I'd argue that the previous state was better:
>
> [jolsa@krava perf]$ ./perf stat -e c/c/
> event syntax error: 'c/c/'
> \___ unknown term
>
>
> jirka

I am agnostic. We can either have the previous state or the new state,
I'm keen to resolve the memory leak. Another alternative is to warn
that multiple errors have occurred before dropping or printing the
previous error. As the code is shared in memory places the approach
taken here was to try to not conceal anything that could potentially
be useful. Given this, is the preference to keep the status quo
without any warning?

Thanks,
Ian

Ian Rogers

unread,
Oct 25, 2019, 11:47:26 AM10/25/19
to Jiri Olsa, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, LKML, net...@vger.kernel.org, b...@vger.kernel.org, clang-built-linux, Stephane Eranian
What I see happening with PARSER_DEBUG for 'm{' is (I've tried to
manually tweak the line numbers to be consistent with the current
parse-events.y, sorry for any discrepancies):

Starting parse
Entering state 0
Reading a token: Next token is token PE_START_EVENTS (1.1: )
Shifting token PE_START_EVENTS (1.1: )
Entering state 1
Reading a token: Next token is token PE_EVENT_NAME (1.0: )
Shifting token PE_EVENT_NAME (1.0: )
Entering state 8
Reading a token: Next token is token PE_NAME (1.0: )
Shifting token PE_NAME (1.0: )
Entering state 46
Reading a token: Next token is token '{' (1.1: )
Reducing stack by rule 50 (line 510):
-> $$ = nterm opt_event_config (1.0: )
Stack now 0 1 8 46
Entering state 51
Reducing stack by rule 27 (line 229):
$1 = token PE_NAME (1.0: )
$2 = nterm opt_event_config (1.0: )
-> $$ = nterm event_pmu (1.0: )
Stack now 0 1 8
Entering state 25
Reducing stack by rule 19 (line 219):
$1 = nterm event_pmu (1.0: )
-> $$ = nterm event_def (1.0: )
Stack now 0 1 8
Entering state 47
Reducing stack by rule 17 (line 210):
$1 = token PE_EVENT_NAME (1.0: )
$2 = nterm event_def (1.0: )
-> $$ = nterm event_name (1.0: )
Stack now 0 1
Entering state 23
Next token is token '{' (1.1: )
Reducing stack by rule 16 (line 207):
$1 = nterm event_name (1.0: )
-> $$ = nterm event_mod (1.0: )
Stack now 0 1
Entering state 22
Reducing stack by rule 14 (line 191):
$1 = nterm event_mod (1.0: )
-> $$ = nterm event (1.0: )
Stack now 0 1
Entering state 21
Reducing stack by rule 7 (line 147):
$1 = nterm event (1.0: )
-> $$ = nterm groups (1.0: )
Stack now 0 1
Entering state 18
Next token is token '{' (1.1: )
Reducing stack by rule 3 (line 119):
$1 = nterm groups (1.0: )
-> $$ = nterm start_events (1.0: )
Stack now 0 1
Entering state 17
Reducing stack by rule 1 (line 115):
$1 = token PE_START_EVENTS (1.1: )
$2 = nterm start_events (1.0: )
-> $$ = nterm start (1.1: )
Stack now 0
Entering state 3
Next token is token '{' (1.1: )
Error: popping nterm start (1.1: )
Stack now 0
Cleanup: discarding lookahead token '{' (1.1: )
Stack now 0

Working backward through this we're going:
start: PE_START_EVENTS start_events
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L115

start_events: groups
{
struct parse_events_state *parse_state = _parse_state;
parse_events_update_lists($1, &parse_state->list); // <--- where list
gets onto the state
}
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L119

groups: event
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L147

event: event_mod
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L191

event_mod: event_name
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L207

event_name: PE_EVENT_NAME event_def
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L210

event_def: event_pmu
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L219

event_pmu: PE_NAME opt_event_config
{
...
ALLOC_LIST(list); // <--- where list gets allocated
...
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L229

opt_event_config:
https://github.com/torvalds/linux/blob/master/tools/perf/util/parse-events.y#L510

So the parse_state is ending up with a list, however, parsing is
failing. If the list isn't adding to the evlist then it becomes a
leak. Splicing it onto the evlist allows the caller to clean this up
and avoids the leak. An alternate approach is to free the failed list
and not get the caller to clean up. A way to do this is to create an
evlist, splice the failed list onto it and then free it - which winds
up being fairly identical to this approach, and this approach is a
smaller change.

Thanks,
Ian

Ian Rogers

unread,
Oct 25, 2019, 11:52:21 AM10/25/19
to Jiri Olsa, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, LKML, net...@vger.kernel.org, b...@vger.kernel.org, clang-built-linux, Stephane Eranian
On Fri, Oct 25, 2019 at 1:10 AM Jiri Olsa <jo...@redhat.com> wrote:
>
> On Thu, Oct 24, 2019 at 12:01:56PM -0700, Ian Rogers wrote:
> > Make it easier to release memory associated with parse event terms by
> > duplicating the string for the config name and ensuring the val string
> > is a duplicate.
> >
> > Currently the parser may memory leak terms and this is addressed in a
> > later patch.
>
> please move that patch before this one

Doing that causes a use after free, or freeing of stack or .rodata.
The strings need to be on the heap before they can be cleaned up,
hence the order the patches appear here. There are already memory
leaks here and so while this does make more of them, it is a temporary
state until the later freeing patch is added. I wanted to avoid a
large monolithic patch.

Thanks,
Ian

Ian Rogers

unread,
Oct 25, 2019, 12:08:47 PM10/25/19
to Jiri Olsa, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, LKML, net...@vger.kernel.org, b...@vger.kernel.org, clang-built-linux, Stephane Eranian
On Fri, Oct 25, 2019 at 1:27 AM Jiri Olsa <jo...@redhat.com> wrote:
>
> On Thu, Oct 24, 2019 at 12:01:59PM -0700, Ian Rogers wrote:
> > If parsing fails then destructors are ran to clean the up the stack.
> > Rename the head union member to make the term and evlist use cases more
> > distinct, this simplifies matching the correct destructor.
>
> nice did not know about this.. looks like it's been in bison for some time, right?

Looks like it wasn't in Bison 1 but in Bison 2, we're at Bison 3 and
Bison 2 is > 14 years old:
https://web.archive.org/web/20050924004158/http://www.gnu.org/software/bison/manual/html_mono/bison.html#Destructor-Decl
Thanks, I'll address this.

Ian

Ian Rogers

unread,
Oct 25, 2019, 2:08:36 PM10/25/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
The parse events parser leaks memory for certain expressions as well
as allowing a char* to reference stack, heap or .rodata. This series
of patches improves the hygeine and adds free-ing operations to
reclaim memory in the parser in error and non-error situations.

The series of patches was generated with LLVM's address sanitizer and
libFuzzer:
https://llvm.org/docs/LibFuzzer.html
called on the parse_events function with randomly generated input. With
the patches no leaks or memory corruption issues were present.

The v4 patches address review comments from Jiri Olsa, turning a long
error message into a single warning, fixing the data type in a list
iterator and reordering patches.

The v3 patches address review comments from Jiri Olsa improving commit
messages, handling ENOMEM errors from strdup better, and removing a
printed warning if an invalid event is passed.

The v2 patches are preferable to an earlier proposed patch:
perf tools: avoid reading out of scope array

Ian Rogers (9):
perf tools: add parse events handle error
perf tools: move ALLOC_LIST into a function
perf tools: avoid a malloc for array events
perf tools: splice events onto evlist even on error
perf tools: ensure config and str in terms are unique
perf tools: add destructors for parse event terms
perf tools: before yyabort-ing free components
perf tools: if pmu configuration fails free terms
perf tools: add a deep delete for parse event terms

tools/perf/util/parse-events.c | 177 ++++++++++-----
tools/perf/util/parse-events.h | 3 +
tools/perf/util/parse-events.y | 388 ++++++++++++++++++++++++---------
tools/perf/util/pmu.c | 32 +--
4 files changed, 433 insertions(+), 167 deletions(-)

--
2.24.0.rc0.303.g954a862665-goog

Ian Rogers

unread,
Oct 25, 2019, 2:08:39 PM10/25/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Parse event error handling may overwrite one error string with another
creating memory leaks. Introduce a helper routine that warns about
multiple error messages as well as avoiding the memory leak.

A reproduction of this problem can be seen with:
perf stat -e c/c/
After this change this produces:
WARNING: multiple event parsing errors
event syntax error: 'c/c/'
\___ unknown term

valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore
Run 'perf list' for a list of valid events

Usage: perf stat [<options>] [<command>]

-e, --event <event> event selector. use 'perf list' to list available events

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 84 +++++++++++++++++++++-------------
tools/perf/util/parse-events.h | 2 +
tools/perf/util/pmu.c | 30 +++++++-----
3 files changed, 73 insertions(+), 43 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index db882f630f7e..c516d0cce946 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -182,6 +182,22 @@ static int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)

#define MAX_EVENT_LENGTH 512

+void parse_events__handle_error(struct parse_events_error *err, int idx,
+ char *str, char *help)
+{
+ char *new_str = NULL;
+
+ if (WARN(!str, "WARNING: failed to provide error string\n")) {
+ free(help);
+ return;
+ }
+ WARN_ONCE(err->str, "WARNING: multiple event parsing errors\n");
+ err->idx = idx;
+ free(err->str);
+ err->str = new_str ?: str;
+ free(err->help);
+ err->help = help;
+}

struct tracepoint_path *tracepoint_id_to_path(u64 config)
{
@@ -932,11 +948,11 @@ static int check_type_val(struct parse_events_term *term,
return 0;

if (err) {
- err->idx = term->err_val;
- if (type == PARSE_EVENTS__TERM_TYPE_NUM)
- err->str = strdup("expected numeric value");
- else
- err->str = strdup("expected string value");
+ parse_events__handle_error(err, term->err_val,
+ type == PARSE_EVENTS__TERM_TYPE_NUM
+ ? strdup("expected numeric value")
+ : strdup("expected string value"),
+ NULL);
}
return -EINVAL;
}
@@ -972,8 +988,11 @@ static bool config_term_shrinked;
static bool
config_term_avail(int term_type, struct parse_events_error *err)
{
+ char *err_str;
+
if (term_type < 0 || term_type >= __PARSE_EVENTS__TERM_TYPE_NR) {
- err->str = strdup("Invalid term_type");
+ parse_events__handle_error(err, -1,
+ strdup("Invalid term_type"), NULL);
return false;
}
if (!config_term_shrinked)
@@ -992,9 +1011,9 @@ config_term_avail(int term_type, struct parse_events_error *err)
return false;

/* term_type is validated so indexing is safe */
- if (asprintf(&err->str, "'%s' is not usable in 'perf stat'",
- config_term_names[term_type]) < 0)
- err->str = NULL;
+ if (asprintf(&err_str, "'%s' is not usable in 'perf stat'",
+ config_term_names[term_type]) >= 0)
+ parse_events__handle_error(err, -1, err_str, NULL);
return false;
}
}
@@ -1036,17 +1055,20 @@ do { \
case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
CHECK_TYPE_VAL(STR);
if (strcmp(term->val.str, "no") &&
- parse_branch_str(term->val.str, &attr->branch_sample_type)) {
- err->str = strdup("invalid branch sample type");
- err->idx = term->err_val;
+ parse_branch_str(term->val.str,
+ &attr->branch_sample_type)) {
+ parse_events__handle_error(err, term->err_val,
+ strdup("invalid branch sample type"),
+ NULL);
return -EINVAL;
}
break;
case PARSE_EVENTS__TERM_TYPE_TIME:
CHECK_TYPE_VAL(NUM);
if (term->val.num > 1) {
- err->str = strdup("expected 0 or 1");
- err->idx = term->err_val;
+ parse_events__handle_error(err, term->err_val,
+ strdup("expected 0 or 1"),
+ NULL);
return -EINVAL;
}
break;
@@ -1080,8 +1102,9 @@ do { \
case PARSE_EVENTS__TERM_TYPE_PERCORE:
CHECK_TYPE_VAL(NUM);
if ((unsigned int)term->val.num > 1) {
- err->str = strdup("expected 0 or 1");
- err->idx = term->err_val;
+ parse_events__handle_error(err, term->err_val,
+ strdup("expected 0 or 1"),
+ NULL);
return -EINVAL;
}
break;
@@ -1089,9 +1112,9 @@ do { \
CHECK_TYPE_VAL(NUM);
break;
default:
- err->str = strdup("unknown term");
- err->idx = term->err_term;
- err->help = parse_events_formats_error_string(NULL);
+ parse_events__handle_error(err, term->err_term,
+ strdup("unknown term"),
+ parse_events_formats_error_string(NULL));
return -EINVAL;
}

@@ -1142,9 +1165,9 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
return config_term_common(attr, term, err);
default:
if (err) {
- err->idx = term->err_term;
- err->str = strdup("unknown term");
- err->help = strdup("valid terms: call-graph,stack-size\n");
+ parse_events__handle_error(err, term->err_term,
+ strdup("unknown term"),
+ strdup("valid terms: call-graph,stack-size\n"));
}
return -EINVAL;
}
@@ -1323,10 +1346,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,

pmu = perf_pmu__find(name);
if (!pmu) {
- if (asprintf(&err->str,
+ char *err_str;
+
+ if (asprintf(&err_str,
"Cannot find PMU `%s'. Missing kernel support?",
- name) < 0)
- err->str = NULL;
+ name) >= 0)
+ parse_events__handle_error(err, -1, err_str, NULL);
return -EINVAL;
}

@@ -2797,13 +2822,10 @@ void parse_events__clear_array(struct parse_events_array *a)
void parse_events_evlist_error(struct parse_events_state *parse_state,
int idx, const char *str)
{
- struct parse_events_error *err = parse_state->error;
-
- if (!err)
+ if (!parse_state->error)
return;
- err->idx = idx;
- err->str = strdup(str);
- WARN_ONCE(!err->str, "WARNING: failed to allocate error string");
+
+ parse_events__handle_error(parse_state->error, idx, strdup(str), NULL);
}

static void config_terms_list(char *buf, size_t buf_sz)
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 769e07cddaa2..34f58d24a06a 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -124,6 +124,8 @@ struct parse_events_state {
struct list_head *terms;
};

+void parse_events__handle_error(struct parse_events_error *err, int idx,
+ char *str, char *help);
void parse_events__shrink_config_terms(void);
int parse_events__is_hardcoded_term(struct parse_events_term *term);
int parse_events_term__num(struct parse_events_term **term,
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index adbe97e941dd..f9f427d4c313 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1050,9 +1050,9 @@ static int pmu_config_term(struct list_head *formats,
if (err) {
char *pmu_term = pmu_formats_string(formats);

- err->idx = term->err_term;
- err->str = strdup("unknown term");
- err->help = parse_events_formats_error_string(pmu_term);
+ parse_events__handle_error(err, term->err_term,
+ strdup("unknown term"),
+ parse_events_formats_error_string(pmu_term));
free(pmu_term);
}
return -EINVAL;
@@ -1080,8 +1080,9 @@ static int pmu_config_term(struct list_head *formats,
if (term->no_value &&
bitmap_weight(format->bits, PERF_PMU_FORMAT_BITS) > 1) {
if (err) {
- err->idx = term->err_val;
- err->str = strdup("no value assigned for term");
+ parse_events__handle_error(err, term->err_val,
+ strdup("no value assigned for term"),
+ NULL);
}
return -EINVAL;
}
@@ -1094,8 +1095,9 @@ static int pmu_config_term(struct list_head *formats,
term->config, term->val.str);
}
if (err) {
- err->idx = term->err_val;
- err->str = strdup("expected numeric value");
+ parse_events__handle_error(err, term->err_val,
+ strdup("expected numeric value"),
+ NULL);
}
return -EINVAL;
}
@@ -1108,11 +1110,15 @@ static int pmu_config_term(struct list_head *formats,
max_val = pmu_format_max_value(format->bits);
if (val > max_val) {
if (err) {
- err->idx = term->err_val;
- if (asprintf(&err->str,
- "value too big for format, maximum is %llu",
- (unsigned long long)max_val) < 0)
- err->str = strdup("value too big for format");
+ char *err_str;
+
+ parse_events__handle_error(err, term->err_val,
+ asprintf(&err_str,
+ "value too big for format, maximum is %llu",
+ (unsigned long long)max_val) < 0
+ ? strdup("value too big for format")
+ : err_str,
+ NULL);
return -EINVAL;
}
/*
--
2.24.0.rc0.303.g954a862665-goog

Ian Rogers

unread,
Oct 25, 2019, 2:08:42 PM10/25/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Having a YYABORT in a macro makes it hard to free memory for components
of a rule. Separate the logic out.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 65 ++++++++++++++++++++++------------
1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 48126ae4cd13..5863acb34780 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -25,12 +25,17 @@ do { \
YYABORT; \
} while (0)

-#define ALLOC_LIST(list) \
-do { \
- list = malloc(sizeof(*list)); \
- ABORT_ON(!list); \
- INIT_LIST_HEAD(list); \
-} while (0)
+static struct list_head* alloc_list()
+{
+ struct list_head *list;
+
+ list = malloc(sizeof(*list));
+ if (!list)
+ return NULL;
+
+ INIT_LIST_HEAD(list);
+ return list;
+}

static void inc_group_count(struct list_head *list,
struct parse_events_state *parse_state)
@@ -238,7 +243,8 @@ PE_NAME opt_pmu_config
if (error)
error->idx = @1.first_column;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
struct perf_pmu *pmu = NULL;
int ok = 0;
@@ -306,7 +312,8 @@ value_sym '/' event_config '/'
int type = $1 >> 16;
int config = $1 & 255;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_numeric(_parse_state, list, type, config, $3));
parse_events_terms__delete($3);
$$ = list;
@@ -318,7 +325,8 @@ value_sym sep_slash_slash_dc
int type = $1 >> 16;
int config = $1 & 255;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_numeric(_parse_state, list, type, config, NULL));
$$ = list;
}
@@ -327,7 +335,8 @@ PE_VALUE_SYM_TOOL sep_slash_slash_dc
{
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_tool(_parse_state, list, $1));
$$ = list;
}
@@ -339,7 +348,8 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT '-' PE_NAME_CACHE_OP_RESULT opt_e
struct parse_events_error *error = parse_state->error;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, $3, $5, error, $6));
parse_events_terms__delete($6);
$$ = list;
@@ -351,7 +361,8 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT opt_event_config
struct parse_events_error *error = parse_state->error;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, $3, NULL, error, $4));
parse_events_terms__delete($4);
$$ = list;
@@ -363,7 +374,8 @@ PE_NAME_CACHE_TYPE opt_event_config
struct parse_events_error *error = parse_state->error;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_cache(list, &parse_state->idx, $1, NULL, NULL, error, $2));
parse_events_terms__delete($2);
$$ = list;
@@ -375,7 +387,8 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
(void *) $2, $6, $4));
$$ = list;
@@ -386,7 +399,8 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE sep_dc
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
(void *) $2, NULL, $4));
$$ = list;
@@ -397,7 +411,8 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
(void *) $2, $4, 0));
$$ = list;
@@ -408,7 +423,8 @@ PE_PREFIX_MEM PE_VALUE sep_dc
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_breakpoint(list, &parse_state->idx,
(void *) $2, NULL, 0));
$$ = list;
@@ -421,7 +437,8 @@ tracepoint_name opt_event_config
struct parse_events_error *error = parse_state->error;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
if (error)
error->idx = @1.first_column;

@@ -457,7 +474,8 @@ PE_VALUE ':' PE_VALUE opt_event_config
{
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_numeric(_parse_state, list, (u32)$1, $3, $4));
parse_events_terms__delete($4);
$$ = list;
@@ -468,7 +486,8 @@ PE_RAW opt_event_config
{
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, $1, $2));
parse_events_terms__delete($2);
$$ = list;
@@ -480,7 +499,8 @@ PE_BPF_OBJECT opt_event_config
struct parse_events_state *parse_state = _parse_state;
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_load_bpf(parse_state, list, $1, false, $2));
parse_events_terms__delete($2);
$$ = list;
@@ -490,7 +510,8 @@ PE_BPF_SOURCE opt_event_config
{
struct list_head *list;

- ALLOC_LIST(list);
+ list = alloc_list();
+ ABORT_ON(!list);
ABORT_ON(parse_events_load_bpf(_parse_state, list, $1, true, $2));
parse_events_terms__delete($2);
$$ = list;
--
2.24.0.rc0.303.g954a862665-goog

Ian Rogers

unread,
Oct 25, 2019, 2:08:45 PM10/25/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Use realloc rather than malloc+memcpy to possibly avoid a memory
allocation when appending array elements.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 5863acb34780..ffa1a1b63796 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -689,14 +689,12 @@ array_terms ',' array_term
struct parse_events_array new_array;

new_array.nr_ranges = $1.nr_ranges + $3.nr_ranges;
- new_array.ranges = malloc(sizeof(new_array.ranges[0]) *
- new_array.nr_ranges);
+ new_array.ranges = realloc($1.ranges,
+ sizeof(new_array.ranges[0]) *
+ new_array.nr_ranges);
ABORT_ON(!new_array.ranges);
- memcpy(&new_array.ranges[0], $1.ranges,
- $1.nr_ranges * sizeof(new_array.ranges[0]));
memcpy(&new_array.ranges[$1.nr_ranges], $3.ranges,
$3.nr_ranges * sizeof(new_array.ranges[0]));
- free($1.ranges);
free($3.ranges);
$$ = new_array;
}
--
2.24.0.rc0.303.g954a862665-goog

Ian Rogers

unread,
Oct 25, 2019, 2:08:47 PM10/25/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
If event parsing fails the event list is leaked, instead splice the list
onto the out result and let the caller cleanup.

An example input for parse_events found by libFuzzer that reproduces
this memory leak is 'm{'.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c516d0cce946..4c4c6f3e866a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1952,15 +1952,20 @@ int parse_events(struct evlist *evlist, const char *str,

ret = parse_events__scanner(str, &parse_state, PE_START_EVENTS);
perf_pmu__parse_cleanup();
+
+ if (!ret && list_empty(&parse_state.list)) {
+ WARN_ONCE(true, "WARNING: event parser found nothing\n");
+ return -1;
+ }
+
+ /*
+ * Add list to the evlist even with errors to allow callers to clean up.
+ */
+ perf_evlist__splice_list_tail(evlist, &parse_state.list);
+
if (!ret) {
struct evsel *last;

- if (list_empty(&parse_state.list)) {
- WARN_ONCE(true, "WARNING: event parser found nothing\n");
- return -1;
- }
-
- perf_evlist__splice_list_tail(evlist, &parse_state.list);
evlist->nr_groups += parse_state.nr_groups;
last = evlist__last(evlist);
last->cmdline_group_boundary = true;
--
2.24.0.rc0.303.g954a862665-goog

Ian Rogers

unread,
Oct 25, 2019, 2:08:50 PM10/25/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
Make it easier to release memory associated with parse event terms by
duplicating the string for the config name and ensuring the val string
is a duplicate.

Currently the parser may memory leak terms and this is addressed in a
later patch.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.c | 51 ++++++++++++++++++++++++++++------
tools/perf/util/parse-events.y | 4 ++-
2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 4c4c6f3e866a..fb6436a74869 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1414,7 +1414,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
char *str, struct list_head **listp)
{
- struct list_head *head;
struct parse_events_term *term;
struct list_head *list;
struct perf_pmu *pmu = NULL;
@@ -1431,19 +1430,30 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
@@ -1452,8 +1462,10 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
}
}
}
- if (!ok)
+ if (!ok) {
+ free(list);
return -1;
+ }
*listp = list;
return 0;
}
@@ -2748,30 +2760,51 @@ int parse_events_term__sym_hw(struct parse_events_term **term,
+ }
+ if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
+ return new_term(new, &temp, NULL, term->val.num);
+
+ str = strdup(term->val.str);
+ if (!str)
+ return -ENOMEM;
+ return new_term(new, &temp, str, 0);
}

int parse_events_copy_term_list(struct list_head *old,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index ffa1a1b63796..545ab7cefc20 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -665,9 +665,11 @@ PE_NAME array '=' PE_VALUE
PE_DRV_CFG_TERM
{
struct parse_events_term *term;
+ char *config = strdup($1);

+ ABORT_ON(!config);
ABORT_ON(parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG,
- $1, $1, &@1, NULL));
+ config, $1, &@1, NULL));
$$ = term;
}

--
2.24.0.rc0.303.g954a862665-goog

Ian Rogers

unread,
Oct 25, 2019, 2:08:52 PM10/25/19
to Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andi Kleen, Jin Yao, Adrian Hunter, Kan Liang, John Garry, linux-...@vger.kernel.org, net...@vger.kernel.org, b...@vger.kernel.org, clang-bu...@googlegroups.com, Stephane Eranian, Ian Rogers
If parsing fails then destructors are ran to clean the up the stack.
Rename the head union member to make the term and evlist use cases more
distinct, this simplifies matching the correct destructor.

Signed-off-by: Ian Rogers <iro...@google.com>
---
tools/perf/util/parse-events.y | 69 +++++++++++++++++++++++-----------
1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 545ab7cefc20..035edfa8d42e 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -12,6 +12,7 @@
#include <stdio.h>
#include <linux/compiler.h>
#include <linux/types.h>
+#include <linux/zalloc.h>
#include "pmu.h"
#include "evsel.h"
#include "parse-events.h"
@@ -37,6 +38,25 @@ static struct list_head* alloc_list()
return list;
}

+static void free_list_evsel(struct list_head* list_evsel)
+{
+ struct evsel *evsel, *tmp;
+
+ list_for_each_entry_safe(evsel, tmp, list_evsel, core.node) {
+ list_del_init(&evsel->core.node);
+ perf_evsel__delete(evsel);
+ }
+ free(list_evsel);
+}
+
+static void free_term(struct parse_events_term *term)
+{
+ if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
+ free(term->val.str);
+ zfree(&term->array.ranges);
+ free(term);
+}
+
static void inc_group_count(struct list_head *list,
struct parse_events_state *parse_state)
{
2.24.0.rc0.303.g954a862665-goog

Ian Rogers

unread,
Oct 25, 2019, 2:08:55 PM10/25/19