[PATCH] perf: double free at function perf_hpp__reset_output_field

[PATCH] perf: double free at function perf_hpp__reset_output_field

[changbin.du]

From: Changbin Du <changbin.du@gmail.com>

Some perf_hpp_fmt both registered at field and sort list. For such
instance, we only can free it when removed from the both lists.

Signed-off-by: Changbin Du <changbin.du@gmail.com>
Signed-off-by: Changbin Du <changbin.du@intel.com>
---
 tools/perf/ui/hist.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 5d632dc..f94b301 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
 
 void perf_hpp__reset_output_field(struct perf_hpp_list *list)
 {
-	struct perf_hpp_fmt *fmt, *tmp;
+	struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
 
 	/* reset output fields */
-	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
-		list_del_init(&fmt->list);
-		list_del_init(&fmt->sort_list);
-		fmt_free(fmt);
+	perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
+		list_del_init(&field_fmt->list);
+		/* reset sort keys */
+		perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
+			if (field_fmt == sort_fmt) {
+				list_del_init(&field_fmt->sort_list);
+				break;
+			}
+		}
+		fmt_free(field_fmt);
 	}
 
-	/* reset sort keys */
-	perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
-		list_del_init(&fmt->list);
-		list_del_init(&fmt->sort_list);
-		fmt_free(fmt);
+	/* reset remaining sort keys */
+	perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) {
+		list_del_init(&sort_fmt->sort_list);
+		fmt_free(sort_fmt);
 	}
 }
 
-- 
2.7.4

[PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[changbin.du]

From: Changbin Du <changbin.du@intel.com>

Some perf_hpp_fmt both registered at field and sort list. For such
instance, we only can free it when removed from the both lists. This
function currently only used by self-test code, but still should fix
it.

Signed-off-by: Changbin Du <changbin.du@intel.com>
---
v2: removed redundant Signed-off.

---
 tools/perf/ui/hist.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 5d632dc..f94b301 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
 
 void perf_hpp__reset_output_field(struct perf_hpp_list *list)
 {
-	struct perf_hpp_fmt *fmt, *tmp;
+	struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
 
 	/* reset output fields */
-	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
-		list_del_init(&fmt->list);
-		list_del_init(&fmt->sort_list);
-		fmt_free(fmt);
+	perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
+		list_del_init(&field_fmt->list);
+		/* reset sort keys */
+		perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
+			if (field_fmt == sort_fmt) {
+				list_del_init(&field_fmt->sort_list);
+				break;
+			}
+		}
+		fmt_free(field_fmt);
 	}
 
-	/* reset sort keys */
-	perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
-		list_del_init(&fmt->list);
-		list_del_init(&fmt->sort_list);
-		fmt_free(fmt);
+	/* reset remaining sort keys */
+	perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) {
+		list_del_init(&sort_fmt->sort_list);
+		fmt_free(sort_fmt);
 	}
 }
 
-- 
2.7.4

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Arnaldo Carvalho de Melo]

Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> From: Changbin Du <changbin.du@intel.com>
> 
> Some perf_hpp_fmt both registered at field and sort list. For such
> instance, we only can free it when removed from the both lists. This
> function currently only used by self-test code, but still should fix
> it.

Looks sane, applying,

Jiri, Namhyung, please holler (or ack) if needed,

- Arnaldo
 
> Signed-off-by: Changbin Du <changbin.du@intel.com>
> ---
> v2: removed redundant Signed-off.
> 
> ---
>  tools/perf/ui/hist.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5d632dc..f94b301 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
>  
>  void perf_hpp__reset_output_field(struct perf_hpp_list *list)
>  {
> -	struct perf_hpp_fmt *fmt, *tmp;
> +	struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
>  
>  	/* reset output fields */
> -	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> -		list_del_init(&fmt->list);
> -		list_del_init(&fmt->sort_list);
> -		fmt_free(fmt);
> +	perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> +		list_del_init(&field_fmt->list);
> +		/* reset sort keys */
> +		perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
> +			if (field_fmt == sort_fmt) {
> +				list_del_init(&field_fmt->sort_list);
> +				break;
> +			}
> +		}
> +		fmt_free(field_fmt);
>  	}
>  
> -	/* reset sort keys */
> -	perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
> -		list_del_init(&fmt->list);
> -		list_del_init(&fmt->sort_list);
> -		fmt_free(fmt);
> +	/* reset remaining sort keys */
> +	perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) {
> +		list_del_init(&sort_fmt->sort_list);
> +		fmt_free(sort_fmt);
>  	}
>  }
>  
> -- 
> 2.7.4

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Namhyung Kim]

Hi Arnaldo,

On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
>> From: Changbin Du <changbin.du@intel.com>
>>
>> Some perf_hpp_fmt both registered at field and sort list. For such
>> instance, we only can free it when removed from the both lists. This
>> function currently only used by self-test code, but still should fix
>> it.
>
> Looks sane, applying,
>
> Jiri, Namhyung, please holler (or ack) if needed,

Did you actually see the double free problem?  AFAICS the old code
removed a fmt from both list before free it.  In the first loop, fmt that
was linked to both output list and sort list will be remove.  And the
second loop frees fmt that was linked only to the sort list (IOW, it
frees fmt that was not freed in the first loop).

Thanks,
Namhyung


>
> - Arnaldo
>
>> Signed-off-by: Changbin Du <changbin.du@intel.com>
>> ---
>> v2: removed redundant Signed-off.
>>
>> ---
>>  tools/perf/ui/hist.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
>> index 5d632dc..f94b301 100644
>> --- a/tools/perf/ui/hist.c
>> +++ b/tools/perf/ui/hist.c
>> @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
>>
>>  void perf_hpp__reset_output_field(struct perf_hpp_list *list)
>>  {
>> -     struct perf_hpp_fmt *fmt, *tmp;
>> +     struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
>>
>>       /* reset output fields */
>> -     perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
>> -             list_del_init(&fmt->list);
>> -             list_del_init(&fmt->sort_list);
>> -             fmt_free(fmt);
>> +     perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
>> +             list_del_init(&field_fmt->list);
>> +             /* reset sort keys */
>> +             perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
>> +                     if (field_fmt == sort_fmt) {
>> +                             list_del_init(&field_fmt->sort_list);
>> +                             break;
>> +                     }
>> +             }
>> +             fmt_free(field_fmt);
>>       }
>>
>> -     /* reset sort keys */
>> -     perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
>> -             list_del_init(&fmt->list);
>> -             list_del_init(&fmt->sort_list);
>> -             fmt_free(fmt);
>> +     /* reset remaining sort keys */
>> +     perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) {
>> +             list_del_init(&sort_fmt->sort_list);
>> +             fmt_free(sort_fmt);
>>       }
>>  }
>>
>> --
>> 2.7.4

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Arnaldo Carvalho de Melo]

Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> >> From: Changbin Du <changbin.du@intel.com>
> >>
> >> Some perf_hpp_fmt both registered at field and sort list. For such
> >> instance, we only can free it when removed from the both lists. This
> >> function currently only used by self-test code, but still should fix
> >> it.
> >
> > Looks sane, applying,
> >
> > Jiri, Namhyung, please holler (or ack) if needed,
> 
> Did you actually see the double free problem?  AFAICS the old code

I assumed that he had seen it, in some self-test code, Changbin, can you
please show command output or further describe when this patch would be
necessary?

- Arnaldo

> removed a fmt from both list before free it.  In the first loop, fmt that
> was linked to both output list and sort list will be remove.  And the
> second loop frees fmt that was linked only to the sort list (IOW, it
> frees fmt that was not freed in the first loop).
> 
> Thanks,
> Namhyung
> 
> 
> >
> > - Arnaldo
> >
> >> Signed-off-by: Changbin Du <changbin.du@intel.com>
> >> ---
> >> v2: removed redundant Signed-off.
> >>
> >> ---
> >>  tools/perf/ui/hist.c | 25 +++++++++++++++----------
> >>  1 file changed, 15 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> >> index 5d632dc..f94b301 100644
> >> --- a/tools/perf/ui/hist.c
> >> +++ b/tools/perf/ui/hist.c
> >> @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> >>
> >>  void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> >>  {
> >> -     struct perf_hpp_fmt *fmt, *tmp;
> >> +     struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
> >>
> >>       /* reset output fields */
> >> -     perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> >> -             list_del_init(&fmt->list);
> >> -             list_del_init(&fmt->sort_list);
> >> -             fmt_free(fmt);
> >> +     perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> >> +             list_del_init(&field_fmt->list);
> >> +             /* reset sort keys */
> >> +             perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
> >> +                     if (field_fmt == sort_fmt) {
> >> +                             list_del_init(&field_fmt->sort_list);
> >> +                             break;
> >> +                     }
> >> +             }
> >> +             fmt_free(field_fmt);
> >>       }
> >>
> >> -     /* reset sort keys */
> >> -     perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
> >> -             list_del_init(&fmt->list);
> >> -             list_del_init(&fmt->sort_list);
> >> -             fmt_free(fmt);
> >> +     /* reset remaining sort keys */
> >> +     perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) {
> >> +             list_del_init(&sort_fmt->sort_list);
> >> +             fmt_free(sort_fmt);
> >>       }
> >>  }
> >>
> >> --
> >> 2.7.4

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Du, Changbin]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

On Tue, Apr 04, 2017 at 12:51:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu:
> > Hi Arnaldo,
> > 
> > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> > >> From: Changbin Du <changbin.du@intel.com>
> > >>
> > >> Some perf_hpp_fmt both registered at field and sort list. For such
> > >> instance, we only can free it when removed from the both lists. This
> > >> function currently only used by self-test code, but still should fix
> > >> it.
> > >
> > > Looks sane, applying,
> > >
> > > Jiri, Namhyung, please holler (or ack) if needed,
> > 
> > Did you actually see the double free problem?  AFAICS the old code
> 
> I assumed that he had seen it, in some self-test code, Changbin, can you
> please show command output or further describe when this patch would be
> necessary?
> 
Arnaldo, I did observe this issue but not in self-test code. The self-test code
uses that function but does not have a case that a fmt linked to two both list. 
I found this issue when I try to add 'dynamic sort' feature to perf, which
I use this function to reset out fields.

Anyway, it is clear that this is a real bug, a potential issue need to fix.

> - Arnaldo
> 
> > removed a fmt from both list before free it.  In the first loop, fmt that
> > was linked to both output list and sort list will be remove.  And the
> > second loop frees fmt that was linked only to the sort list (IOW, it
> > frees fmt that was not freed in the first loop).
> >
This is right. It is to handle the fmts that linked to both two lists.

> > Thanks,
> > Namhyung
> > 
> > 
> > >
> > > - Arnaldo
> > >

-- 
Thanks,
Changbin Du

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Jiri Olsa]

On Wed, Apr 05, 2017 at 10:44:22AM +0800, Du, Changbin wrote:
> On Tue, Apr 04, 2017 at 12:51:03PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu:
> > > Hi Arnaldo,
> > > 
> > > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
> > > <acme@kernel.org> wrote:
> > > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> > > >> From: Changbin Du <changbin.du@intel.com>
> > > >>
> > > >> Some perf_hpp_fmt both registered at field and sort list. For such
> > > >> instance, we only can free it when removed from the both lists. This
> > > >> function currently only used by self-test code, but still should fix
> > > >> it.
> > > >
> > > > Looks sane, applying,
> > > >
> > > > Jiri, Namhyung, please holler (or ack) if needed,
> > > 
> > > Did you actually see the double free problem?  AFAICS the old code
> > 
> > I assumed that he had seen it, in some self-test code, Changbin, can you
> > please show command output or further describe when this patch would be
> > necessary?
> > 
> Arnaldo, I did observe this issue but not in self-test code. The self-test code
> uses that function but does not have a case that a fmt linked to two both list. 
> I found this issue when I try to add 'dynamic sort' feature to perf, which
> I use this function to reset out fields.

could you post it with the rest of your patches? might be easier to review

thanks,
jirka

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Du, Changbin]

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

On Sun, Apr 09, 2017 at 07:05:52PM +0200, Jiri Olsa wrote:
> On Wed, Apr 05, 2017 at 10:44:22AM +0800, Du, Changbin wrote:
> > On Tue, Apr 04, 2017 at 12:51:03PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu:
> > > > Hi Arnaldo,
> > > > 
> > > > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
> > > > <acme@kernel.org> wrote:
> > > > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu:
> > > > >> From: Changbin Du <changbin.du@intel.com>
> > > > >>
> > > > >> Some perf_hpp_fmt both registered at field and sort list. For such
> > > > >> instance, we only can free it when removed from the both lists. This
> > > > >> function currently only used by self-test code, but still should fix
> > > > >> it.
> > > > >
> > > > > Looks sane, applying,
> > > > >
> > > > > Jiri, Namhyung, please holler (or ack) if needed,
> > > > 
> > > > Did you actually see the double free problem?  AFAICS the old code
> > > 
> > > I assumed that he had seen it, in some self-test code, Changbin, can you
> > > please show command output or further describe when this patch would be
> > > necessary?
> > > 
> > Arnaldo, I did observe this issue but not in self-test code. The self-test code
> > uses that function but does not have a case that a fmt linked to two both list. 
> > I found this issue when I try to add 'dynamic sort' feature to perf, which
> > I use this function to reset out fields.
> 
> could you post it with the rest of your patches? might be easier to review
>
jirka, I am sorry that the 'dynamic sort' is only half done. Now I am
very busy with work at hand. I will send the rest of patches when I
finish them. Could we check out this fix first?

> thanks,
> jirka

-- 
Thanks,
Changbin Du

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Jiri Olsa]

On Tue, Apr 04, 2017 at 12:19:40PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > ---
> >  tools/perf/ui/hist.c | 25 +++++++++++++++----------
> >  1 file changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index 5d632dc..f94b301 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> >  
> >  void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> >  {
> > -	struct perf_hpp_fmt *fmt, *tmp;
> > +	struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
> >  
> >  	/* reset output fields */
> > -	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > -		list_del_init(&fmt->list);
> > -		list_del_init(&fmt->sort_list);
> > -		fmt_free(fmt);
> > +	perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> > +		list_del_init(&field_fmt->list);
> > +		/* reset sort keys */
> > +		perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
> > +			if (field_fmt == sort_fmt) {
> > +				list_del_init(&field_fmt->sort_list);
> > +				break;
> > +			}
> > +		}

I agree with Namhyung in here.. seems like the only thing you
added is to check if the field_fmt was also linked in as a sort
entry before you call list_del_init on it

which I think should be also done with list_empty function, but
more importantly I dont see a reason for that.. list_del_init
call should be fine on empty list

please describe the issue in more details, perhaps we'ew missing
something

jirka

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Du, Changbin]

[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]

On Mon, Apr 10, 2017 at 10:39:50AM +0200, Jiri Olsa wrote:
> On Tue, Apr 04, 2017 at 12:19:40PM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > > ---
> > >  tools/perf/ui/hist.c | 25 +++++++++++++++----------
> > >  1 file changed, 15 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > index 5d632dc..f94b301 100644
> > > --- a/tools/perf/ui/hist.c
> > > +++ b/tools/perf/ui/hist.c
> > > @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> > >  
> > >  void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> > >  {
> > > -	struct perf_hpp_fmt *fmt, *tmp;
> > > +	struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
> > >  
> > >  	/* reset output fields */
> > > -	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > -		list_del_init(&fmt->list);
> > > -		list_del_init(&fmt->sort_list);
> > > -		fmt_free(fmt);
> > > +	perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> > > +		list_del_init(&field_fmt->list);
> > > +		/* reset sort keys */
> > > +		perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
> > > +			if (field_fmt == sort_fmt) {
> > > +				list_del_init(&field_fmt->sort_list);
> > > +				break;
> > > +			}
> > > +		}
> 
> I agree with Namhyung in here.. seems like the only thing you
> added is to check if the field_fmt was also linked in as a sort
> entry before you call list_del_init on it
>
This is correct.

> which I think should be also done with list_empty function, but
> more importantly I dont see a reason for that.. list_del_init
> call should be fine on empty list
> 
You didn't catch the problem here. The problem is double free a fmt.
For exampe, fmt A is linked to both list. Then it will be first free
by the first iteration over list, then it will be freed again at the
second iteration over sort_list. This must cause application crash.

> please describe the issue in more details, perhaps we'ew missing
> something
> 
> jirka

-- 
Thanks,
Changbin Du

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Jiri Olsa]

On Mon, Apr 10, 2017 at 06:21:12PM +0800, Du, Changbin wrote:
> On Mon, Apr 10, 2017 at 10:39:50AM +0200, Jiri Olsa wrote:
> > On Tue, Apr 04, 2017 at 12:19:40PM -0300, Arnaldo Carvalho de Melo wrote:
> > 
> > SNIP
> > 
> > > > ---
> > > >  tools/perf/ui/hist.c | 25 +++++++++++++++----------
> > > >  1 file changed, 15 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > > index 5d632dc..f94b301 100644
> > > > --- a/tools/perf/ui/hist.c
> > > > +++ b/tools/perf/ui/hist.c
> > > > @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> > > >  
> > > >  void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> > > >  {
> > > > -	struct perf_hpp_fmt *fmt, *tmp;
> > > > +	struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
> > > >  
> > > >  	/* reset output fields */
> > > > -	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > > -		list_del_init(&fmt->list);
> > > > -		list_del_init(&fmt->sort_list);
> > > > -		fmt_free(fmt);
> > > > +	perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> > > > +		list_del_init(&field_fmt->list);
> > > > +		/* reset sort keys */
> > > > +		perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
> > > > +			if (field_fmt == sort_fmt) {
> > > > +				list_del_init(&field_fmt->sort_list);
> > > > +				break;
> > > > +			}
> > > > +		}
> > 
> > I agree with Namhyung in here.. seems like the only thing you
> > added is to check if the field_fmt was also linked in as a sort
> > entry before you call list_del_init on it
> >
> This is correct.
> 
> > which I think should be also done with list_empty function, but
> > more importantly I dont see a reason for that.. list_del_init
> > call should be fine on empty list
> > 
> You didn't catch the problem here. The problem is double free a fmt.
> For exampe, fmt A is linked to both list. Then it will be first free
> by the first iteration over list, then it will be freed again at the
> second iteration over sort_list. This must cause application crash.

the original code takes it out of both lists,
so the next itaration won't go over that entry

jirka

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Du, Changbin]

[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]

On Mon, Apr 10, 2017 at 01:33:25PM +0200, Jiri Olsa wrote:
> On Mon, Apr 10, 2017 at 06:21:12PM +0800, Du, Changbin wrote:
> > On Mon, Apr 10, 2017 at 10:39:50AM +0200, Jiri Olsa wrote:
> > > On Tue, Apr 04, 2017 at 12:19:40PM -0300, Arnaldo Carvalho de Melo wrote:
> > > 
> > > SNIP
> > > 
> > > > > ---
> > > > >  tools/perf/ui/hist.c | 25 +++++++++++++++----------
> > > > >  1 file changed, 15 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > > > index 5d632dc..f94b301 100644
> > > > > --- a/tools/perf/ui/hist.c
> > > > > +++ b/tools/perf/ui/hist.c
> > > > > @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> > > > >  
> > > > >  void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> > > > >  {
> > > > > -	struct perf_hpp_fmt *fmt, *tmp;
> > > > > +	struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
> > > > >  
> > > > >  	/* reset output fields */
> > > > > -	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > > > -		list_del_init(&fmt->list);
> > > > > -		list_del_init(&fmt->sort_list);
> > > > > -		fmt_free(fmt);
> > > > > +	perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> > > > > +		list_del_init(&field_fmt->list);
> > > > > +		/* reset sort keys */
> > > > > +		perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
> > > > > +			if (field_fmt == sort_fmt) {
> > > > > +				list_del_init(&field_fmt->sort_list);
> > > > > +				break;
> > > > > +			}
> > > > > +		}
> > > 
> > > I agree with Namhyung in here.. seems like the only thing you
> > > added is to check if the field_fmt was also linked in as a sort
> > > entry before you call list_del_init on it
> > >
> > This is correct.
> > 
> > > which I think should be also done with list_empty function, but
> > > more importantly I dont see a reason for that.. list_del_init
> > > call should be fine on empty list
> > > 
> > You didn't catch the problem here. The problem is double free a fmt.
> > For exampe, fmt A is linked to both list. Then it will be first free
> > by the first iteration over list, then it will be freed again at the
> > second iteration over sort_list. This must cause application crash.
> 
> the original code takes it out of both lists,
> so the next itaration won't go over that entry
>
oh, my bad, my desc is wrong. I replayed the crash. The problem is
list_del_init a unlinked entry.

perf: Segmentation fault
-------- backtrace --------
./perf[0x57394b]
/lib/x86_64-linux-gnu/libc.so.6(+0x354b0)[0x7fb8da3034b0]
./perf(perf_hpp__reset_output_field+0xb7)[0x55dfe7]
./perf(hists__sort_by_fields+0x3d7)[0x509777]
./perf[0x5704c1]
./perf(perf_evlist__tui_browse_hists+0x2e5)[0x5723e5]
./perf(cmd_report+0x1a9b)[0x43b4fb]
./perf[0x494731]
./perf(main+0x704)[0x426304]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7fb8da2ee830]
./perf(_start+0x29)[0x4263f9]
[0x0]

(gdb) print fmt.list
$4 = {next = 0x100, prev = 0x200}    // LIST_POISON
(gdb) print fmt.sort_list
$5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}

In this case, the fmt is linked in sort_list, but not in list. So crash
at the list_del_init(&fmt->list) of second loop.

Another potential case is the fmt is linked in list, but not in sort_list.

Oh, my brain was broken. correct patch but wrong commit message. :(
Will drop this one and submit a new one.

> jirka

-- 
Thanks,
Changbin Du

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Jiri Olsa]

On Tue, Apr 11, 2017 at 11:06:14AM +0800, Du, Changbin wrote:

SNIP

> > the original code takes it out of both lists,
> > so the next itaration won't go over that entry
> >
> oh, my bad, my desc is wrong. I replayed the crash. The problem is
> list_del_init a unlinked entry.
> 
> perf: Segmentation fault
> -------- backtrace --------
> ./perf[0x57394b]
> /lib/x86_64-linux-gnu/libc.so.6(+0x354b0)[0x7fb8da3034b0]
> ./perf(perf_hpp__reset_output_field+0xb7)[0x55dfe7]
> ./perf(hists__sort_by_fields+0x3d7)[0x509777]
> ./perf[0x5704c1]
> ./perf(perf_evlist__tui_browse_hists+0x2e5)[0x5723e5]
> ./perf(cmd_report+0x1a9b)[0x43b4fb]
> ./perf[0x494731]
> ./perf(main+0x704)[0x426304]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7fb8da2ee830]
> ./perf(_start+0x29)[0x4263f9]
> [0x0]
> 
> (gdb) print fmt.list
> $4 = {next = 0x100, prev = 0x200}    // LIST_POISON
> (gdb) print fmt.sort_list
> $5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}
> 
> In this case, the fmt is linked in sort_list, but not in list. So crash
> at the list_del_init(&fmt->list) of second loop.

so the only place I can see the POISON could get there
is in perf_hpp__column_unregister.. can't we just get
rid of it like below

jirka


---
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 5d632dca672a..7577effbf746 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -529,7 +529,7 @@ void perf_hpp_list__prepend_sort_field(struct perf_hpp_list *list,
 
 void perf_hpp__column_unregister(struct perf_hpp_fmt *format)
 {
-	list_del(&format->list);
+	list_del_init(&format->list);
 }
 
 void perf_hpp__cancel_cumulate(void)

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Du, Changbin]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

> > (gdb) print fmt.sort_list
> > $5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}
> > 
> > In this case, the fmt is linked in sort_list, but not in list. So crash
> > at the list_del_init(&fmt->list) of second loop.
> 
> so the only place I can see the POISON could get there
> is in perf_hpp__column_unregister.. can't we just get
> rid of it like below
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5d632dca672a..7577effbf746 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -529,7 +529,7 @@ void perf_hpp_list__prepend_sort_field(struct perf_hpp_list *list,
>  
>  void perf_hpp__column_unregister(struct perf_hpp_fmt *format)
>  {
> -	list_del(&format->list);
> +	list_del_init(&format->list);
>  }
>  
yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
No rule rather than create one.

But anyway, both are ok for me. What's your options?

>  void perf_hpp__cancel_cumulate(void)

-- 
Thanks,
Changbin Du

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Jiri Olsa]

On Tue, Apr 11, 2017 at 04:25:50PM +0800, Du, Changbin wrote:
> > > (gdb) print fmt.sort_list
> > > $5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}
> > > 
> > > In this case, the fmt is linked in sort_list, but not in list. So crash
> > > at the list_del_init(&fmt->list) of second loop.
> > 
> > so the only place I can see the POISON could get there
> > is in perf_hpp__column_unregister.. can't we just get
> > rid of it like below
> > 
> > jirka
> > 
> > 
> > ---
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index 5d632dca672a..7577effbf746 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -529,7 +529,7 @@ void perf_hpp_list__prepend_sort_field(struct perf_hpp_list *list,
> >  
> >  void perf_hpp__column_unregister(struct perf_hpp_fmt *format)
> >  {
> > -	list_del(&format->list);
> > +	list_del_init(&format->list);
> >  }
> >  
> yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> No rule rather than create one.
> 
> But anyway, both are ok for me. What's your options?

hum, also I dont think we need to touch that bit at all
if we are going to remove it right away.. how about the
change below?

jirka


---
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 5d632dca672a..0ee7db43dd7d 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -613,15 +613,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list *list)
 
 	/* reset output fields */
 	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
-		list_del_init(&fmt->list);
-		list_del_init(&fmt->sort_list);
+		list_del(&fmt->list);
+		/* Remove the fmt from next loop processing. */
+		list_del(&fmt->sort_list);
 		fmt_free(fmt);
 	}
 
 	/* reset sort keys */
 	perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
-		list_del_init(&fmt->list);
-		list_del_init(&fmt->sort_list);
+		list_del(&fmt->sort_list);
 		fmt_free(fmt);
 	}
 }

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Du, Changbin]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

> > >  
> > yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> > No rule rather than create one.
> > 
> > But anyway, both are ok for me. What's your options?
> 
> hum, also I dont think we need to touch that bit at all
> if we are going to remove it right away.. how about the
> change below?
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5d632dca672a..0ee7db43dd7d 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -613,15 +613,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list *list)
>  
>  	/* reset output fields */
>  	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> -		list_del_init(&fmt->list);
> -		list_del_init(&fmt->sort_list);
> +		list_del(&fmt->list);
> +		/* Remove the fmt from next loop processing. */
> +		list_del(&fmt->sort_list);
>  		fmt_free(fmt);
What if the fmt is not linked to sort_list? I see it is possible (please
checking perf_hpp__setup_output_field()). I am not sure if we really has
sunch case currently, just concern :)

>  	}
>  
>  	/* reset sort keys */
>  	perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
> -		list_del_init(&fmt->list);
> -		list_del_init(&fmt->sort_list);
> +		list_del(&fmt->sort_list);
>  		fmt_free(fmt);
>  	}
>  }

-- 
Thanks,
Changbin Du

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Jiri Olsa]

On Tue, Apr 11, 2017 at 06:13:17PM +0800, Du, Changbin wrote:
> > > >  
> > > yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> > > No rule rather than create one.
> > > 
> > > But anyway, both are ok for me. What's your options?
> > 
> > hum, also I dont think we need to touch that bit at all
> > if we are going to remove it right away.. how about the
> > change below?
> > 
> > jirka
> > 
> > 
> > ---
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index 5d632dca672a..0ee7db43dd7d 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -613,15 +613,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> >  
> >  	/* reset output fields */
> >  	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > -		list_del_init(&fmt->list);
> > -		list_del_init(&fmt->sort_list);
> > +		list_del(&fmt->list);
> > +		/* Remove the fmt from next loop processing. */
> > +		list_del(&fmt->sort_list);
> >  		fmt_free(fmt);
> What if the fmt is not linked to sort_list? I see it is possible (please
> checking perf_hpp__setup_output_field()). I am not sure if we really has
> sunch case currently, just concern :)

if it's not linked to sort_list, then sort_list is initialized
and list_del should do no harm

jirka

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Du, Changbin]

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

On Tue, Apr 11, 2017 at 12:32:49PM +0200, Jiri Olsa wrote:
> On Tue, Apr 11, 2017 at 06:13:17PM +0800, Du, Changbin wrote:
> > > > >  
> > > > yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> > > > No rule rather than create one.
> > > > 
> > > > But anyway, both are ok for me. What's your options?
> > > 
> > > hum, also I dont think we need to touch that bit at all
> > > if we are going to remove it right away.. how about the
> > > change below?
> > > 
> > > jirka
> > > 
> > > 
> > > ---
> > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > index 5d632dca672a..0ee7db43dd7d 100644
> > > --- a/tools/perf/ui/hist.c
> > > +++ b/tools/perf/ui/hist.c
> > > @@ -613,15 +613,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> > >  
> > >  	/* reset output fields */
> > >  	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > -		list_del_init(&fmt->list);
> > > -		list_del_init(&fmt->sort_list);
> > > +		list_del(&fmt->list);
> > > +		/* Remove the fmt from next loop processing. */
> > > +		list_del(&fmt->sort_list);
> > >  		fmt_free(fmt);
> > What if the fmt is not linked to sort_list? I see it is possible (please
> > checking perf_hpp__setup_output_field()). I am not sure if we really has
> > sunch case currently, just concern :)
> 
> if it's not linked to sort_list, then sort_list is initialized
> and list_del should do no harm
> 
ok, then it's fine if you insist.

> jirka

-- 
Thanks,
Changbin Du

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Du, Changbin]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]


Hi jirka, Will you send a patch to fix this issue? If not I will send my
solution in a new thread.

I have given up to add 'dynamic sort' feature since my code is not working
and I am engaged in other things. I still hope this fix can be picked up.
Thanks!

On Wed, Apr 12, 2017 at 09:48:08AM +0800, Du, Changbin wrote:
> On Tue, Apr 11, 2017 at 12:32:49PM +0200, Jiri Olsa wrote:
> > On Tue, Apr 11, 2017 at 06:13:17PM +0800, Du, Changbin wrote:
> > > > > >  
> > > > > yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
> > > > > No rule rather than create one.
> > > > > 
> > > > > But anyway, both are ok for me. What's your options?
> > > > 
> > > > hum, also I dont think we need to touch that bit at all
> > > > if we are going to remove it right away.. how about the
> > > > change below?
> > > > 
> > > > jirka
> > > > 
> > > > 
> > > > ---
> > > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > > index 5d632dca672a..0ee7db43dd7d 100644
> > > > --- a/tools/perf/ui/hist.c
> > > > +++ b/tools/perf/ui/hist.c
> > > > @@ -613,15 +613,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> > > >  
> > > >  	/* reset output fields */
> > > >  	perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > > -		list_del_init(&fmt->list);
> > > > -		list_del_init(&fmt->sort_list);
> > > > +		list_del(&fmt->list);
> > > > +		/* Remove the fmt from next loop processing. */
> > > > +		list_del(&fmt->sort_list);
> > > >  		fmt_free(fmt);
> > > What if the fmt is not linked to sort_list? I see it is possible (please
> > > checking perf_hpp__setup_output_field()). I am not sure if we really has
> > > sunch case currently, just concern :)
> > 
> > if it's not linked to sort_list, then sort_list is initialized
> > and list_del should do no harm
> > 
> ok, then it's fine if you insist.
> 
> > jirka
> 
> -- 
> Thanks,
> Changbin Du



-- 
Thanks,
Changbin Du

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

[Jiri Olsa]

On Wed, May 31, 2017 at 03:05:51PM +0800, Du, Changbin wrote:
> 
> Hi jirka, Will you send a patch to fix this issue? If not I will send my
> solution in a new thread.

oops, forgot about this one.. I'll pick it up

thanks,
jirka