Re: [PATCH v3 3/9] perf tools: ensure config and str in terms are unique

From: Ian Rogers <irogers@google.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andi Kleen <ak@linux.intel.com>,
	Jin Yao <yao.jin@linux.intel.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	John Garry <john.garry@huawei.com>,
	LKML <linux-kernel@vger.kernel.org>,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	clang-built-linux <clang-built-linux@googlegroups.com>,
	Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH v3 3/9] perf tools: ensure config and str in terms are unique
Date: Fri, 25 Oct 2019 08:52:07 -0700	[thread overview]
Message-ID: <CAP-5=fW6PV5xYDyNViz_U9Y5Up8B30tUoyCuf_jM0XLj2ESQRA@mail.gmail.com> (raw)
In-Reply-To: <20191025081046.GG31679@krava>

On Fri, Oct 25, 2019 at 1:10 AM Jiri Olsa <jolsa@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

> thanks,
> jirka
>
> >
> > Signed-off-by: Ian Rogers <irogers@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));
> >       $$ = term;
> >  }
> >
> > --
> > 2.23.0.866.gb869b98d4c-goog
> >
>