rpms/ibus
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Resolves: #1671286 wrong mutex

Browse Source
This commit is contained in:
Takao Fujiwara 2019-02-05 19:40:33 +09:00
parent 97404e255a
commit 769eef0b90
2 changed files with 59 additions and 294 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

348
ibus-1385349-segv-bus-proxy.patch
View File

@ -1,6 +1,6 @@
From 988059d40c9b6cffc3039a6d7623dc73672b0ad5 Mon Sep 17 00:00:00 2001 From 018a0f889d18c41e314f0b1297d1dc559603142b Mon Sep 17 00:00:00 2001
From: fujiwarat <takao.fujiwara1@gmail.com> From: fujiwarat <takao.fujiwara1@gmail.com>
Date: Tue, 29 Jan 2019 17:49:47 +0900 Date: Tue, 5 Feb 2019 18:36:04 +0900
Subject: [PATCH] Fix SEGV in bus_panel_proxy_focus_in() Subject: [PATCH] Fix SEGV in bus_panel_proxy_focus_in()
rhbz#1349148, rhbz#1385349 rhbz#1349148, rhbz#1385349
@ -36,13 +36,13 @@ BUG=rhbz#1432252
BUG=rhbz#1601577 BUG=rhbz#1601577
BUG=rhbz#1663528 BUG=rhbz#1663528
--- ---
bus/dbusimpl.c | 236 +++++++++++++++++++++++++++++++++++++--------- bus/dbusimpl.c | 70 +++++++++++++++++++++++++++++++++++++++++------
bus/engineproxy.c | 9 +- bus/engineproxy.c | 9 +++++-
bus/ibusimpl.c | 21 ++++- bus/ibusimpl.c | 21 ++++++++++++--
3 files changed, 215 insertions(+), 51 deletions(-) 3 files changed, 88 insertions(+), 12 deletions(-)
diff --git a/bus/dbusimpl.c b/bus/dbusimpl.c diff --git a/bus/dbusimpl.c b/bus/dbusimpl.c
index b54ef817..2eb5565d 100644 index b54ef817..fb38faf0 100644
--- a/bus/dbusimpl.c --- a/bus/dbusimpl.c
+++ b/bus/dbusimpl.c +++ b/bus/dbusimpl.c
@@ -2,7 +2,8 @@ @@ -2,7 +2,8 @@
@ -55,116 +55,7 @@ index b54ef817..2eb5565d 100644
* *
* This library is free software; you can redistribute it and/or * This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public * modify it under the terms of the GNU Lesser General Public
@@ -43,13 +44,23 @@ enum { @@ -344,6 +345,8 @@ bus_name_service_set_primary_owner (BusNameService *service,
static guint dbus_signals[LAST_SIGNAL] = { 0 };
+/* rhbz#1663528 Check the value of is_locked before g_mutex_clear()
+ * because if the mutex is not unlocked, g_mutex_clear() causes assert.
+ */
+typedef struct {
+ GMutex lock;
+ gboolean is_locked;
+ GList *queue;
+} BusDBusQueue;
+
struct _BusDBusImpl {
IBusService parent;
/* instance members */
/* a map from a unique bus name (e.g. ":1.0") to a BusConnection. */
GHashTable *unique_names;
- /* a map from a requested well-known name (e.g. "org.freedesktop.IBus.Panel") to a BusNameService. */
+ /* a map from a requested well-known name (e.g. "org.freedesktop.IBus.Panel"
+ *) to a BusNameService. */
GHashTable *names;
/* a list of IBusService objects. */
GList *objects;
@@ -60,11 +71,8 @@ struct _BusDBusImpl {
/* a serial number used to generate a unique name of a bus. */
guint id;
- GMutex dispatch_lock;
- GList *dispatch_queue;
-
- GMutex forward_lock;
- GList *forward_queue;
+ BusDBusQueue *dispatch;
+ BusDBusQueue *forward;
/* a list of BusMethodCall to be used to reply when services are
really available */
@@ -255,6 +263,69 @@ static const gchar introspection_xml[] =
" </interface>"
"</node>";
+/* Use the functions instead of the macros because the macros would caused
+ * optimized g_mutex_unlock() with GCC9. This also has a dummy return variable
+ * to prevent the reverse order of safe_lock() and safe_unlock() with the
+ * optimization in Fedora builds.
+ */
+static gboolean
+bus_dbus_queue_mutex_safe_lock(BusDBusQueue *queue,
+ GDBusMessage *message,
+ int line,
+ const gchar *func) {
+ int tout = 0;
+ while (G_UNLIKELY ((queue)->is_locked)) {
+ g_usleep (1);
+ tout++;
+ if (tout > 60)
+ break;
+ }
+ if (G_UNLIKELY (tout)) {
+ const gchar *path = g_dbus_message_get_path (message);
+ const gchar *interface = g_dbus_message_get_interface (message);
+ const gchar *member = g_dbus_message_get_member (message);
+ const gchar *sender = g_dbus_message_get_sender (message);
+ const gchar *dest = g_dbus_message_get_destination (message);
+ g_warning ("%d:%s:(%s:%s:%s:%s:%s):%d: was locked.",
+ line, func,
+ path ? path : "(null)",
+ interface ? interface : "(null)",
+ member ? member : "(null)",
+ sender ? sender : "(null)",
+ dest ? dest : "(null)",
+ tout);
+ }
+ (queue)->is_locked = TRUE;
+ g_mutex_lock (&((queue)->lock));
+ return TRUE;
+}
+
+static gboolean
+bus_dbus_queue_mutex_safe_unlock(BusDBusQueue *queue,
+ GDBusMessage *message,
+ int line,
+ const gchar *func) {
+ if (G_LIKELY ((queue)->is_locked)) {
+ g_mutex_unlock (&((queue)->lock));
+ } else {
+ const gchar *path = g_dbus_message_get_path (message);
+ const gchar *interface = g_dbus_message_get_interface (message);
+ const gchar *member = g_dbus_message_get_member (message);
+ const gchar *sender = g_dbus_message_get_sender (message);
+ const gchar *dest = g_dbus_message_get_destination (message);
+ g_warning ("%d:%s:(%s:%s:%s:%s:%s):" \
+ " was unlocked by bus_dbus_impl_destroy",
+ line, func,
+ path ? path : "(null)",
+ interface ? interface : "(null)",
+ member ? member : "(null)",
+ sender ? sender : "(null)",
+ dest ? dest : "(null)");
+ }
+ (queue)->is_locked = FALSE;
+ return FALSE;
+}
+
static void
bus_connection_owner_set_flags (BusConnectionOwner *owner,
guint32 flags)
@@ -344,6 +415,8 @@ bus_name_service_set_primary_owner (BusNameService *service,
BusConnectionOwner *owner, BusConnectionOwner *owner,
BusDBusImpl *dbus) BusDBusImpl *dbus)
{ {
@ -173,7 +64,7 @@ index b54ef817..2eb5565d 100644
g_assert (service != NULL); g_assert (service != NULL);
g_assert (owner != NULL); g_assert (owner != NULL);
g_assert (dbus != NULL); g_assert (dbus != NULL);
@@ -351,6 +424,13 @@ bus_name_service_set_primary_owner (BusNameService *service, @@ -351,6 +354,13 @@ bus_name_service_set_primary_owner (BusNameService *service,
BusConnectionOwner *old = service->owners != NULL ? BusConnectionOwner *old = service->owners != NULL ?
(BusConnectionOwner *)service->owners->data : NULL; (BusConnectionOwner *)service->owners->data : NULL;
@ -187,7 +78,7 @@ index b54ef817..2eb5565d 100644
if (old != NULL) { if (old != NULL) {
g_signal_emit (dbus, g_signal_emit (dbus,
dbus_signals[NAME_LOST], dbus_signals[NAME_LOST],
@@ -370,7 +450,8 @@ bus_name_service_set_primary_owner (BusNameService *service, @@ -370,7 +380,8 @@ bus_name_service_set_primary_owner (BusNameService *service,
0, 0,
owner->conn, owner->conn,
service->name, service->name,
@ -197,7 +88,7 @@ index b54ef817..2eb5565d 100644
bus_connection_get_unique_name (owner->conn)); bus_connection_get_unique_name (owner->conn));
if (old != NULL && old->do_not_queue != 0) { if (old != NULL && old->do_not_queue != 0) {
@@ -427,6 +508,7 @@ bus_name_service_remove_owner (BusNameService *service, @@ -427,6 +438,7 @@ bus_name_service_remove_owner (BusNameService *service,
BusDBusImpl *dbus) BusDBusImpl *dbus)
{ {
GSList *owners; GSList *owners;
@ -205,7 +96,7 @@ index b54ef817..2eb5565d 100644
g_assert (service != NULL); g_assert (service != NULL);
g_assert (owner != NULL); g_assert (owner != NULL);
@@ -439,6 +521,13 @@ bus_name_service_remove_owner (BusNameService *service, @@ -439,6 +451,13 @@ bus_name_service_remove_owner (BusNameService *service,
BusConnectionOwner *_new = NULL; BusConnectionOwner *_new = NULL;
if (owners->next != NULL) { if (owners->next != NULL) {
_new = (BusConnectionOwner *)owners->next->data; _new = (BusConnectionOwner *)owners->next->data;
@ -219,7 +110,7 @@ index b54ef817..2eb5565d 100644
} }
if (dbus != NULL) { if (dbus != NULL) {
@@ -447,7 +536,7 @@ bus_name_service_remove_owner (BusNameService *service, @@ -447,7 +466,7 @@ bus_name_service_remove_owner (BusNameService *service,
0, 0,
owner->conn, owner->conn,
service->name); service->name);
@ -228,7 +119,7 @@ index b54ef817..2eb5565d 100644
g_signal_emit (dbus, g_signal_emit (dbus,
dbus_signals[NAME_ACQUIRED], dbus_signals[NAME_ACQUIRED],
0, 0,
@@ -460,7 +549,7 @@ bus_name_service_remove_owner (BusNameService *service, @@ -460,7 +479,7 @@ bus_name_service_remove_owner (BusNameService *service,
_new != NULL ? _new->conn : NULL, _new != NULL ? _new->conn : NULL,
service->name, service->name,
bus_connection_get_unique_name (owner->conn), bus_connection_get_unique_name (owner->conn),
@ -237,47 +128,57 @@ index b54ef817..2eb5565d 100644
} }
} }
@@ -581,8 +670,10 @@ bus_dbus_impl_init (BusDBusImpl *dbus) @@ -591,6 +610,7 @@ static void
NULL, bus_dbus_impl_destroy (BusDBusImpl *dbus)
(GDestroyNotify) bus_name_service_free); {
GList *p;
+ int i;
- g_mutex_init (&dbus->dispatch_lock); for (p = dbus->objects; p != NULL; p = p->next) {
- g_mutex_init (&dbus->forward_lock); IBusService *object = (IBusService *) p->data;
+ dbus->dispatch = g_slice_new0 (BusDBusQueue); @@ -628,12 +648,39 @@ bus_dbus_impl_destroy (BusDBusImpl *dbus)
+ g_mutex_init (&dbus->dispatch->lock); dbus->unique_names = NULL;
+ dbus->forward = g_slice_new0 (BusDBusQueue); dbus->names = NULL;
+ g_mutex_init (&dbus->forward->lock);
/* other members are automatically zero-initialized. */ + for (i = 0; g_idle_remove_by_data (dbus); i++) {
} + if (i > 1000) {
@@ -632,8 +723,24 @@ bus_dbus_impl_destroy (BusDBusImpl *dbus) + g_warning ("Too many idle threads were generated by " \
+ "bus_dbus_impl_forward_message_idle_cb and " \
+ "bus_dbus_impl_dispatch_message_by_rule_idle_cb");
+ break;
+ }
+ }
g_list_free_full (dbus->start_service_calls,
(GDestroyNotify) bus_method_call_free); (GDestroyNotify) bus_method_call_free);
dbus->start_service_calls = NULL; dbus->start_service_calls = NULL;
- g_mutex_clear (&dbus->dispatch_lock); - g_mutex_clear (&dbus->dispatch_lock);
- g_mutex_clear (&dbus->forward_lock); - g_mutex_clear (&dbus->forward_lock);
+ /* rhbz#1663528 Check the value of is_locked before g_mutex_clear() + /* rhbz#1663528 Call g_mutex_trylock() before g_mutex_clear()
+ * because if the mutex is not unlocked, g_mutex_clear() causes assert. + * because if the mutex is not unlocked, g_mutex_clear() causes assert.
+ */ + */
+ if (G_UNLIKELY (dbus->dispatch->is_locked)) { +#define BUS_DBUS_MUTEX_SAFE_CLEAR(mtex) { \
+ g_mutex_unlock (&dbus->dispatch->lock); + int count = 0; \
+ g_warning ("dbus->dispatch was not unlocked"); + while (!g_mutex_trylock ((mtex))) { \
+ dbus->dispatch->is_locked = FALSE; + g_usleep (1); \
+ } + if (count > 60) { \
+ g_mutex_clear (&dbus->dispatch->lock); + g_warning (#mtex " is dead lock"); \
+ g_slice_free (BusDBusQueue, dbus->dispatch); + break; \
+ } \
+ ++count; \
+ } \
+ g_mutex_unlock ((mtex)); \
+ g_mutex_clear ((mtex)); \
+}
+ +
+ if (G_UNLIKELY (dbus->forward->is_locked)) { + BUS_DBUS_MUTEX_SAFE_CLEAR (&dbus->dispatch_lock);
+ g_mutex_unlock (&dbus->forward->lock); + BUS_DBUS_MUTEX_SAFE_CLEAR (&dbus->forward_lock);
+ g_warning ("dbus->forward was not unlocked"); +
+ dbus->forward->is_locked = FALSE; +#undef BUS_DBUS_MUTEX_SAFE_CLEAR
+ }
+ g_mutex_clear (&dbus->forward->lock);
+ g_slice_free (BusDBusQueue, dbus->forward);
/* FIXME destruct _lock and _queue members. */ /* FIXME destruct _lock and _queue members. */
IBUS_OBJECT_CLASS(bus_dbus_impl_parent_class)->destroy ((IBusObject *) dbus); IBUS_OBJECT_CLASS(bus_dbus_impl_parent_class)->destroy ((IBusObject *) dbus);
@@ -1464,13 +1571,20 @@ bus_dbus_impl_connection_filter_cb (GDBusConnection *dbus_connection, @@ -1464,13 +1511,20 @@ bus_dbus_impl_connection_filter_cb (GDBusConnection *dbus_connection,
gboolean incoming, gboolean incoming,
gpointer user_data) gpointer user_data)
{ {
@ -300,145 +201,6 @@ index b54ef817..2eb5565d 100644
if (incoming) { if (incoming) {
/* is incoming message */ /* is incoming message */
@@ -1721,18 +1835,24 @@ struct _BusForwardData {
/**
* bus_dbus_impl_forward_message_ible_cb:
*
- * Process the first element of the dbus->forward_queue. The first element is forwarded by g_dbus_connection_send_message.
+ * Process the first element of the dbus->forward->queue. The first element is
+ * forwarded by g_dbus_connection_send_message.
*/
static gboolean
bus_dbus_impl_forward_message_idle_cb (BusDBusImpl *dbus)
{
- g_return_val_if_fail (dbus->forward_queue != NULL, FALSE);
-
- g_mutex_lock (&dbus->forward_lock);
- BusForwardData *data = (BusForwardData *) dbus->forward_queue->data;
- dbus->forward_queue = g_list_delete_link (dbus->forward_queue, dbus->forward_queue);
- gboolean has_message = (dbus->forward_queue != NULL);
- g_mutex_unlock (&dbus->forward_lock);
+ g_return_val_if_fail (dbus->forward->queue != NULL, FALSE);
+
+ BusForwardData *data = (BusForwardData *) dbus->forward->queue->data;
+ dbus->forward->is_locked = bus_dbus_queue_mutex_safe_lock (
+ dbus->forward, data->message,
+ __LINE__, G_STRFUNC);
+ dbus->forward->queue = g_list_delete_link (dbus->forward->queue,
+ dbus->forward->queue);
+ gboolean has_message = (dbus->forward->queue != NULL);
+ dbus->forward->is_locked = bus_dbus_queue_mutex_safe_unlock (
+ dbus->forward, data->message,
+ __LINE__, G_STRFUNC);
do {
const gchar *destination = g_dbus_message_get_destination (data->message);
@@ -1791,18 +1911,25 @@ bus_dbus_impl_forward_message (BusDBusImpl *dbus,
if (G_UNLIKELY (IBUS_OBJECT_DESTROYED (dbus)))
return;
- /* FIXME the check above might not be sufficient. dbus object could be destroyed in the main thread right after the check, though the
- * dbus structure itself would not be freed (since the dbus object is ref'ed in bus_dbus_impl_new_connection.)
- * Anyway, it'd be better to investigate whether the thread safety issue could cause any real problems. */
+ /* FIXME the check above might not be sufficient. dbus object could be
+ * destroyed in the main thread right after the check, though the
+ * dbus structure itself would not be freed (since the dbus object is
+ * ref'ed in bus_dbus_impl_new_connection.)
+ * Anyway, it'd be better to investigate whether the thread safety issue
+ * could cause any real problems. */
BusForwardData *data = g_slice_new (BusForwardData);
data->message = g_object_ref (message);
data->sender_connection = g_object_ref (connection);
- g_mutex_lock (&dbus->forward_lock);
- gboolean is_running = (dbus->forward_queue != NULL);
- dbus->forward_queue = g_list_append (dbus->forward_queue, data);
- g_mutex_unlock (&dbus->forward_lock);
+ dbus->forward->is_locked = bus_dbus_queue_mutex_safe_lock (
+ dbus->forward, message,
+ __LINE__, G_STRFUNC);
+ gboolean is_running = (dbus->forward->queue != NULL);
+ dbus->forward->queue = g_list_append (dbus->forward->queue, data);
+ dbus->forward->is_locked = bus_dbus_queue_mutex_safe_unlock (
+ dbus->forward, message,
+ __LINE__, G_STRFUNC);
if (!is_running) {
g_idle_add_full (G_PRIORITY_DEFAULT,
@@ -1840,29 +1967,40 @@ bus_dispatch_data_free (BusDispatchData *data)
/**
* bus_dbus_impl_dispatch_message_by_rule_idle_cb:
*
- * Process the first element of the dbus->dispatch_queue.
+ * Process the first element of the dbus->dispatch->queue.
*/
static gboolean
bus_dbus_impl_dispatch_message_by_rule_idle_cb (BusDBusImpl *dbus)
{
- g_return_val_if_fail (dbus->dispatch_queue != NULL, FALSE);
+ g_return_val_if_fail (dbus->dispatch->queue != NULL, FALSE);
if (G_UNLIKELY (IBUS_OBJECT_DESTROYED (dbus))) {
/* dbus was destryed */
- g_mutex_lock (&dbus->dispatch_lock);
- g_list_free_full (dbus->dispatch_queue,
+ BusDispatchData *data = (BusDispatchData *) dbus->dispatch->queue->data;
+ dbus->dispatch->is_locked = bus_dbus_queue_mutex_safe_lock (
+ dbus->dispatch, data->message,
+ __LINE__, G_STRFUNC);
+ g_list_free_full (dbus->dispatch->queue,
(GDestroyNotify) bus_dispatch_data_free);
- dbus->dispatch_queue = NULL;
- g_mutex_unlock (&dbus->dispatch_lock);
- return FALSE; /* return FALSE to prevent this callback to be called again. */
+ dbus->dispatch->queue = NULL;
+ dbus->dispatch->is_locked = bus_dbus_queue_mutex_safe_unlock (
+ dbus->dispatch, data->message,
+ __LINE__, G_STRFUNC);
+ return FALSE;
+ /* return FALSE to prevent this callback to be called again. */
}
/* remove fist node */
- g_mutex_lock (&dbus->dispatch_lock);
- BusDispatchData *data = (BusDispatchData *) dbus->dispatch_queue->data;
- dbus->dispatch_queue = g_list_delete_link (dbus->dispatch_queue, dbus->dispatch_queue);
- gboolean has_message = (dbus->dispatch_queue != NULL);
- g_mutex_unlock (&dbus->dispatch_lock);
+ BusDispatchData *data = (BusDispatchData *) dbus->dispatch->queue->data;
+ dbus->dispatch->is_locked = bus_dbus_queue_mutex_safe_lock (
+ dbus->dispatch, data->message,
+ __LINE__, G_STRFUNC);
+ dbus->dispatch->queue = g_list_delete_link (dbus->dispatch->queue,
+ dbus->dispatch->queue);
+ gboolean has_message = (dbus->dispatch->queue != NULL);
+ dbus->dispatch->is_locked = bus_dbus_queue_mutex_safe_unlock (
+ dbus->dispatch, data->message,
+ __LINE__, G_STRFUNC);
GList *link = NULL;
GList *recipients = NULL;
@@ -1916,11 +2054,15 @@ bus_dbus_impl_dispatch_message_by_rule (BusDBusImpl *dbus,
g_object_set_qdata ((GObject *) message, dispatched_quark, GINT_TO_POINTER (1));
/* append dispatch data into the queue, and start idle task if necessary */
- g_mutex_lock (&dbus->dispatch_lock);
- gboolean is_running = (dbus->dispatch_queue != NULL);
- dbus->dispatch_queue = g_list_append (dbus->dispatch_queue,
+ dbus->dispatch->is_locked = bus_dbus_queue_mutex_safe_lock (
+ dbus->dispatch, message,
+ __LINE__, G_STRFUNC);
+ gboolean is_running = (dbus->dispatch->queue != NULL);
+ dbus->dispatch->queue = g_list_append (dbus->dispatch->queue,
bus_dispatch_data_new (message, skip_connection));
- g_mutex_unlock (&dbus->dispatch_lock);
+ dbus->dispatch->is_locked = bus_dbus_queue_mutex_safe_unlock (
+ dbus->dispatch, message,
+ __LINE__, G_STRFUNC);
if (!is_running) {
g_idle_add_full (G_PRIORITY_DEFAULT,
(GSourceFunc) bus_dbus_impl_dispatch_message_by_rule_idle_cb,
diff --git a/bus/engineproxy.c b/bus/engineproxy.c diff --git a/bus/engineproxy.c b/bus/engineproxy.c
index 2d98995c..2176e0c9 100644 index 2d98995c..2176e0c9 100644
--- a/bus/engineproxy.c --- a/bus/engineproxy.c
@ -528,5 +290,5 @@ index bbbb5770..77fcf42f 100644
bus_ibus_impl_component_name_owner_changed (ibus, name, old_name, new_name); bus_ibus_impl_component_name_owner_changed (ibus, name, old_name, new_name);
} }
-- --
2.19.1 2.20.1

5
ibus.spec
View File

@ -35,7 +35,7 @@
Name: ibus Name: ibus
Version: 1.5.19 Version: 1.5.19
Release: 15%{?dist} Release: 16%{?dist}
Summary: Intelligent Input Bus for Linux OS Summary: Intelligent Input Bus for Linux OS
License: LGPLv2+ License: LGPLv2+
URL: https://github.com/ibus/%name/wiki URL: https://github.com/ibus/%name/wiki
@ -424,6 +424,9 @@ dconf update || :
%{_datadir}/gtk-doc/html/* %{_datadir}/gtk-doc/html/*
%changelog %changelog
* Tue Feb 05 2019 Takao Fujiwara <tfujiwar@redhat.com> - 1.5.19-16
- Resolves: #1671286 wrong mutex
* Mon Feb 04 2019 Kalev Lember <klember@redhat.com> - 1.5.19-15 * Mon Feb 04 2019 Kalev Lember <klember@redhat.com> - 1.5.19-15
- Update BRs for vala packaging changes - Update BRs for vala packaging changes
- Co-own vala and gir directories - Co-own vala and gir directories