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@xxxxxxxxxx> wrote:
> Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@xxxxxxxxx escreveu:
>> From: Changbin Du <changbin.du@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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