[PATCH] perf trace: Fix potential USE_AFTER_FREE problem

[PATCH] perf trace: Fix potential USE_AFTER_FREE problem

[Bo YU]

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;
 	struct parse_events_error err = { .idx = 0, };
 	int ret = parse_events(evlist, "probe:vfs_getname*", &err);
 
-- 
2.11.0

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

[Jiri Olsa]

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?

jirka

>  	struct parse_events_error err = { .idx = 0, };
>  	int ret = parse_events(evlist, "probe:vfs_getname*", &err);
>  
> -- 
> 2.11.0
> 

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

[YU Bo]

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 :(.
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 :)
Thanks,
>
>jirka
>
>>  	struct parse_events_error err = { .idx = 0, };
>>  	int ret = parse_events(evlist, "probe:vfs_getname*", &err);
>>
>> --
>> 2.11.0
>>

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@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