2019-02-21 12:23:50

by Jonas Rabenstein

Subject: [PATCH] perf hist: fix memory leak if histogram entry is reused

In __hists__add_entry the srcline of the addr_location is duplicated
for the hist_entry. If hists__findnew_entry returns an already existing
hist_entry the srcline has to be freed again as no further reference to
that duplicated srcline would exists anymore.

Signed-off-by: Jonas Rabenstein <[email protected]>
---
tools/perf/util/hist.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 8aad8330e392..25b8dbdbbe87 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -623,6 +623,9 @@ __hists__add_entry(struct hists *hists,
.ops = ops,
}, *he = hists__findnew_entry(hists, &entry, al, sample_self);

+ if (he && he->srcline != entry.srcline)
+ free(entry.srcline);
+
if (!hists->has_callchains && he && he->callchain_size != 0)
hists->has_callchains = true;
return he;
--
2.19.2

2019-02-21 12:40:11

by Jiri Olsa

Subject: Re: [PATCH] perf hist: fix memory leak if histogram entry is reused

On Thu, Feb 21, 2019 at 01:23:06PM +0100, Jonas Rabenstein wrote:
> In __hists__add_entry the srcline of the addr_location is duplicated
> for the hist_entry. If hists__findnew_entry returns an already existing
> hist_entry the srcline has to be freed again as no further reference to
> that duplicated srcline would exists anymore.
>
> Signed-off-by: Jonas Rabenstein <[email protected]>
> ---
> tools/perf/util/hist.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 8aad8330e392..25b8dbdbbe87 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -623,6 +623,9 @@ __hists__add_entry(struct hists *hists,
> .ops = ops,
> }, *he = hists__findnew_entry(hists, &entry, al, sample_self);
>
> + if (he && he->srcline != entry.srcline)
> + free(entry.srcline);
> +
> if (!hists->has_callchains && he && he->callchain_size != 0)
> hists->has_callchains = true;
> return he;

nice, do we have similar issue in here?

jirka


---
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 216388003dea..e65e6822c848 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -966,7 +966,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
.map = al->map,
.sym = al->sym,
},
- .srcline = al->srcline ? strdup(al->srcline) : NULL,
+ .srcline = al->srcline,
.parent = iter->parent,
.raw_data = sample->raw_data,
.raw_size = sample->raw_size,

2019-02-21 16:31:11

by Jiri Olsa

Subject: Re: [PATCH] perf hist: fix memory leak if histogram entry is reused

On Thu, Feb 21, 2019 at 02:53:20PM +0100, Jonas Rabenstein wrote:
> On Thu, Feb 21, 2019 at 01:39:09PM +0100, Jiri Olsa wrote:
> > On Thu, Feb 21, 2019 at 01:23:06PM +0100, Jonas Rabenstein wrote:
> > > In __hists__add_entry the srcline of the addr_location is duplicated
> > > for the hist_entry. If hists__findnew_entry returns an already existing
> > > hist_entry the srcline has to be freed again as no further reference to
> > > that duplicated srcline would exists anymore.
> > >
> > > Signed-off-by: Jonas Rabenstein <[email protected]>
> > > ---
> > > tools/perf/util/hist.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index 8aad8330e392..25b8dbdbbe87 100644
> > > --- a/tools/perf/util/hist.c
> > > +++ b/tools/perf/util/hist.c
> > > @@ -623,6 +623,9 @@ __hists__add_entry(struct hists *hists,
> > > .ops = ops,
> > > }, *he = hists__findnew_entry(hists, &entry, al, sample_self);
> > >
> > > + if (he && he->srcline != entry.srcline)
> > > + free(entry.srcline);
> > > +
> > > if (!hists->has_callchains && he && he->callchain_size != 0)
> > > hists->has_callchains = true;
> > > return he;
> >
> > nice, do we have similar issue in here?
> >
> > jirka
> >
> >
> > ---
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index 216388003dea..e65e6822c848 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -966,7 +966,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
> > .map = al->map,
> > .sym = al->sym,
> > },
> > - .srcline = al->srcline ? strdup(al->srcline) : NULL,
> > + .srcline = al->srcline,
> While this shouldn't leak the memory, we may end up with an al->srcline
> to get free afterwards while still having a reference on it within the
> hist_entry... Also I could not find where/how the hist_entry is freed up
> so we may get an double free if both of al and he clean the srcline.
> Of course, with your solution we could spare the useless strdup/free if
> we find an hist_entry (which should be the typical case for hotspots..).
> Taking a deeper look thus should be beneficial - but I do not have the
> time for that right now because I'm still working on the inline-symbol
> integration which is more important for me...

ok, I'll check it

jirka

2019-02-23 03:19:36

by Namhyung Kim

Subject: Re: [PATCH] perf hist: fix memory leak if histogram entry is reused

Hi,

On Fri, Feb 22, 2019 at 1:29 AM Jiri Olsa <[email protected]> wrote:
>
> On Thu, Feb 21, 2019 at 02:53:20PM +0100, Jonas Rabenstein wrote:
> > On Thu, Feb 21, 2019 at 01:39:09PM +0100, Jiri Olsa wrote:
> > > On Thu, Feb 21, 2019 at 01:23:06PM +0100, Jonas Rabenstein wrote:
> > > > In __hists__add_entry the srcline of the addr_location is duplicated
> > > > for the hist_entry. If hists__findnew_entry returns an already existing
> > > > hist_entry the srcline has to be freed again as no further reference to
> > > > that duplicated srcline would exists anymore.
> > > >
> > > > Signed-off-by: Jonas Rabenstein <[email protected]>
> > > > ---
> > > > tools/perf/util/hist.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > > index 8aad8330e392..25b8dbdbbe87 100644
> > > > --- a/tools/perf/util/hist.c
> > > > +++ b/tools/perf/util/hist.c
> > > > @@ -623,6 +623,9 @@ __hists__add_entry(struct hists *hists,
> > > > .ops = ops,
> > > > }, *he = hists__findnew_entry(hists, &entry, al, sample_self);
> > > >
> > > > + if (he && he->srcline != entry.srcline)
> > > > + free(entry.srcline);
> > > > +
> > > > if (!hists->has_callchains && he && he->callchain_size != 0)
> > > > hists->has_callchains = true;
> > > > return he;
> > >
> > > nice, do we have similar issue in here?
> > >
> > > jirka
> > >
> > >
> > > ---
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index 216388003dea..e65e6822c848 100644
> > > --- a/tools/perf/util/hist.c
> > > +++ b/tools/perf/util/hist.c
> > > @@ -966,7 +966,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
> > > .map = al->map,
> > > .sym = al->sym,
> > > },
> > > - .srcline = al->srcline ? strdup(al->srcline) : NULL,
> > > + .srcline = al->srcline,
> > While this shouldn't leak the memory, we may end up with an al->srcline
> > to get free afterwards while still having a reference on it within the
> > hist_entry... Also I could not find where/how the hist_entry is freed up
> > so we may get an double free if both of al and he clean the srcline.
> > Of course, with your solution we could spare the useless strdup/free if
> > we find an hist_entry (which should be the typical case for hotspots..).
> > Taking a deeper look thus should be beneficial - but I do not have the
> > time for that right now because I'm still working on the inline-symbol
> > integration which is more important for me...
>
> ok, I'll check it

I think we can defer calling strdup() to hist_entry__init().

Thanks,
Namhyung

2019-02-25 13:55:24

by Jiri Olsa

Subject: Re: [PATCH] perf hist: fix memory leak if histogram entry is reused

On Sat, Feb 23, 2019 at 12:18:18PM +0900, Namhyung Kim wrote:
> Hi,
>
> On Fri, Feb 22, 2019 at 1:29 AM Jiri Olsa <[email protected]> wrote:
> >
> > On Thu, Feb 21, 2019 at 02:53:20PM +0100, Jonas Rabenstein wrote:
> > > On Thu, Feb 21, 2019 at 01:39:09PM +0100, Jiri Olsa wrote:
> > > > On Thu, Feb 21, 2019 at 01:23:06PM +0100, Jonas Rabenstein wrote:
> > > > > In __hists__add_entry the srcline of the addr_location is duplicated
> > > > > for the hist_entry. If hists__findnew_entry returns an already existing
> > > > > hist_entry the srcline has to be freed again as no further reference to
> > > > > that duplicated srcline would exists anymore.
> > > > >
> > > > > Signed-off-by: Jonas Rabenstein <[email protected]>
> > > > > ---
> > > > > tools/perf/util/hist.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > > > index 8aad8330e392..25b8dbdbbe87 100644
> > > > > --- a/tools/perf/util/hist.c
> > > > > +++ b/tools/perf/util/hist.c
> > > > > @@ -623,6 +623,9 @@ __hists__add_entry(struct hists *hists,
> > > > > .ops = ops,
> > > > > }, *he = hists__findnew_entry(hists, &entry, al, sample_self);
> > > > >
> > > > > + if (he && he->srcline != entry.srcline)
> > > > > + free(entry.srcline);
> > > > > +
> > > > > if (!hists->has_callchains && he && he->callchain_size != 0)
> > > > > hists->has_callchains = true;
> > > > > return he;
> > > >
> > > > nice, do we have similar issue in here?
> > > >
> > > > jirka
> > > >
> > > >
> > > > ---
> > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > > index 216388003dea..e65e6822c848 100644
> > > > --- a/tools/perf/util/hist.c
> > > > +++ b/tools/perf/util/hist.c
> > > > @@ -966,7 +966,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
> > > > .map = al->map,
> > > > .sym = al->sym,
> > > > },
> > > > - .srcline = al->srcline ? strdup(al->srcline) : NULL,
> > > > + .srcline = al->srcline,
> > > While this shouldn't leak the memory, we may end up with an al->srcline
> > > to get free afterwards while still having a reference on it within the
> > > hist_entry... Also I could not find where/how the hist_entry is freed up
> > > so we may get an double free if both of al and he clean the srcline.
> > > Of course, with your solution we could spare the useless strdup/free if
> > > we find an hist_entry (which should be the typical case for hotspots..).
> > > Taking a deeper look thus should be beneficial - but I do not have the
> > > time for that right now because I'm still working on the inline-symbol
> > > integration which is more important for me...
> >
> > ok, I'll check it
>
> I think we can defer calling strdup() to hist_entry__init().

right, thats actualy better fix even for the original patch,
I'll try to post it soon

thanks,
jirka