Re: [PATCH v3 2/9] perf tools: splice events onto evlist even on error

[Ian Rogers <irogers@google.com>]

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
> >
>