[Spice-devel] [PATCH spice-gtk 1/2] usbredir: Check for existing usb channels after libusb init

[Marc-André Lureau]

ack to both

On Wed, Mar 14, 2012 at 3:27 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> Currently trying to view a usbredir enabled vm from virt-manager causes
> virt-manager to crash. This crash is caused by the following happening:
>
> -virt-manager sets up the session, including connecting all the channels
> -a spice-gtk internal code path calls spice_usb_device_manager_get()
> -spice_usb_device_manager_get calls channel_new on all already connected
>  usb channels
> -channel_new does:
>  spice_usbredir_channel_set_context(SPICE_USBREDIR_CHANNEL(channel),
>                                    self->priv->context);
> -But self->priv->context has not been set yet (so is NULL) -> segfault!
>
> This patch fixes this by moving the iterating over already connected
> usb channels to after the setting of self->priv->context. Note this means
> that the channels will no longer get checked when there is no USB_REDIR
> support. That is not a problem since spice_usb_device_manager_initable_init
> will return FALSE in that case anyways.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  gtk/usb-device-manager.c |   45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index 29e9b4c..14b60c9 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -62,13 +62,6 @@
>  */
>
>  /* ------------------------------------------------------------------ */
> -/* Prototypes for private functions */
> -static void channel_new(SpiceSession *session, SpiceChannel *channel,
> -                        gpointer user_data);
> -static void channel_destroy(SpiceSession *session, SpiceChannel *channel,
> -                            gpointer user_data);
> -
> -/* ------------------------------------------------------------------ */
>  /* gobject glue                                                       */
>
>  #define SPICE_USB_DEVICE_MANAGER_GET_PRIVATE(obj)                                  \
> @@ -109,6 +102,10 @@ struct _SpiceUsbDeviceManagerPrivate {
>  };
>
>  #ifdef USE_USBREDIR
> +static void channel_new(SpiceSession *session, SpiceChannel *channel,
> +                        gpointer user_data);
> +static void channel_destroy(SpiceSession *session, SpiceChannel *channel,
> +                            gpointer user_data);
>  static void spice_usb_device_manager_uevent_cb(GUdevClient     *client,
>                                                const gchar     *action,
>                                                GUdevDevice     *udevice,
> @@ -148,11 +145,11 @@ static gboolean spice_usb_device_manager_initable_init(GInitable  *initable,
>                                                     GCancellable  *cancellable,
>                                                     GError        **err)
>  {
> -    GList *list;
> -    GList *it;
>     SpiceUsbDeviceManager *self;
>     SpiceUsbDeviceManagerPrivate *priv;
>  #ifdef USE_USBREDIR
> +    GList *list;
> +    GList *it;
>     int rc;
>     const gchar *const subsystems[] = {"usb", NULL};
>  #endif
> @@ -175,17 +172,8 @@ static gboolean spice_usb_device_manager_initable_init(GInitable  *initable,
>         return FALSE;
>     }
>
> -    g_signal_connect(priv->session, "channel-new",
> -                     G_CALLBACK(channel_new), self);
> -    g_signal_connect(priv->session, "channel-destroy",
> -                     G_CALLBACK(channel_destroy), self);
> -    list = spice_session_get_channels(priv->session);
> -    for (it = g_list_first(list); it != NULL; it = g_list_next(it)) {
> -        channel_new(priv->session, it->data, (gpointer*)self);
> -    }
> -    g_list_free(list);
> -
>  #ifdef USE_USBREDIR
> +    /* Initialize libusb */
>     rc = libusb_init(&priv->context);
>     if (rc < 0) {
>         const char *desc = spice_usbutil_libusb_strerror(rc);
> @@ -195,6 +183,18 @@ static gboolean spice_usb_device_manager_initable_init(GInitable  *initable,
>         return FALSE;
>     }
>
> +    /* Start listening for usb channels connect/disconnect */
> +    g_signal_connect(priv->session, "channel-new",
> +                     G_CALLBACK(channel_new), self);
> +    g_signal_connect(priv->session, "channel-destroy",
> +                     G_CALLBACK(channel_destroy), self);
> +    list = spice_session_get_channels(priv->session);
> +    for (it = g_list_first(list); it != NULL; it = g_list_next(it)) {
> +        channel_new(priv->session, it->data, (gpointer*)self);
> +    }
> +    g_list_free(list);
> +
> +    /* Start listening for usb devices plug / unplug */
>     priv->udev = g_udev_client_new(subsystems);
>     g_signal_connect(G_OBJECT(priv->udev), "uevent",
>                      G_CALLBACK(spice_usb_device_manager_uevent_cb), self);
> @@ -460,10 +460,11 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>     g_type_class_add_private(klass, sizeof(SpiceUsbDeviceManagerPrivate));
>  }
>
> +#ifdef USE_USBREDIR
> +
>  /* ------------------------------------------------------------------ */
>  /* gudev / libusb Helper functions                                    */
>
> -#ifdef USE_USBREDIR
>  static gboolean spice_usb_device_manager_get_udev_bus_n_address(
>     GUdevDevice *udev, int *bus, int *address)
>  {
> @@ -480,7 +481,6 @@ static gboolean spice_usb_device_manager_get_udev_bus_n_address(
>
>     return *bus && *address;
>  }
> -#endif
>
>  /* ------------------------------------------------------------------ */
>  /* callbacks                                                          */
> @@ -491,11 +491,9 @@ static void channel_new(SpiceSession *session, SpiceChannel *channel,
>     SpiceUsbDeviceManager *self = user_data;
>
>     if (SPICE_IS_USBREDIR_CHANNEL(channel)) {
> -#ifdef USE_USBREDIR
>         spice_usbredir_channel_set_context(SPICE_USBREDIR_CHANNEL(channel),
>                                            self->priv->context);
>         spice_channel_connect(channel);
> -#endif
>         g_ptr_array_add(self->priv->channels, channel);
>     }
>  }
> @@ -509,7 +507,6 @@ static void channel_destroy(SpiceSession *session, SpiceChannel *channel,
>         g_ptr_array_remove(self->priv->channels, channel);
>  }
>
> -#ifdef USE_USBREDIR
>  static void spice_usb_device_manager_auto_connect_cb(GObject      *gobject,
>                                                      GAsyncResult *res,
>                                                      gpointer      user_data)
> --
> 1.7.9.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau