ibus/ibus-1385349-segv-bus-proxy.patch

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