| Message ID | 20191024190202.109403-3-irogers@google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Improvements to memory usage by parse events | expand |
On Thu, Oct 24, 2019 at 12:01:55PM -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. > > An example input for parse_events found by libFuzzer that reproduces > this memory leak is 'm{'. > > Signed-off-by: Ian Rogers <irogers@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; > + } > + > + /* > + * Add list to the evlist even with errors to allow callers to clean up. > + */ > + perf_evlist__splice_list_tail(evlist, &parse_state.list); 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 > + > 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 >
On Fri, Oct 25, 2019 at 1:01 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Oct 24, 2019 at 12:01:55PM -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. > > > > An example input for parse_events found by libFuzzer that reproduces > > this memory leak is 'm{'. > > > > Signed-off-by: Ian Rogers <irogers@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; > > + } > > + > > + /* > > + * Add list to the evlist even with errors to allow callers to clean up. > > + */ > > + perf_evlist__splice_list_tail(evlist, &parse_state.list); > > 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 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 > > + > > 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 > > >
On Fri, Oct 25, 2019 at 08:47:12AM -0700, Ian Rogers wrote: SNIP > 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. agreed, thanks for the all the details jirka
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; + } + + /* + * 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;
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 <irogers@google.com> --- tools/perf/util/parse-events.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)