Messages in this thread | | | Date | Tue, 11 Apr 2017 11:06:14 +0800 | From | "Du, Changbin" <> | Subject | Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field |
| |
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 [unhandled content-type:application/pgp-signature] | |