From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34168) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQGDB-0006ro-2W for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:48:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQGD8-0006F1-Ql for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:48:31 -0400 Received: from mail-vk0-x243.google.com ([2607:f8b0:400c:c05::243]:32977) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQGD8-0006Ex-Ju for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:48:30 -0400 Received: by mail-vk0-x243.google.com with SMTP id s189so7054650vkh.0 for ; Thu, 21 Jul 2016 08:48:30 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5790E196.1030203@gmail.com> References: <20160719085432.4572-1-marcandre.lureau@redhat.com> <20160719085432.4572-19-marcandre.lureau@redhat.com> <5790E196.1030203@gmail.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Thu, 21 Jul 2016 19:48:29 +0400 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 18/37] acpi-build: fix array leak List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: QEMU Hi On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum wrote: > On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote: >> >> From: Marc-Andr=C3=A9 Lureau >> >> 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=C3=A9 Lureau >> --- >> 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 =3D >> g_ptr_array_new_with_free_func(crs_range_free); >> + GPtrArray *free_ranges =3D 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 =3D 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 wrapp= er, > 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=3Dpxb,bus_nr=3D0x80,bus=3Dpci.0= -device > e1000,bus=3Dpxb 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= : =3D=3D20313=3D=3D 32 bytes in 1 blocks are definitely lost in loss record 5,326 of 10,190 =3D=3D20313=3D=3D at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) =3D=3D20313=3D=3D by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.= 0.4800.1) =3D=3D20313=3D=3D by 0x1E181D42: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.4800.1) =3D=3D20313=3D=3D by 0x1E139880: g_ptr_array_sized_new (in /usr/lib64/libglib-2.0.so.0.4800.1) =3D=3D20313=3D=3D by 0x1E13991D: g_ptr_array_new_with_free_func (in /usr/lib64/libglib-2.0.so.0.4800.1) =3D=3D20313=3D=3D by 0x3E8BE8: crs_range_merge (acpi-build.c:797) =3D=3D20313=3D=3D by 0x3E8FE6: build_crs (acpi-build.c:918) =3D=3D20313=3D=3D by 0x3ED857: build_dsdt (acpi-build.c:2014) =3D=3D20313=3D=3D by 0x3EF659: acpi_build (acpi-build.c:2590) =3D=3D20313=3D=3D by 0x3EFE61: acpi_setup (acpi-build.c:2793) =3D=3D20313=3D=3D by 0x3D810D: pc_machine_done (pc.c:1270) =3D=3D20313=3D=3D by 0x7E6A23: notifier_list_notify (notify.c:40) > > > Thanks, > Marcel > >> } >> >> /* >> > > --=20 Marc-Andr=C3=A9 Lureau