Re: [libvirt] [dbus PATCH v2 1/3] Implement FSFreeze method for Domain Interface

On Wed, Apr 18, 2018 at 01:52:17PM +0200, Katerina Koukiou wrote: > Signed-off-by: Katerina Koukiou <kkoukiou(a)redhat.com&gt; > --- > data/org.libvirt.Domain.xml | 7 +++++++ > src/domain.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/src/domain.c b/src/domain.c > index 5e59094..9a6ff1d 100644 > --- a/src/domain.c > +++ b/src/domain.c > @@ -677,6 +677,46 @@ virtDBusDomainDetachDevice(GVariant *inArgs, > virtDBusUtilSetLastVirtError(error); > } > > +static void > +virtDBusDomainFSFreeze(GVariant *inArgs, > + GUnixFDList *inFDs G_GNUC_UNUSED, > + const gchar *objectPath, > + gpointer userData, > + GVariant **outArgs, > + GUnixFDList **outFDs G_GNUC_UNUSED, > + GError **error) > +{ > + virtDBusConnect *connect = userData; > + g_autoptr(virDomain) domain = NULL; > + g_autofree const gchar **mountpoints = NULL; > + const gchar **tmp; > + GVariantIter *iter; > + gsize nmountpoints = 0; > + guint flags; > + gint ret; > + > + g_variant_get(inArgs, "(asu)", &iter, &flags); > + > + nmountpoints = g_variant_iter_n_children(iter); > + if (nmountpoints > 0) { > + mountpoints = g_new0(const gchar*, nmountpoints); > + tmp = mountpoints; > + while (g_variant_iter_loop(iter, "&s", tmp)) g_variant_iter_loop seems to access tmp even if it returns 0. For an array with two strings, valgrind reports: ==27339== Invalid read of size 8 ==27339== at 0x54B2CB3: g_variant_valist_get (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x54B4AAB: g_variant_iter_loop (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x407AD1: virtDBusDomainFSFreeze (domain.c:720) ==27339== by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224) ==27339== by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263) ==27339== by 0x54A146F: g_thread_pool_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x54A0AA4: g_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x611E636: start_thread (in /lib64/libpthread-2.25.so) ==27339== by 0x642FBCE: clone (in /lib64/libc-2.25.so) ==27339== Address 0x1064f980 is 0 bytes after a block of size 16 alloc'd ==27339== at 0x4C2CEE6: calloc (vg_replace_malloc.c:711) ==27339== by 0x547EAF0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x407AAA: virtDBusDomainFSFreeze (domain.c:718) ==27339== by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224) ==27339== by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263) ==27339== Invalid write of size 8 ==27339== at 0x54B2CF0: g_variant_valist_get (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x54B4AAB: g_variant_iter_loop (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x407AD1: virtDBusDomainFSFreeze (domain.c:720) ==27339== by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224) ==27339== by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263) ==27339== by 0x54A146F: g_thread_pool_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x54A0AA4: g_thread_proxy (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x611E636: start_thread (in /lib64/libpthread-2.25.so) ==27339== by 0x642FBCE: clone (in /lib64/libc-2.25.so) ==27339== Address 0x1064f980 is 0 bytes after a block of size 16 alloc'd ==27339== at 0x4C2CEE6: calloc (vg_replace_malloc.c:711) ==27339== by 0x547EAF0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5200.3) ==27339== by 0x407AAA: virtDBusDomainFSFreeze (domain.c:718) ==27339== by 0x40B723: virtDBusGDBusHandleMethod (gdbus.c:224) ==27339== by 0x40B723: virtDBusGDBusMethodCallThread (gdbus.c:263) So yes, you should allocate nmountpoints + 1: https://www.redhat.com/archives/libvir-list/2018-April/msg01647.html

> + tmp++; > + g_variant_iter_free(iter); This should be moved after the if (n > 0) condition. g_variant_get initialized the iterator even if the string array has zero elements.