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

["Du, Changbin" <changbin.du@intel.com>]

[-- 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 --]