Messages in this thread | | | Date | Tue, 11 Oct 2016 02:28:25 -0700 | From | Krister Johansen <> | Subject | Re: callchain map refcounting fixes was Re: [PATCH perf/core] perf script: fix a use after free crash. |
| |
On Sat, Oct 08, 2016 at 11:13:21PM -0700, Krister Johansen wrote: > On Fri, Oct 07, 2016 at 11:22:00AM +0900, Namhyung Kim wrote: > > On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote: > > > Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu: > > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > > > > index b02992e..f8335e8 100644 > > > > --- a/tools/perf/util/hist.c > > > > +++ b/tools/perf/util/hist.c > > > > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter, > > > > { > > > > zfree(&iter->priv); > > > > iter->he = NULL; > > > > + map__zput(al->map); > > > > What is this needed? Why other places like iter_finish_normal_entry > > isn't? > > I put a map__zput() here because iter_next_cumulative_entry() calls > fill_callchain_info(), which takes a reference on the al->map in the > struct addr_location that it returns. I had forgotten all of this by > the time you asked. When I went back to work out my rationale, I > stumbled across addr_location__put(). Think it would make sense to > relocate the put of al->map there instead?
I gave the addr_location__put() approach a try, but it caused me to remember why I had kept this small. There are certain circumstances where callers that lookup maps don't take references, seemingly because the map is already contained within a map group. If I move this to addr_location__put(), then I need to expand the scope of this change. Right now, it's just focusing on making sure that any map that gets embedded into a callchain cursor, or retrieved from one and held onto, gets referenced.
In other words, moving this to addr_location__put() frees a bunch of maps that are acquired from elsewhere without their reference counts incremented.
I made the rest of the modifications we discussed. I'll send a v2 patch in a separate e-mail.
-K
| |