linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>,
	linux-perf-users@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf hist: fix memory leak if histogram entry is reused
Date: Thu, 21 Feb 2019 14:53:20 +0100	[thread overview]
Message-ID: <20190221135320.io7egcwajehfrdd5@studium.uni-erlangen.de> (raw)
In-Reply-To: <20190221123909.GG10990@krava>

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,

  reply	other threads:[~2019-02-21 13:53 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-21 12:23 Jonas Rabenstein
2019-02-21 12:39 ` Jiri Olsa
2019-02-21 13:53   ` Jonas Rabenstein [this message]
2019-02-21 16:28     ` Jiri Olsa
2019-02-23  3:18       ` Namhyung Kim
2019-02-25 13:54         ` Jiri Olsa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190221135320.io7egcwajehfrdd5@studium.uni-erlangen.de \
    --to=jonas.rabenstein@studium.uni-erlangen.de \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --subject='Re: [PATCH] perf hist: fix memory leak if histogram entry is reused' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox