From 988059d40c9b6cffc3039a6d7623dc73672b0ad5 Mon Sep 17 00:00:00 2001
From: fujiwarat <takao.fujiwara1@gmail.com>
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 <shawn.p.huang@gmail.com>
- * Copyright (C) 2008-2013 Red Hat, Inc.
+ * Copyright (C) 2015-2019 Takao Fujiwara <takao.fujiwara1@gmail.com>
+ * 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[] =
" </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,
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