rpms/spice-gtk
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Add upstream patches fixing USB event thread leak

Browse Source 
Resolves: rhbz#1217202 (virt-manager)
May help with rhbz#1338042 (gnome-boxes)
This commit is contained in:
Christophe Fergeau 2016-07-11 11:19:25 +02:00
parent 8c1f78c56a
commit a089ec2f5f
6 changed files with 306 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
0001-usbredir-Fix-GTask-leak.patch Normal file
View File

@ -0,0 +1,29 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau@redhat.com>
Date: Wed, 29 Jun 2016 16:29:04 +0200
Subject: [PATCH] usbredir: Fix GTask leak
spice_usbredir_channel_disconnect_device_async() creates a GTask and
then calls g_task_run_in_thread(). This method will take the reference
it needs on the GTask, it does not take ownership of the passed-in
GTask. This means we need to unref the GTask we created after calling
g_task_run_in_thread(), otherwise we are going to leak the GTask, as
well as the channel it's associated with.
Since it's an USB redir channel, this also causes some USB device
manager/USB event thread leaks.
---
src/channel-usbredir.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 2c5feae..4d669c4 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -527,6 +527,7 @@ void spice_usbredir_channel_disconnect_device_async(SpiceUsbredirChannel *channe
g_return_if_fail(channel != NULL);
g_task_run_in_thread(task, _disconnect_device_thread);
+ g_object_unref(task);
}
G_GNUC_INTERNAL

60
0002-usbredir-Use-atomic-for-UsbDeviceManager-event_threa.patch Normal file
View File

@ -0,0 +1,60 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau@redhat.com>
Date: Wed, 29 Jun 2016 16:48:18 +0200
Subject: [PATCH] usbredir: Use atomic for UsbDeviceManager::event_thread_run
This variable is accessed from 2 different threads (main thread and USB
event thread), so some care must be taken to read/write it.
---
src/usb-device-manager.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 1fc8fc1..808ec6c 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -112,7 +112,7 @@ struct _SpiceUsbDeviceManagerPrivate {
libusb_context *context;
int event_listeners;
GThread *event_thread;
- gboolean event_thread_run;
+ gint event_thread_run;
struct usbredirfilter_rule *auto_conn_filter_rules;
struct usbredirfilter_rule *redirect_on_connect_rules;
int auto_conn_filter_rules_count;
@@ -380,7 +380,7 @@ static void spice_usb_device_manager_dispose(GObject *gobject)
priv->hp_handle = 0;
}
#endif
- if (priv->event_thread && !priv->event_thread_run) {
+ if (priv->event_thread && !g_atomic_int_get(&priv->event_thread_run)) {
g_thread_join(priv->event_thread);
priv->event_thread = NULL;
}
@@ -1280,7 +1280,7 @@ static gpointer spice_usb_device_manager_usb_ev_thread(gpointer user_data)
SpiceUsbDeviceManagerPrivate *priv = self->priv;
int rc;
- while (priv->event_thread_run) {
+ while (g_atomic_int_get(&priv->event_thread_run)) {
rc = libusb_handle_events(priv->context);
if (rc && rc != LIBUSB_ERROR_INTERRUPTED) {
const char *desc = spice_usbutil_libusb_strerror(rc);
@@ -1310,7 +1310,7 @@ gboolean spice_usb_device_manager_start_event_listening(
g_thread_join(priv->event_thread);
priv->event_thread = NULL;
}
- priv->event_thread_run = TRUE;
+ g_atomic_int_set(&priv->event_thread_run, TRUE);
priv->event_thread = g_thread_new("usb_ev_thread",
spice_usb_device_manager_usb_ev_thread,
self);
@@ -1326,7 +1326,7 @@ void spice_usb_device_manager_stop_event_listening(
priv->event_listeners--;
if (priv->event_listeners == 0)
- priv->event_thread_run = FALSE;
+ g_atomic_int_set(&priv->event_thread_run, FALSE);
}
static void spice_usb_device_manager_check_redir_on_connect(

107
0003-usb-channel-Really-stop-listening-for-USB-events-on-.patch Normal file
View File

@ -0,0 +1,107 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau@redhat.com>
Date: Wed, 29 Jun 2016 16:57:36 +0200
Subject: [PATCH] usb-channel: Really stop listening for USB events on
disconnection
When using USB redirection, it's fairly easy to leak the thread handling
USB events, which will eventually cause problems in long lived apps.
In particular, in virt-manager, one can:
- start a VM
- connect to it with SPICE
- open the USB redirection window
- redirect a device
- close the SPICE window
-> the SpiceUsbDeviceManager instance will be destroyed (including the
USB context it owns), but the associated event thread will keep running.
Since it's running a loop blocking on libusb_handle_events(priv->context),
the loop will eventually try to use the USB context we just destroyed
causing a crash.
We can get in this situation when redirecting a USB device because we
will call spice_usb_device_manager_start_event_listening() in
spice_usbredir_channel_open_device(). The matching
spice_usb_device_manager_stop_event_listening() call is supposed to
happen in spice_usbredir_channel_disconnect_device(), however by the
time it's called in the scenario described above, the session associated
with the channel will already have been set to NULL in
spice_session_channel_destroy().
The session is only needed in order to get the SpiceUsbDeviceManager
instance we need to call spice_usb_device_manager_stop_event_listening()
on. This patch stores it in SpiceChannelUsbredir instead, this way we
don't need SpiceChannel::session to be non-NULL during device
disconnection.
This should fix the issues described in
https://bugzilla.redhat.com/show_bug.cgi?id=1217202
(virt-manager) and most
likely https://bugzilla.redhat.com/show_bug.cgi?id=1337007 (gnome-boxes)
as well.
---
src/channel-usbredir.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 4d669c4..bc32d6b 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -79,6 +79,7 @@ struct _SpiceUsbredirChannelPrivate {
SpiceUsbAclHelper *acl_helper;
#endif
GMutex device_connect_mutex;
+ SpiceUsbDeviceManager *usb_device_manager;
};
static void channel_set_handlers(SpiceChannelClass *klass);
@@ -188,6 +189,11 @@ static void spice_usbredir_channel_dispose(GObject *obj)
SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(obj);
spice_usbredir_channel_disconnect_device(channel);
+ /* This should have been set to NULL during device disconnection,
+ * but better not to leak it if this does not happen for some reason
+ */
+ g_warn_if_fail(channel->priv->usb_device_manager == NULL);
+ g_clear_object(&channel->priv->usb_device_manager);
/* Chain up to the parent class */
if (G_OBJECT_CLASS(spice_usbredir_channel_parent_class)->dispose)
@@ -275,6 +281,7 @@ static gboolean spice_usbredir_channel_open_device(
SpiceUsbredirChannel *channel, GError **err)
{
SpiceUsbredirChannelPrivate *priv = channel->priv;
+ SpiceSession *session;
libusb_device_handle *handle = NULL;
int rc, status;
@@ -300,10 +307,9 @@ static gboolean spice_usbredir_channel_open_device(
return FALSE;
}
- if (!spice_usb_device_manager_start_event_listening(
- spice_usb_device_manager_get(
- spice_channel_get_session(SPICE_CHANNEL(channel)), NULL),
- err)) {
+ session = spice_channel_get_session(SPICE_CHANNEL(channel));
+ priv->usb_device_manager = g_object_ref(spice_usb_device_manager_get(session, NULL));
+ if (!spice_usb_device_manager_start_event_listening(priv->usb_device_manager, err)) {
usbredirhost_set_device(priv->host, NULL);
return FALSE;
}
@@ -481,12 +487,10 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
* usbredirhost_set_device NULL will interrupt the
* libusb_handle_events call in the thread.
*/
- {
- SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(channel));
- if (session != NULL)
- spice_usb_device_manager_stop_event_listening(
- spice_usb_device_manager_get(session, NULL));
- }
+ g_warn_if_fail(priv->usb_device_manager != NULL);
+ spice_usb_device_manager_stop_event_listening(priv->usb_device_manager);
+ g_clear_object(&priv->usb_device_manager);
+
/* 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);

48
0004-usb-device-manager-Avoid-USB-event-thread-leak.patch Normal file
View File

@ -0,0 +1,48 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau@redhat.com>
Date: Wed, 29 Jun 2016 16:57:36 +0200
Subject: [PATCH] usb-device-manager: Avoid USB event thread leak
This is a follow-up of the previous commit ('usb-channel: Really stop
listening for USB events on disconnection').
Since the USB event thread has to be stopped when we destroy the
associated SpiceUsbDeviceManager, spice_usb_device_manager_dispose()
should force event_thread_run to FALSE even if
spice_usb_device_manager_stop_event_listening() was not enough. When
this happens, this means that there is a bug in the internal users of
spice_usb_device_manager_start_event_listening(), but with this change,
we'll at least warn about it, and avoid a thread leak/potential future
crash.
---
src/usb-device-manager.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 808ec6c..53505fa 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -375,12 +375,22 @@ static void spice_usb_device_manager_dispose(GObject *gobject)
#ifdef USE_LIBUSB_HOTPLUG
if (priv->hp_handle) {
spice_usb_device_manager_stop_event_listening(self);
+ if (g_atomic_int_get(&priv->event_thread_run)) {
+ /* Force termination of the event thread even if there were some
+ * mismatched spice_usb_device_manager_{start,stop}_event_listening
+ * calls. Otherwise, the usb event thread will be leaked, and will
+ * try to use the libusb context we destroy in finalize(), which would
+ * cause a crash */
+ g_warn_if_reached();
+ g_atomic_int_set(&priv->event_thread_run, FALSE);
+ }
/* This also wakes up the libusb_handle_events() in the event_thread */
libusb_hotplug_deregister_callback(priv->context, priv->hp_handle);
priv->hp_handle = 0;
}
#endif
- if (priv->event_thread && !g_atomic_int_get(&priv->event_thread_run)) {
+ if (priv->event_thread) {
+ g_warn_if_fail(g_atomic_int_get(&priv->event_thread_run) == FALSE);
g_thread_join(priv->event_thread);
priv->event_thread = NULL;
}

50
0005-util-fix-off-by-one-array-access.patch Normal file
View File

@ -0,0 +1,50 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@redhat.com>
Date: Thu, 7 Jul 2016 18:23:24 +0200
Subject: [PATCH] util: fix off-by-one array access
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Thanks to ASAN, I found this off-by-one memory access in the unix2dos
code:
/util/unix2dos: =================================================================
==23589==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000dd2f at pc 0x00000040428e bp 0x7ffd6fc31b90 sp 0x7ffd6fc31b80
READ of size 1 at 0x60200000dd2f thread T0
#0 0x40428d in spice_convert_newlines /home/elmarco/src/spice/spice-gtk/src/spice-util.c:355
#1 0x40443a in spice_unix2dos /home/elmarco/src/spice/spice-gtk/src/spice-util.c:382
#2 0x401eae in test_unix2dos /home/elmarco/src/spice/spice-gtk/tests/util.c:69
#3 0x7fb8bcd81983 (/lib64/libglib-2.0.so.0+0x6e983)
#4 0x7fb8bcd81b4e (/lib64/libglib-2.0.so.0+0x6eb4e)
#5 0x7fb8bcd81d5d in g_test_run_suite (/lib64/libglib-2.0.so.0+0x6ed5d)
#6 0x7fb8bcd81d80 in g_test_run (/lib64/libglib-2.0.so.0+0x6ed80)
#7 0x402cce in main /home/elmarco/src/spice/spice-gtk/tests/util.c:207
#8 0x7fb8bc755730 in __libc_start_main (/lib64/libc.so.6+0x20730)
#9 0x401818 in _start (/home/elmarco/src/spice/spice-gtk/tests/util+0x401818)
0x60200000dd2f is located 1 bytes to the left of 4-byte region [0x60200000dd30,0x60200000dd34)
allocated by thread T0 here:
#0 0x7fb8c10421d0 in realloc (/lib64/libasan.so.3+0xc71d0)
#1 0x7fb8bcd61f1f in g_realloc (/lib64/libglib-2.0.so.0+0x4ef1f)
SUMMARY: AddressSanitizer: heap-buffer-overflow /home/elmarco/src/spice/spice-gtk/src/spice-util.c:355 in spice_convert_newlines
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
src/spice-util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/spice-util.c b/src/spice-util.c
index 7788921..bca3abc 100644
--- a/src/spice-util.c
+++ b/src/spice-util.c
@@ -352,7 +352,7 @@ static gchar* spice_convert_newlines(const gchar *str, gssize len,
if (nl) {
/* let's not double \r if it's already in the line */
if (to == NEWLINE_TYPE_CR_LF &&
- output->str[output->len - 1] != '\r')
+ (output->len == 0 || output->str[output->len - 1] != '\r'))
g_string_append_c(output, '\r');
g_string_append_c(output, '\n');

14
spice-gtk.spec
View File

@ -2,7 +2,7 @@
Name: spice-gtk
Version: 0.32
Release: 1%{?dist}
Release: 2%{?dist}
Summary: A GTK+ widget for SPICE clients
Group: System Environment/Libraries
@ -10,6 +10,11 @@ License: LGPLv2+
URL: http://spice-space.org/page/Spice-Gtk
#VCS: git:git://anongit.freedesktop.org/spice/spice-gtk
Source0: http://www.spice-space.org/download/gtk/%{name}-%{version}%{?_version_suffix}.tar.bz2
Patch1: 0001-usbredir-Fix-GTask-leak.patch
Patch2: 0002-usbredir-Use-atomic-for-UsbDeviceManager-event_threa.patch
Patch3: 0003-usb-channel-Really-stop-listening-for-USB-events-on-.patch
Patch4: 0004-usb-device-manager-Avoid-USB-event-thread-leak.patch
Patch5: 0005-util-fix-off-by-one-array-access.patch
BuildRequires: intltool
BuildRequires: usbredir-devel >= 0.5.2
@ -101,7 +106,7 @@ spicy-screenshot is a tool to capture screen-shots of a SPICE desktop.
%prep
%setup -q
%autosetup -S git_am
find . -name '*.stamp' | xargs touch
@ -183,6 +188,11 @@ rm -f %{buildroot}%{_libdir}/*.la
%{_bindir}/spicy-stats
%changelog
* Mon Jul 11 2016 Christophe Fergeau <cfergeau@redhat.com> 0.32-2
- Add upstream patches fixing USB event thread leak
Resolves: rhbz#1217202 (virt-manager)
May help with rhbz#1338042 (gnome-boxes)
* Tue Jun 21 2016 Marc-André Lureau <marcandre.lureau@redhat.com> 0.32-1
- Update to new 0.32 upstream release