linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf parse-events: fix memory leaks found on parse_events
@ 2020-03-16  4:14 Ian Rogers
  2020-03-18 10:40 ` Jiri Olsa
  0 siblings, 1 reply; 4+ messages in thread
From: Ian Rogers @ 2020-03-16  4:14 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Andi Kleen, Adrian Hunter, Leo Yan, linux-kernel,
	clang-built-linux
  Cc: Stephane Eranian, Ian Rogers

Memory leaks found by applying LLVM's libfuzzer on the parse_events
function.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/perf/evlist.c        | 2 ++
 tools/perf/util/parse-events.c | 2 ++
 tools/perf/util/parse-events.y | 3 ++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index 5b9f2ca50591..6485d1438f75 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -125,8 +125,10 @@ static void perf_evlist__purge(struct perf_evlist *evlist)
 void perf_evlist__exit(struct perf_evlist *evlist)
 {
 	perf_cpu_map__put(evlist->cpus);
+	perf_cpu_map__put(evlist->all_cpus);
 	perf_thread_map__put(evlist->threads);
 	evlist->cpus = NULL;
+	evlist->all_cpus = NULL;
 	evlist->threads = NULL;
 	fdarray__exit(&evlist->pollfd);
 }
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index a14995835d85..997862224292 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1482,6 +1482,8 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 
 		list_for_each_entry_safe(pos, tmp, &config_terms, list) {
 			list_del_init(&pos->list);
+			if (pos->free_str)
+				free(pos->val.str);
 			free(pos);
 		}
 		return -EINVAL;
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 94f8bcd83582..8212cc771667 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -44,7 +44,7 @@ static void free_list_evsel(struct list_head* list_evsel)
 
 	list_for_each_entry_safe(evsel, tmp, list_evsel, core.node) {
 		list_del_init(&evsel->core.node);
-		perf_evsel__delete(evsel);
+		evsel__delete(evsel);
 	}
 	free(list_evsel);
 }
@@ -326,6 +326,7 @@ PE_NAME opt_pmu_config
 	}
 	parse_events_terms__delete($2);
 	parse_events_terms__delete(orig_terms);
+	free(pattern);
 	free($1);
 	$$ = list;
 #undef CLEANUP_YYABORT
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] perf parse-events: fix memory leaks found on parse_events
  2020-03-16  4:14 [PATCH] perf parse-events: fix memory leaks found on parse_events Ian Rogers
@ 2020-03-18 10:40 ` Jiri Olsa
  2020-03-19  3:56   ` Ian Rogers
  0 siblings, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2020-03-18 10:40 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, Andi Kleen,
	Adrian Hunter, Leo Yan, linux-kernel, clang-built-linux,
	Stephane Eranian

On Sun, Mar 15, 2020 at 09:14:31PM -0700, Ian Rogers wrote:
> Memory leaks found by applying LLVM's libfuzzer on the parse_events
> function.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/perf/evlist.c        | 2 ++
>  tools/perf/util/parse-events.c | 2 ++
>  tools/perf/util/parse-events.y | 3 ++-
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index 5b9f2ca50591..6485d1438f75 100644
> --- a/tools/lib/perf/evlist.c
> +++ b/tools/lib/perf/evlist.c
> @@ -125,8 +125,10 @@ static void perf_evlist__purge(struct perf_evlist *evlist)
>  void perf_evlist__exit(struct perf_evlist *evlist)
>  {
>  	perf_cpu_map__put(evlist->cpus);
> +	perf_cpu_map__put(evlist->all_cpus);

ugh, yes, could you please put it to separate libperf patch?

>  	perf_thread_map__put(evlist->threads);
>  	evlist->cpus = NULL;
> +	evlist->all_cpus = NULL;

there's already change adding this waiting on the list:
  https://lore.kernel.org/lkml/1583665157-349023-1-git-send-email-zhe.he@windriver.com/

>  	evlist->threads = NULL;
>  	fdarray__exit(&evlist->pollfd);
>  }
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index a14995835d85..997862224292 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1482,6 +1482,8 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  
>  		list_for_each_entry_safe(pos, tmp, &config_terms, list) {
>  			list_del_init(&pos->list);
> +			if (pos->free_str)
> +				free(pos->val.str);

ack, would be nice to have  perf_evsel__free_config_terms generalized
to work directly over config terms list, so we'd have only single
cleanup function

>  			free(pos);
>  		}
>  		return -EINVAL;
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 94f8bcd83582..8212cc771667 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -44,7 +44,7 @@ static void free_list_evsel(struct list_head* list_evsel)
>  
>  	list_for_each_entry_safe(evsel, tmp, list_evsel, core.node) {
>  		list_del_init(&evsel->core.node);
> -		perf_evsel__delete(evsel);
> +		evsel__delete(evsel);

ack

>  	}
>  	free(list_evsel);
>  }
> @@ -326,6 +326,7 @@ PE_NAME opt_pmu_config
>  	}
>  	parse_events_terms__delete($2);
>  	parse_events_terms__delete(orig_terms);
> +	free(pattern);

ack

could you please send the separate change for libperf?
and synchronize with that other patch mentioned above

thanks,
jirka


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] perf parse-events: fix memory leaks found on parse_events
  2020-03-18 10:40 ` Jiri Olsa
@ 2020-03-19  3:56   ` Ian Rogers
  2020-03-23 10:59     ` Jiri Olsa
  0 siblings, 1 reply; 4+ messages in thread
From: Ian Rogers @ 2020-03-19  3:56 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, Andi Kleen,
	Adrian Hunter, Leo Yan, LKML, clang-built-linux,
	Stephane Eranian

On Wed, Mar 18, 2020 at 3:40 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Sun, Mar 15, 2020 at 09:14:31PM -0700, Ian Rogers wrote:
> > Memory leaks found by applying LLVM's libfuzzer on the parse_events
> > function.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/lib/perf/evlist.c        | 2 ++
> >  tools/perf/util/parse-events.c | 2 ++
> >  tools/perf/util/parse-events.y | 3 ++-
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> > index 5b9f2ca50591..6485d1438f75 100644
> > --- a/tools/lib/perf/evlist.c
> > +++ b/tools/lib/perf/evlist.c
> > @@ -125,8 +125,10 @@ static void perf_evlist__purge(struct perf_evlist *evlist)
> >  void perf_evlist__exit(struct perf_evlist *evlist)
> >  {
> >       perf_cpu_map__put(evlist->cpus);
> > +     perf_cpu_map__put(evlist->all_cpus);
>
> ugh, yes, could you please put it to separate libperf patch?

Done. https://lkml.org/lkml/2020/3/18/1318

> >       perf_thread_map__put(evlist->threads);
> >       evlist->cpus = NULL;
> > +     evlist->all_cpus = NULL;
>
> there's already change adding this waiting on the list:
>   https://lore.kernel.org/lkml/1583665157-349023-1-git-send-email-zhe.he@windriver.com/

I'm not seeing this in perf/core on
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
The previous thread mentions Arnaldo porting it. It is only 1
statement so I've left it in.

> >       evlist->threads = NULL;
> >       fdarray__exit(&evlist->pollfd);
> >  }
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index a14995835d85..997862224292 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1482,6 +1482,8 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> >
> >               list_for_each_entry_safe(pos, tmp, &config_terms, list) {
> >                       list_del_init(&pos->list);
> > +                     if (pos->free_str)
> > +                             free(pos->val.str);
>
> ack, would be nice to have  perf_evsel__free_config_terms generalized
> to work directly over config terms list, so we'd have only single
> cleanup function
>
> >                       free(pos);
> >               }
> >               return -EINVAL;
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 94f8bcd83582..8212cc771667 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -44,7 +44,7 @@ static void free_list_evsel(struct list_head* list_evsel)
> >
> >       list_for_each_entry_safe(evsel, tmp, list_evsel, core.node) {
> >               list_del_init(&evsel->core.node);
> > -             perf_evsel__delete(evsel);
> > +             evsel__delete(evsel);
>
> ack
>
> >       }
> >       free(list_evsel);
> >  }
> > @@ -326,6 +326,7 @@ PE_NAME opt_pmu_config
> >       }
> >       parse_events_terms__delete($2);
> >       parse_events_terms__delete(orig_terms);
> > +     free(pattern);
>
> ack
>
> could you please send the separate change for libperf?
> and synchronize with that other patch mentioned above

Done-ish. Thanks,
Ian

> thanks,
> jirka
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200318104011.GF821557%40krava.

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] perf parse-events: fix memory leaks found on parse_events
  2020-03-19  3:56   ` Ian Rogers
@ 2020-03-23 10:59     ` Jiri Olsa
  0 siblings, 0 replies; 4+ messages in thread
From: Jiri Olsa @ 2020-03-23 10:59 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, Andi Kleen,
	Adrian Hunter, Leo Yan, LKML, clang-built-linux,
	Stephane Eranian

On Wed, Mar 18, 2020 at 08:56:26PM -0700, Ian Rogers wrote:
> On Wed, Mar 18, 2020 at 3:40 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Sun, Mar 15, 2020 at 09:14:31PM -0700, Ian Rogers wrote:
> > > Memory leaks found by applying LLVM's libfuzzer on the parse_events
> > > function.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/lib/perf/evlist.c        | 2 ++
> > >  tools/perf/util/parse-events.c | 2 ++
> > >  tools/perf/util/parse-events.y | 3 ++-
> > >  3 files changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> > > index 5b9f2ca50591..6485d1438f75 100644
> > > --- a/tools/lib/perf/evlist.c
> > > +++ b/tools/lib/perf/evlist.c
> > > @@ -125,8 +125,10 @@ static void perf_evlist__purge(struct perf_evlist *evlist)
> > >  void perf_evlist__exit(struct perf_evlist *evlist)
> > >  {
> > >       perf_cpu_map__put(evlist->cpus);
> > > +     perf_cpu_map__put(evlist->all_cpus);
> >
> > ugh, yes, could you please put it to separate libperf patch?
> 
> Done. https://lkml.org/lkml/2020/3/18/1318
> 
> > >       perf_thread_map__put(evlist->threads);
> > >       evlist->cpus = NULL;
> > > +     evlist->all_cpus = NULL;
> >
> > there's already change adding this waiting on the list:
> >   https://lore.kernel.org/lkml/1583665157-349023-1-git-send-email-zhe.he@windriver.com/

Arnaldo, could you plz pull this one ^^^ ?

thanks,
jirka


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-23 10:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-16  4:14 [PATCH] perf parse-events: fix memory leaks found on parse_events Ian Rogers
2020-03-18 10:40 ` Jiri Olsa
2020-03-19  3:56   ` Ian Rogers
2020-03-23 10:59     ` Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).