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

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Wed, Apr 18, 2018 at 01:52:17PM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou <kkoukiou@xxxxxxxxxx>
> ---
> 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.

> +    }
> +
> +    domain = virtDBusDomainGetVirDomain(connect, objectPath, error);
> +    if (!domain)
> +        return;
> +
> +    ret = virDomainFSFreeze(domain, mountpoints, nmountpoints, flags);
> +    if (ret < 0)
> +        return virtDBusUtilSetLastVirtError(error);
> +
> +    *outArgs = g_variant_new("(u)", ret);
> +}
> +
> static void
> virtDBusDomainFSTrim(GVariant *inArgs,
>                      GUnixFDList *inFDs G_GNUC_UNUSED,

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list