Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 14 Feb 2019 12:23:57 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
        alexander.shishkin@linux.intel.com, namhyung@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf trace: Fix potential USE_AFTER_FREE  problem
Message-ID: <20190214112357.GB26714@krava>
References: <20190214052356.26884-1-tsu.yubo@gmail.com>
 <20190214083411.GA25842@krava>
 <20190214102238.nobo5vfsvmlyjyhv@yubo-2>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190214102238.nobo5vfsvmlyjyhv@yubo-2>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@gmail.com>
> > > 
> > > 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@gmail.com>
> > > ---
> > >  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