From 988059d40c9b6cffc3039a6d7623dc73672b0ad5 Mon Sep 17 00:00:00 2001 From: fujiwarat Date: Tue, 29 Jan 2019 17:49:47 +0900 Subject: [PATCH] Fix SEGV in bus_panel_proxy_focus_in() rhbz#1349148, rhbz#1385349 SEGV in BUS_IS_PANEL_PROXY() in bus_panel_proxy_focus_in() Check if GDBusConnect is closed before bus_panel_proxy_new() is called. rhbz#1350291 SEGV in BUS_IS_CONNECTION(skip_connection) in bus_dbus_impl_dispatch_message_by_rule() check if dbus_connection is closed in bus_dbus_impl_connection_filter_cb(). rhbz#1406699 SEGV in new_owner!=NULL in bus_dbus_impl_name_owner_changed() which is called by bus_name_service_remove_owner() If bus_connection_get_unique_name()==NULL, set new_owner="" in bus_name_service_remove_owner() rhbz#1432252 SEGV in old_owner!=NULL in bus_dbus_impl_name_owner_changed() which is called by bus_name_service_set_primary_owner() If bus_connection_get_unique_name()==NULL, set old_owner="" in bus_name_service_set_primary_owner() rhbz#1601577 SEGV in ibus_engine_desc_get_layout() in bus_engine_proxy_new_internal() WIP: Added a GError to get the error message to check why the SEGV happened. rhbz#1663528 SEGV in g_mutex_clear() in bus_dbus_impl_destroy() If the mutex is not unlocked, g_mutex_clear() causes assert. BUG=rhbz#1349148 BUG=rhbz#1385349 BUG=rhbz#1350291 BUG=rhbz#1406699 BUG=rhbz#1432252 BUG=rhbz#1601577 BUG=rhbz#1663528 --- bus/dbusimpl.c | 236 +++++++++++++++++++++++++++++++++++++--------- bus/engineproxy.c | 9 +- bus/ibusimpl.c | 21 ++++- 3 files changed, 215 insertions(+), 51 deletions(-) diff --git a/bus/dbusimpl.c b/bus/dbusimpl.c index b54ef817..2eb5565d 100644 --- a/bus/dbusimpl.c +++ b/bus/dbusimpl.c @@ -2,7 +2,8 @@ /* vim:set et sts=4: */ /* ibus - The Input Bus * Copyright (C) 2008-2013 Peng Huang - * Copyright (C) 2008-2013 Red Hat, Inc. + * Copyright (C) 2015-2019 Takao Fujiwara + * Copyright (C) 2008-2019 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -43,13 +44,23 @@ enum { 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[] = " " ""; +/* 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, BusDBusImpl *dbus) { + gboolean has_old_owner = FALSE; + g_assert (service != NULL); g_assert (owner != NULL); g_assert (dbus != NULL); @@ -351,6 +424,13 @@ bus_name_service_set_primary_owner (BusNameService *service, BusConnectionOwner *old = service->owners != NULL ? (BusConnectionOwner *)service->owners->data : NULL; + /* rhbz#1432252 If bus_connection_get_unique_name() == NULL, + * "Hello" method is not received yet. + */ + if (old != NULL && bus_connection_get_unique_name (old->conn) != NULL) { + has_old_owner = TRUE; + } + if (old != NULL) { g_signal_emit (dbus, dbus_signals[NAME_LOST], @@ -370,7 +450,8 @@ bus_name_service_set_primary_owner (BusNameService *service, 0, owner->conn, service->name, - old != NULL ? bus_connection_get_unique_name (old->conn) : "", + has_old_owner ? bus_connection_get_unique_name (old->conn) : + "", bus_connection_get_unique_name (owner->conn)); if (old != NULL && old->do_not_queue != 0) { @@ -427,6 +508,7 @@ bus_name_service_remove_owner (BusNameService *service, BusDBusImpl *dbus) { GSList *owners; + gboolean has_new_owner = FALSE; g_assert (service != NULL); g_assert (owner != NULL); @@ -439,6 +521,13 @@ bus_name_service_remove_owner (BusNameService *service, BusConnectionOwner *_new = NULL; if (owners->next != NULL) { _new = (BusConnectionOwner *)owners->next->data; + /* rhbz#1406699 If bus_connection_get_unique_name() == NULL, + * "Hello" method is not received yet. + */ + if (_new != NULL && + bus_connection_get_unique_name (_new->conn) != NULL) { + has_new_owner = TRUE; + } } if (dbus != NULL) { @@ -447,7 +536,7 @@ bus_name_service_remove_owner (BusNameService *service, 0, owner->conn, service->name); - if (_new != NULL) { + if (has_new_owner) { g_signal_emit (dbus, dbus_signals[NAME_ACQUIRED], 0, @@ -460,7 +549,7 @@ bus_name_service_remove_owner (BusNameService *service, _new != NULL ? _new->conn : NULL, service->name, bus_connection_get_unique_name (owner->conn), - _new != NULL ? bus_connection_get_unique_name (_new->conn) : ""); + has_new_owner ? bus_connection_get_unique_name (_new->conn) : ""); } } @@ -581,8 +670,10 @@ bus_dbus_impl_init (BusDBusImpl *dbus) NULL, (GDestroyNotify) bus_name_service_free); - g_mutex_init (&dbus->dispatch_lock); - g_mutex_init (&dbus->forward_lock); + dbus->dispatch = g_slice_new0 (BusDBusQueue); + g_mutex_init (&dbus->dispatch->lock); + dbus->forward = g_slice_new0 (BusDBusQueue); + g_mutex_init (&dbus->forward->lock); /* other members are automatically zero-initialized. */ } @@ -632,8 +723,24 @@ bus_dbus_impl_destroy (BusDBusImpl *dbus) (GDestroyNotify) bus_method_call_free); dbus->start_service_calls = NULL; - g_mutex_clear (&dbus->dispatch_lock); - g_mutex_clear (&dbus->forward_lock); + /* rhbz#1663528 Check the value of is_locked before g_mutex_clear() + * because if the mutex is not unlocked, g_mutex_clear() causes assert. + */ + if (G_UNLIKELY (dbus->dispatch->is_locked)) { + g_mutex_unlock (&dbus->dispatch->lock); + g_warning ("dbus->dispatch was not unlocked"); + dbus->dispatch->is_locked = FALSE; + } + g_mutex_clear (&dbus->dispatch->lock); + g_slice_free (BusDBusQueue, dbus->dispatch); + + if (G_UNLIKELY (dbus->forward->is_locked)) { + g_mutex_unlock (&dbus->forward->lock); + g_warning ("dbus->forward was not unlocked"); + dbus->forward->is_locked = FALSE; + } + g_mutex_clear (&dbus->forward->lock); + g_slice_free (BusDBusQueue, dbus->forward); /* FIXME destruct _lock and _queue members. */ IBUS_OBJECT_CLASS(bus_dbus_impl_parent_class)->destroy ((IBusObject *) dbus); @@ -1464,13 +1571,20 @@ bus_dbus_impl_connection_filter_cb (GDBusConnection *dbus_connection, gboolean incoming, gpointer user_data) { + BusDBusImpl *dbus; + BusConnection *connection; + g_assert (G_IS_DBUS_CONNECTION (dbus_connection)); g_assert (G_IS_DBUS_MESSAGE (message)); g_assert (BUS_IS_DBUS_IMPL (user_data)); - BusDBusImpl *dbus = (BusDBusImpl *) user_data; - BusConnection *connection = bus_connection_lookup (dbus_connection); + if (g_dbus_connection_is_closed (dbus_connection)) + return NULL; + + dbus = (BusDBusImpl *) user_data; + connection = bus_connection_lookup (dbus_connection); g_assert (connection != NULL); + g_assert (BUS_IS_CONNECTION (connection)); if (incoming) { /* 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 index 2d98995c..2176e0c9 100644 --- a/bus/engineproxy.c +++ b/bus/engineproxy.c @@ -665,6 +665,7 @@ bus_engine_proxy_new_internal (const gchar *path, IBusEngineDesc *desc, GDBusConnection *connection) { + GError *error = NULL; g_assert (path); g_assert (IBUS_IS_ENGINE_DESC (desc)); g_assert (G_IS_DBUS_CONNECTION (connection)); @@ -673,7 +674,7 @@ bus_engine_proxy_new_internal (const gchar *path, BusEngineProxy *engine = (BusEngineProxy *) g_initable_new (BUS_TYPE_ENGINE_PROXY, NULL, - NULL, + &error, "desc", desc, "g-connection", connection, "g-interface-name", IBUS_INTERFACE_ENGINE, @@ -681,6 +682,12 @@ bus_engine_proxy_new_internal (const gchar *path, "g-default-timeout", g_gdbus_timeout, "g-flags", flags, NULL); + /* FIXME: rhbz#1601577 */ + if (error) { + /* show abrt local variable */ + gchar *message = g_strdup (error->message); + g_error ("%s", message); + } const gchar *layout = ibus_engine_desc_get_layout (desc); if (layout != NULL && layout[0] != '\0') { engine->keymap = ibus_keymap_get (layout); diff --git a/bus/ibusimpl.c b/bus/ibusimpl.c index bbbb5770..77fcf42f 100644 --- a/bus/ibusimpl.c +++ b/bus/ibusimpl.c @@ -464,13 +464,16 @@ _dbus_name_owner_changed_cb (BusDBusImpl *dbus, else if (!g_strcmp0 (name, IBUS_SERVICE_PANEL_EXTENSION_EMOJI)) panel_type = PANEL_TYPE_EXTENSION_EMOJI; - if (panel_type != PANEL_TYPE_NONE) { + do { + if (panel_type == PANEL_TYPE_NONE) + break; if (g_strcmp0 (new_name, "") != 0) { /* a Panel process is started. */ BusConnection *connection; BusInputContext *context = NULL; BusPanelProxy **panel = (panel_type == PANEL_TYPE_PANEL) ? &ibus->panel : &ibus->emoji_extension; + GDBusConnection *dbus_connection = NULL; if (*panel != NULL) { ibus_proxy_destroy ((IBusProxy *)(*panel)); @@ -479,9 +482,21 @@ _dbus_name_owner_changed_cb (BusDBusImpl *dbus, g_assert (*panel == NULL); } - connection = bus_dbus_impl_get_connection_by_name (BUS_DEFAULT_DBUS, new_name); + connection = bus_dbus_impl_get_connection_by_name (BUS_DEFAULT_DBUS, + new_name); g_return_if_fail (connection != NULL); + dbus_connection = bus_connection_get_dbus_connection (connection); + /* rhbz#1349148 rhbz#1385349 + * Avoid SEGV of BUS_IS_PANEL_PROXY (ibus->panel) + * This function is called during destroying the connection + * in this case? */ + if (dbus_connection == NULL || + g_dbus_connection_is_closed (dbus_connection)) { + new_name = ""; + break; + } + *panel = bus_panel_proxy_new (connection, panel_type); if (panel_type == PANEL_TYPE_EXTENSION_EMOJI) ibus->enable_emoji_extension = FALSE; @@ -535,7 +550,7 @@ _dbus_name_owner_changed_cb (BusDBusImpl *dbus, } } } - } + } while (0); bus_ibus_impl_component_name_owner_changed (ibus, name, old_name, new_name); } -- 2.19.1