Re: [PATCH] perf trace: Fix potential USE_AFTER_FREE problem

[Jiri Olsa]

On Thu, Feb 14, 2019 at 05:22:39AM -0500, YU Bo wrote:
> Hi,
> On Thu, Feb 14, 2019 at 09:34:11AM +0100, Jiri Olsa wrote:
> > On Thu, Feb 14, 2019 at 12:23:56AM -0500, Bo YU wrote:
> > > From: Bo Yu <tsu.yubo@xxxxxxxxx>
> > > 
> > > There is a freed pointer "evsel", so fix it.
> > > 
> > > Detected by CoverityScan, CID#1442595("Memory-illegalaccesses
> > > (USE_AFTER_FREE)")
> > > Fixes: 6ab3bc240ade4("perf trace: Support multiple "vfs_getname" probes")
> > > 
> > > Signed-off-by: Bo Yu <tsu.yubo@xxxxxxxxx>
> > > ---
> > >  tools/perf/builtin-trace.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > > index b36061cd1ab8..4036b20a1067 100644
> > > --- a/tools/perf/builtin-trace.c
> > > +++ b/tools/perf/builtin-trace.c
> > > @@ -2515,7 +2515,7 @@ static size_t trace__fprintf_thread_summary(struct trace *trace, FILE *fp);
> > >  static bool perf_evlist__add_vfs_getname(struct perf_evlist *evlist)
> > >  {
> > >  	bool found = false;
> > > -	struct perf_evsel *evsel, *tmp;
> > > +	struct perf_evsel *evsel = NULL, *tmp;
> > 
> > hum, I can't see how this change could matter,
> > could you pelase explain
> First, this is a warning reported by CoverityScan,but in fact i do not how
> to answer your question :(.

I understand that, however at the same time I think
it's good to have an idea what the patch is doing ;-)

> Second, if i remember right, temporary element of list_for_each_entry_safe
> should be initialized with NULL otherwise it will complain via gcc.
> Please correct me :)

hum, from quick look:

	perf_evlist__add_vfs_getname
		struct perf_evsel *evsel;

		evlist__for_each_entry_safe(evlist, evsel, tmp)
		-> __evlist__for_each_entry_safe(&(evlist)->entries, tmp, evsel)

		__evlist__for_each_entry_safe(list, tmp, evsel) \
		-> list_for_each_entry_safe(evsel, tmp, list, node)

		list_for_each_entry_safe(pos, n, head, member)                  \
		-> for (pos = list_first_entry(head, typeof(*pos), member),        \
			n = list_next_entry(pos, member);                       \
			&pos->member != (head);                                    \
			pos = n, n = list_next_entry(n, member))


unless I'm missing something 'evsel' is being initialized
in the for loop init section with this statement:

  pos = list_first_entry(head, typeof(*pos), member)


jirka