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

[Marcel Apfelbaum]

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.

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?


Thanks,
Marcel

>   }
>
>   /*