Re: [Qemu-devel] [PATCH 18/37] acpi-build: fix array leak

[Marc-André Lureau]

Hi

On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum
<address@hidden> wrote:
> On 07/19/2016 11:54 AM, address@hidden wrote:
>>
>> From: Marc-André Lureau <address@hidden>
>>
>> The free_ranges array is used as a temporary pointer array, the segment
>> should still be freed,
>
>
> Right. If I understand, this is the leak.
>
>  however, it shouldn't free the elements themself.
>
> And it didn't, right? otherwise it would not work since these ranges
> are used later.
>
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>   hw/i386/acpi-build.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index fbba461..f4ba3a4 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -761,7 +761,7 @@ static gint crs_range_compare(gconstpointer a,
>> gconstpointer b)
>>   static void crs_replace_with_free_ranges(GPtrArray *ranges,
>>                                            uint64_t start, uint64_t end)
>>   {
>> -    GPtrArray *free_ranges =
>> g_ptr_array_new_with_free_func(crs_range_free);
>> +    GPtrArray *free_ranges = g_ptr_array_new();
>
>
> Indeed, we are not going to free the ranges in this array, adding the
> GDestroyNotify
> here is not needed.
>
>>       uint64_t free_base = start;
>>       int i;
>>
>> @@ -785,7 +785,7 @@ static void crs_replace_with_free_ranges(GPtrArray
>> *ranges,
>>           g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i));
>>       }
>>
>> -    g_ptr_array_free(free_ranges, false);
>> +    g_ptr_array_free(free_ranges, true);
>
>
> This *is* scary since "true" means delete everything, but looking at
> documentation:
>     "If array contents point to dynamically-allocated memory,
>      they should be freed separately if free_seg is TRUE and
>      no GDestroyNotify function has been set for array."
> So your approach should work.
>
> I think I understand the leak. Previous approach deleted the GArray wrapper,
> preserved the pointers (which we need), but also the inner array which we
> don't.

yes, it's only the inner array we need to free.

>
> One question: how did you test that it still works :) ?
> Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device
> e1000,bus=pxb and see the device
> e100 device gets the required resources?

If you run this under it valgrind you get, after the patch the leak is gone:

==20313== 32 bytes in 1 blocks are definitely lost in loss record
5,326 of 10,190
==20313==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==20313==    by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.1)
==20313==    by 0x1E181D42: g_slice_alloc (in
/usr/lib64/libglib-2.0.so.0.4800.1)
==20313==    by 0x1E139880: g_ptr_array_sized_new (in
/usr/lib64/libglib-2.0.so.0.4800.1)
==20313==    by 0x1E13991D: g_ptr_array_new_with_free_func (in
/usr/lib64/libglib-2.0.so.0.4800.1)
==20313==    by 0x3E8BE8: crs_range_merge (acpi-build.c:797)
==20313==    by 0x3E8FE6: build_crs (acpi-build.c:918)
==20313==    by 0x3ED857: build_dsdt (acpi-build.c:2014)
==20313==    by 0x3EF659: acpi_build (acpi-build.c:2590)
==20313==    by 0x3EFE61: acpi_setup (acpi-build.c:2793)
==20313==    by 0x3D810D: pc_machine_done (pc.c:1270)
==20313==    by 0x7E6A23: notifier_list_notify (notify.c:40)



>
>
> Thanks,
> Marcel
>
>>   }
>>
>>   /*
>>
>
>



-- 
Marc-André Lureau