lkml.org 
[lkml]   [2017]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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]
    \
     
     \ /
      Last update: 2017-04-11 05:09    [W:4.145 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site