[PATCH] perf parse-events: fix memory leaks found on parse_events

[Ian Rogers]

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

Signed-off-by: Ian Rogers <iro...@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

[Jiri Olsa]

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 <iro...@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-...@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

[Ian Rogers]

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

Signed-off-by: Ian Rogers <iro...@google.com>
---

tools/perf/util/parse-events.c | 2 ++
tools/perf/util/parse-events.y | 3 ++-

2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 593b6b03785d..1e0bec5c0846 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.696.g5e7596f4ac-goog

[Ian Rogers]

Memory leaks found by applying LLVM's libfuzzer on the tools/perf

parse_events function.

Signed-off-by: Ian Rogers <iro...@google.com>
---

tools/lib/perf/evlist.c | 2 ++
1 file changed, 2 insertions(+)

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);
}

--
2.25.1.696.g5e7596f4ac-goog

[Ian Rogers]

On Wed, Mar 18, 2020 at 3:40 AM Jiri Olsa <jo...@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 <iro...@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-...@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.

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-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200318104011.GF821557%40krava.

[Jiri Olsa]

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

thanks,
jirka

[Jiri Olsa]

On Wed, Mar 18, 2020 at 07:31:00PM -0700, Ian Rogers wrote:
> Memory leaks found by applying LLVM's libfuzzer on the parse_events
> function.
>
> Signed-off-by: Ian Rogers <iro...@google.com>

Acked-by: Jiri Olsa <jo...@redhat.com>

thanks,
jirka

[Jiri Olsa]

On Wed, Mar 18, 2020 at 07:31:01PM -0700, Ian Rogers wrote:
> Memory leaks found by applying LLVM's libfuzzer on the tools/perf
> parse_events function.
>
> Signed-off-by: Ian Rogers <iro...@google.com>

Arnaldo,
could you plz pull first:
https://lore.kernel.org/lkml/1583665157-349023-1-...@windriver.com/

and then merge in this one?

thanks,
jirka

Acked-by: Jiri Olsa <jo...@redhat.com>

[Ian Rogers]

On Mon, Mar 23, 2020 at 3:59 AM Jiri Olsa <jo...@redhat.com> wrote:
>
> On Wed, Mar 18, 2020 at 07:31:00PM -0700, Ian Rogers wrote:
> > Memory leaks found by applying LLVM's libfuzzer on the parse_events
> > function.
> >
> > Signed-off-by: Ian Rogers <iro...@google.com>
>
> Acked-by: Jiri Olsa <jo...@redhat.com>
>
> thanks,
> jirka

Ping.

[Arnaldo Carvalho de Melo]

Em Wed, Mar 18, 2020 at 07:31:00PM -0700, Ian Rogers escreveu:
> Memory leaks found by applying LLVM's libfuzzer on the parse_events
> function.
>
> Signed-off-by: Ian Rogers <iro...@google.com>
> ---
> tools/perf/util/parse-events.c | 2 ++
> tools/perf/util/parse-events.y | 3 ++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 593b6b03785d..1e0bec5c0846 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);

I'm applying it but only after changing it to zfree(&pos->free_str), to
make sure that any othe rcode that may still hold a pointer to pos will
see a NULL in ->free_str and crash sooner rather than later.

> free(pos);
> }
> return -EINVAL;

And the following should be in a different patch

> 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);
> }

And this one in another, I'll fix this up, but please try in the future
to provide different patches for different fixes, so that if we
eventually find out that one of the unrelated fixes is wrong, then we
can revert the patch more easily with 'git revert' instead of having to
do a patch that reverts just part of the bigger hodge-podge patch.

If you go and have a track record of doing this as piecemeal as
possible, I will in turn feel more confident of processing your patches
in a faster fashion ;-) :-)

Thanks,

- Arnaldo

> @@ -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.696.g5e7596f4ac-goog
>

--

- Arnaldo

[Ian Rogers]

Thanks, at some point I'd like to get libfuzzer with asan working for
more than just me so that we don't backslide. It'd also make the
reproductions easier to share.

Ian

[Arnaldo Carvalho de Melo]

If we can detect the presence of the needed components, libraries,
compiler with the right feature set, yeah, a 'perf test' built under
such environment surely would benefit from having further tests,

- Arnaldo

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID: 4599d292128d89e4cf866a0ea9a9b047a2de8418
Gitweb: https://git.kernel.org/tip/4599d292128d89e4cf866a0ea9a9b047a2de8418
Author: Ian Rogers <iro...@google.com>
AuthorDate: Wed, 18 Mar 2020 19:31:01 -07:00
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitterDate: Tue, 05 May 2020 16:35:29 -03:00

libperf evlist: Fix a refcount leak

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

Signed-off-by: Ian Rogers <iro...@google.com>

Cc: Adrian Hunter <adrian...@intel.com>
Cc: Alexander Shishkin <alexander...@linux.intel.com>
Cc: Andi Kleen <a...@linux.intel.com>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: Leo Yan <leo...@linaro.org>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Namhyung Kim <namh...@kernel.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Stephane Eranian <era...@google.com>
Cc: clang-bu...@googlegroups.com
Link: http://lore.kernel.org/lkml/20200319023101....@google.com
[ Did a minor adjustment due to some other previous patch having already set evlist->all_cpus to NULL at perf_evlist__exit() ]
Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---
tools/lib/perf/evlist.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index c481b62..6a875a0 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -123,6 +123,7 @@ 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;

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID: 672f707ef55f0a56b9bb67c3cfbdcab85c233796
Gitweb: https://git.kernel.org/tip/672f707ef55f0a56b9bb67c3cfbdcab85c233796
Author: Ian Rogers <iro...@google.com>
AuthorDate: Wed, 18 Mar 2020 19:31:00 -07:00

Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitterDate: Tue, 05 May 2020 16:35:29 -03:00

perf parse-events: Fix memory leaks found on parse_events

free_list_evsel() deals with tools/perf/ evsels, not with libperf
perf_evsels, use the right destructor and avoid a leak, as
evsel__delete() will delete something perf_evsel__delete() doesn't.

Signed-off-by: Ian Rogers <iro...@google.com>

Cc: Adrian Hunter <adrian...@intel.com>

Acked-by: Jiri Olsa <jo...@redhat.com>

Cc: Alexander Shishkin <alexander...@linux.intel.com>
Cc: Andi Kleen <a...@linux.intel.com>

Cc: Leo Yan <leo...@linaro.org>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Namhyung Kim <namh...@kernel.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Stephane Eranian <era...@google.com>
Cc: clang-bu...@googlegroups.com
Link: http://lore.kernel.org/lkml/20200319023101....@google.com

[ split from a larger patch ]

Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---

tools/perf/util/parse-events.y | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index e879eb2..6ece67e 100644

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID: e8dfb81838b14f82521968343884665b996646ef
Gitweb: https://git.kernel.org/tip/e8dfb81838b14f82521968343884665b996646ef

Author: Ian Rogers <iro...@google.com>
AuthorDate: Wed, 18 Mar 2020 19:31:00 -07:00
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitterDate: Tue, 05 May 2020 16:35:29 -03:00

perf parse-events: Fix memory leaks found on parse_events

Fix a memory leak found by applying LLVM's libfuzzer on parse_events().

Signed-off-by: Ian Rogers <iro...@google.com>
Acked-by: Jiri Olsa <jo...@redhat.com>
Cc: Adrian Hunter <adrian...@intel.com>

Cc: Alexander Shishkin <alexander...@linux.intel.com>
Cc: Andi Kleen <a...@linux.intel.com>
Cc: Leo Yan <leo...@linaro.org>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Namhyung Kim <namh...@kernel.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Stephane Eranian <era...@google.com>
Cc: clang-bu...@googlegroups.com
Link: http://lore.kernel.org/lkml/20200319023101....@google.com

[ split from a larger patch, use zfree() ]

Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---

tools/perf/util/parse-events.c | 1 +

1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5795f3a..6dc9e57 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1482,6 +1482,7 @@ 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);

+ zfree(&pos->val.str);
free(pos);
}
return -EINVAL;

[tip-bot2 for Ian Rogers]

The following commit has been merged into the perf/core branch of tip:

Commit-ID: ba08829aace99b23da31b9b71d8ed5d40a44ed49
Gitweb: https://git.kernel.org/tip/ba08829aace99b23da31b9b71d8ed5d40a44ed49

Author: Ian Rogers <iro...@google.com>
AuthorDate: Wed, 18 Mar 2020 19:31:00 -07:00
Committer: Arnaldo Carvalho de Melo <ac...@redhat.com>
CommitterDate: Tue, 05 May 2020 16:35:29 -03:00

perf parse-events: Fix another memory leaks found on parse_events()

Fix another memory leak found by applying LLVM's libfuzzer on parse_events().

Signed-off-by: Ian Rogers <iro...@google.com>
Acked-by: Jiri Olsa <jo...@redhat.com>
Cc: Adrian Hunter <adrian...@intel.com>
Cc: Alexander Shishkin <alexander...@linux.intel.com>
Cc: Andi Kleen <a...@linux.intel.com>
Cc: Leo Yan <leo...@linaro.org>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Namhyung Kim <namh...@kernel.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Stephane Eranian <era...@google.com>
Cc: clang-bu...@googlegroups.com
Link: http://lore.kernel.org/lkml/20200319023101....@google.com

[ split from a larger patch ]

Signed-off-by: Arnaldo Carvalho de Melo <ac...@redhat.com>
---

tools/perf/util/parse-events.y | 1 +

1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 6ece67e..c4ca932 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y