linux-kernel - Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Apr 2017 00:34:59 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Jiri Olsa <jolsa@...nel.org>, changbin.du@...el.com,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field

Hi Arnaldo,

On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
> Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@...el.com escreveu:
>> From: Changbin Du <changbin.du@...el.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@...el.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