* [PATCH] perf hist: fix memory leak if histogram entry is reused @ 2019-02-21 12:23 Jonas Rabenstein 2019-02-21 12:39 ` Jiri Olsa 0 siblings, 1 reply; 6+ messages in thread From: Jonas Rabenstein @ 2019-02-21 12:23 UTC (permalink / raw) To: linux-perf-users Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-kernel, Jonas Rabenstein 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 <jonas.rabenstein@studium.uni-erlangen.de> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] perf hist: fix memory leak if histogram entry is reused 2019-02-21 12:23 [PATCH] perf hist: fix memory leak if histogram entry is reused Jonas Rabenstein @ 2019-02-21 12:39 ` Jiri Olsa 2019-02-21 13:53 ` Jonas Rabenstein 0 siblings, 1 reply; 6+ messages in thread From: Jiri Olsa @ 2019-02-21 12:39 UTC (permalink / raw) To: Jonas Rabenstein Cc: linux-perf-users, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Namhyung Kim, linux-kernel 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 <jonas.rabenstein@studium.uni-erlangen.de> > --- > 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, ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] perf hist: fix memory leak if histogram entry is reused 2019-02-21 12:39 ` Jiri Olsa @ 2019-02-21 13:53 ` Jonas Rabenstein 2019-02-21 16:28 ` Jiri Olsa 0 siblings, 1 reply; 6+ messages in thread From: Jonas Rabenstein @ 2019-02-21 13:53 UTC (permalink / raw) To: Jiri Olsa Cc: Jonas Rabenstein, linux-perf-users, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Namhyung Kim, linux-kernel 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 <jonas.rabenstein@studium.uni-erlangen.de> > > --- > > 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... - Jonas > .parent = iter->parent, > .raw_data = sample->raw_data, > .raw_size = sample->raw_size, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf hist: fix memory leak if histogram entry is reused 2019-02-21 13:53 ` Jonas Rabenstein @ 2019-02-21 16:28 ` Jiri Olsa 2019-02-23 3:18 ` Namhyung Kim 0 siblings, 1 reply; 6+ messages in thread From: Jiri Olsa @ 2019-02-21 16:28 UTC (permalink / raw) To: Jonas Rabenstein Cc: linux-perf-users, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Namhyung Kim, linux-kernel 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 <jonas.rabenstein@studium.uni-erlangen.de> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf hist: fix memory leak if histogram entry is reused 2019-02-21 16:28 ` Jiri Olsa @ 2019-02-23 3:18 ` Namhyung Kim 2019-02-25 13:54 ` Jiri Olsa 0 siblings, 1 reply; 6+ messages in thread From: Namhyung Kim @ 2019-02-23 3:18 UTC (permalink / raw) To: Jiri Olsa Cc: Jonas Rabenstein, linux-perf-users, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, linux-kernel Hi, On Fri, Feb 22, 2019 at 1:29 AM Jiri Olsa <jolsa@redhat.com> 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 <jonas.rabenstein@studium.uni-erlangen.de> > > > > --- > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf hist: fix memory leak if histogram entry is reused 2019-02-23 3:18 ` Namhyung Kim @ 2019-02-25 13:54 ` Jiri Olsa 0 siblings, 0 replies; 6+ messages in thread From: Jiri Olsa @ 2019-02-25 13:54 UTC (permalink / raw) To: Namhyung Kim Cc: Jonas Rabenstein, linux-perf-users, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, linux-kernel 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 <jolsa@redhat.com> 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 <jonas.rabenstein@studium.uni-erlangen.de> > > > > > --- > > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-25 13:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-21 12:23 [PATCH] perf hist: fix memory leak if histogram entry is reused Jonas Rabenstein 2019-02-21 12:39 ` Jiri Olsa 2019-02-21 13:53 ` Jonas Rabenstein 2019-02-21 16:28 ` Jiri Olsa 2019-02-23 3:18 ` Namhyung Kim 2019-02-25 13:54 ` 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).