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

[Victor Toso]

Hi,

On Tue, Aug 21, 2018 at 03:18:35AM -0400, Frediano Ziglio wrote:
> > 
> > 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!).

Ah, that would be cool.

> 
> > 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.

Yes, not against merging the fix. But if we can get it fixed in
Glib, it is matter to help other projects that port code to
windows to be fixed as well, etc.

Please open a bug in Glib and add a reference to it in the code
(to avoid people reverting this change while the bug is not fixed
there)

    https://gitlab.gnome.org/GNOME/glib/issues

Cheers,
Victor

> > > > ---
> > > >  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180821/3db77b6d/attachment.sig>