From a089ec2f5f1ce6b5f67fc44234aa66ea302135c6 Mon Sep 17 00:00:00 2001 From: Christophe Fergeau Date: Mon, 11 Jul 2016 11:19:25 +0200 Subject: [PATCH] Add upstream patches fixing USB event thread leak Resolves: rhbz#1217202 (virt-manager) May help with rhbz#1338042 (gnome-boxes) --- 0001-usbredir-Fix-GTask-leak.patch | 29 +++++ ...mic-for-UsbDeviceManager-event_threa.patch | 60 ++++++++++ ...ly-stop-listening-for-USB-events-on-.patch | 107 ++++++++++++++++++ ...-manager-Avoid-USB-event-thread-leak.patch | 48 ++++++++ 0005-util-fix-off-by-one-array-access.patch | 50 ++++++++ spice-gtk.spec | 14 ++- 6 files changed, 306 insertions(+), 2 deletions(-) create mode 100644 0001-usbredir-Fix-GTask-leak.patch create mode 100644 0002-usbredir-Use-atomic-for-UsbDeviceManager-event_threa.patch create mode 100644 0003-usb-channel-Really-stop-listening-for-USB-events-on-.patch create mode 100644 0004-usb-device-manager-Avoid-USB-event-thread-leak.patch create mode 100644 0005-util-fix-off-by-one-array-access.patch diff --git a/0001-usbredir-Fix-GTask-leak.patch b/0001-usbredir-Fix-GTask-leak.patch new file mode 100644 index 0000000..c05d945 --- /dev/null +++ b/0001-usbredir-Fix-GTask-leak.patch @@ -0,0 +1,29 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Christophe Fergeau +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 diff --git a/0002-usbredir-Use-atomic-for-UsbDeviceManager-event_threa.patch b/0002-usbredir-Use-atomic-for-UsbDeviceManager-event_threa.patch new file mode 100644 index 0000000..2299324 --- /dev/null +++ b/0002-usbredir-Use-atomic-for-UsbDeviceManager-event_threa.patch @@ -0,0 +1,60 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Christophe Fergeau +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( diff --git a/0003-usb-channel-Really-stop-listening-for-USB-events-on-.patch b/0003-usb-channel-Really-stop-listening-for-USB-events-on-.patch new file mode 100644 index 0000000..87ffdd7 --- /dev/null +++ b/0003-usb-channel-Really-stop-listening-for-USB-events-on-.patch @@ -0,0 +1,107 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Christophe Fergeau +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); diff --git a/0004-usb-device-manager-Avoid-USB-event-thread-leak.patch b/0004-usb-device-manager-Avoid-USB-event-thread-leak.patch new file mode 100644 index 0000000..b1da657 --- /dev/null +++ b/0004-usb-device-manager-Avoid-USB-event-thread-leak.patch @@ -0,0 +1,48 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Christophe Fergeau +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; + } diff --git a/0005-util-fix-off-by-one-array-access.patch b/0005-util-fix-off-by-one-array-access.patch new file mode 100644 index 0000000..ca05a40 --- /dev/null +++ b/0005-util-fix-off-by-one-array-access.patch @@ -0,0 +1,50 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= +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 +--- + 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'); diff --git a/spice-gtk.spec b/spice-gtk.spec index 06c7950..858f594 100644 --- a/spice-gtk.spec +++ b/spice-gtk.spec @@ -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 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 0.32-1 - Update to new 0.32 upstream release