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

[Marcel Apfelbaum]

On 07/21/2016 06:48 PM, Marc-André Lureau wrote:
> 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)

I am sure the leak is gone :), but the devices attached to PXB still work?
I'll test it and get back to you.

Thanks,
Marcel



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