[Spice-devel] [PATCH] Avoid passing libusb functions as callbacks

[Frediano Ziglio]

> 
> Hi,
> On Fri, 2018-08-17 at 15:24 +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Fri, Aug 17, 2018 at 03:12:35PM +0200, jorge.olmos at flexvdi.com
> > wrote:
> > > From: Jorge Olmos <jorge.olmos at flexvdi.com>
> > > 
> > > When building spice-gtk for windows:
> > > - libusb uses __stdcall calling convention when compiled for win32.
> > > It does
> > > not include an option to be compiled with __cdecl calling
> > > convention.
> > > Directly calling libusb functions works fine. But it is a problem
> > > when its
> > > functions are passed as callbacks to a function that expects other
> > > calling
> > > convention.
> > > - glib uses __cdecl calling convention and expects the functions it
> > > receives as parameters to follow __cdecl convention.
> > > 
> > > So the lines included in spice-gtk like:
> > >      g_clear_pointer(&priv->device, libusb_unref_device);
> > > cause libusb_unref_device (compiled with _stdcall convention) to be
> > > called
> > > with __cdecl convention. This causes stack corruption, and hence
> > > crashes.
> > 
> > Have you raised a bug in glib? We use this libraries to help with
> > portability so I'd hope it is possible to fix in glib.
> > 
> 
> I don't think it is even possible to fix this cleanly in glib. A
> compilation of glib can work well with __stdcall OR with __cdecl. But
> not with both of them.
> 

It's possible, the function is called just once in the macro so won't
need all the hackish casts that are currently present. I have a fixed
version of the macro (somewhere!).

> Right now I have just found another patch in spice-gtk, where the same
> problem was solved with the same solution, which also has no
> portability issues:
> 
> https://patchwork.freedesktop.org/patch/92705/
> 

Yes, this problem can cause a stack corruption (as you confirmed) and
should be merged GLib or not.

> 
> 
> > > ---
> > >  src/channel-usbredir.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > > index 6ffe546..1d9c380 100644
> > > --- a/src/channel-usbredir.c
> > > +++ b/src/channel-usbredir.c
> > > @@ -352,7 +352,8 @@ static void spice_usbredir_channel_open_acl_cb(
> > >          spice_usbredir_channel_open_device(channel, &err);
> > >      }
> > >      if (err) {
> > > -        g_clear_pointer(&priv->device, libusb_unref_device);
> > > +        libusb_unref_device(priv->device);
> > > +        priv->device = NULL;
> > >          g_boxed_free(spice_usb_device_get_type(), priv-
> > > >spice_device);
> > >          priv->spice_device = NULL;
> > >          priv->state  = STATE_DISCONNECTED;
> > > @@ -383,7 +384,8 @@ _open_device_async_cb(GTask *task,
> > >      spice_usbredir_channel_lock(channel);
> > >  
> > >      if (!spice_usbredir_channel_open_device(channel, &err)) {
> > > -        g_clear_pointer(&priv->device, libusb_unref_device);
> > > +        libusb_unref_device(priv->device);
> > > +        priv->device = NULL;
> > >          g_boxed_free(spice_usb_device_get_type(), priv-
> > > >spice_device);
> > >          priv->spice_device = NULL;
> > >      }
> > > @@ -504,7 +506,8 @@ void
> > > spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel
> > > *channel)
> > >  
> > >          /* This also closes the libusb handle we passed from
> > > open_device */
> > >          usbredirhost_set_device(priv->host, NULL);
> > > -        g_clear_pointer(&priv->device, libusb_unref_device);
> > > +        libusb_unref_device(priv->device);
> > > +        priv->device = NULL;
> > >          g_boxed_free(spice_usb_device_get_type(), priv-
> > > >spice_device);
> > >          priv->spice_device = NULL;
> > >          priv->state  = STATE_DISCONNECTED;

Frediano