From 1286ce92a5ccf68b5dcf1b4a7c0884ce29d5c51b Mon Sep 17 00:00:00 2001 From: fujiwarat Date: Fri, 12 Jul 2024 23:30:25 +0900 Subject: [PATCH] Fix SEGV in bus_panel_proxy_focus_in() 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#1767976 SEGV in assert(connection != NULL) in bus_dbus_impl_connection_filter_cb() call bus_connection_set_filter() in bus_dbus_impl_destroy(). rhbz#2213445 SEGV in bus_panel_proxy_new() WIP: Add a GError. rhbz#1601577 rhbz#1797726 SEGV in ibus_engine_desc_get_layout() in bus_engine_proxy_new_internal() WIP: Add 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. rhbz#1767691 SEGV in client/x11/main.c:_sighandler(). Do not call atexit functions in _sighandler(). rhbz#2195895 SEGV in client/x11/main.c:_xim_set_cursor_location() check if IBusInputContext was disconnected. rhbz#1795499 rhbz#1936777 SEGV in ibus_bus_get_bus_address() because of no _bus->priv. _changed_cb() should not be called after ibus_bus_destroy() is called. rhbz#1771238 SEGV in assert(m_loop == null) in switcher.vala. Grabbing keyboard could be failed and switcher received the keyboard events and m_loop was not released. rhbz#1797120 SEGV in assert(bus.is_connected()) in panel_binding_construct() Check m_ibus in extension.vala:bus_name_acquired_cb() rhbz#2151344 SEGV with portal_context->owner in name_owner_changed() Maybe g_object_unref() is called but not finalized yet. rhbz#2239633 SEGV with g_object_unref() in ibus_portal_context_handle_destroy() Connect "handle-destroy" signal after g_list_prepend(). BUG=rhbz#1350291 BUG=rhbz#1601577 BUG=rhbz#1663528 BUG=rhbz#1767691 BUG=rhbz#1795499 BUG=rhbz#1771238 BUG=rhbz#1767976 BUG=rhbz#1797120 BUG=rhbz#2151344 BUG=rhbz#2195895 BUG=rhbz#2239633 --- bus/dbusimpl.c | 47 ++++++++++++++++++++++++--- bus/engineproxy.c | 44 +++++++++++++++++++------ bus/panelproxy.c | 9 +++++- client/x11/main.c | 56 ++++++++++++++++++++++++++++---- portal/portal.c | 25 ++++++++++++--- src/ibusbus.c | 6 ++++ ui/gtk3/extension.vala | 4 +++ ui/gtk3/switcher.vala | 73 +++++++++++++++++++++++++----------------- 8 files changed, 208 insertions(+), 56 deletions(-) diff --git a/bus/dbusimpl.c b/bus/dbusimpl.c index 110d864a..391d576a 100644 --- a/bus/dbusimpl.c +++ b/bus/dbusimpl.c @@ -621,6 +621,7 @@ static void bus_dbus_impl_destroy (BusDBusImpl *dbus) { GList *p; + int i; for (p = dbus->objects; p != NULL; p = p->next) { IBusService *object = (IBusService *) p->data; @@ -644,6 +645,10 @@ bus_dbus_impl_destroy (BusDBusImpl *dbus) for (p = dbus->connections; p != NULL; p = p->next) { BusConnection *connection = BUS_CONNECTION (p->data); + /* rhbz#1767976 Fix connection == NULL in + * bus_dbus_impl_connection_filter_cb() + */ + bus_connection_set_filter (connection, NULL, NULL, NULL); g_signal_handlers_disconnect_by_func (connection, bus_dbus_impl_connection_destroy_cb, dbus); ibus_object_destroy (IBUS_OBJECT (connection)); @@ -658,12 +663,39 @@ bus_dbus_impl_destroy (BusDBusImpl *dbus) dbus->unique_names = NULL; dbus->names = NULL; + for (i = 0; g_idle_remove_by_data (dbus); i++) { + if (i > 1000) { + 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); dbus->start_service_calls = NULL; - g_mutex_clear (&dbus->dispatch_lock); - g_mutex_clear (&dbus->forward_lock); + /* rhbz#1663528 Call g_mutex_trylock() before g_mutex_clear() + * because if the mutex is not unlocked, g_mutex_clear() causes assert. + */ +#define BUS_DBUS_MUTEX_SAFE_CLEAR(mtex) { \ + int count = 0; \ + while (!g_mutex_trylock ((mtex))) { \ + g_usleep (1); \ + if (count > 60) { \ + g_warning (#mtex " is dead lock"); \ + break; \ + } \ + ++count; \ + } \ + g_mutex_unlock ((mtex)); \ + g_mutex_clear ((mtex)); \ +} + + BUS_DBUS_MUTEX_SAFE_CLEAR (&dbus->dispatch_lock); + BUS_DBUS_MUTEX_SAFE_CLEAR (&dbus->forward_lock); + +#undef BUS_DBUS_MUTEX_SAFE_CLEAR /* FIXME destruct _lock and _queue members. */ IBUS_OBJECT_CLASS(bus_dbus_impl_parent_class)->destroy ((IBusObject *)dbus); @@ -1539,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 */ diff --git a/bus/engineproxy.c b/bus/engineproxy.c index b3e16066..ba479b59 100644 --- a/bus/engineproxy.c +++ b/bus/engineproxy.c @@ -696,7 +696,8 @@ bus_engine_proxy_g_signal (GDBusProxy *proxy, static BusEngineProxy * bus_engine_proxy_new_internal (const gchar *path, IBusEngineDesc *desc, - GDBusConnection *connection) + GDBusConnection *connection, + GError **error) { GDBusProxyFlags flags; BusEngineProxy *engine; @@ -706,12 +707,20 @@ bus_engine_proxy_new_internal (const gchar *path, g_assert (path); g_assert (IBUS_IS_ENGINE_DESC (desc)); g_assert (G_IS_DBUS_CONNECTION (connection)); + g_assert (error && *error == NULL); + /* rhbz#1601577 engine == NULL if connection is closed. */ + if (g_dbus_connection_is_closed (connection)) { + *error = g_error_new (G_DBUS_ERROR, + G_DBUS_ERROR_FAILED, + "Connection is closed."); + return NULL; + } flags = G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START; engine = (BusEngineProxy *) g_initable_new ( BUS_TYPE_ENGINE_PROXY, NULL, - NULL, + error, "desc", desc, "g-connection", connection, "g-interface-name", IBUS_INTERFACE_ENGINE, @@ -719,6 +728,12 @@ bus_engine_proxy_new_internal (const gchar *path, "g-default-timeout", g_gdbus_timeout, "g-flags", flags, NULL); + /* FIXME: rhbz#1601577 */ + if (!engine) { + /* 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); @@ -818,23 +833,30 @@ create_engine_ready_cb (BusFactoryProxy *factory, GAsyncResult *res, EngineProxyNewData *data) { + GError *error = NULL; + gchar *path; + BusEngineProxy *engine; + g_return_if_fail (data->task != NULL); - GError *error = NULL; - gchar *path = bus_factory_proxy_create_engine_finish (factory, - res, - &error); + path = bus_factory_proxy_create_engine_finish (factory, res, &error); if (path == NULL) { g_task_return_error (data->task, error); engine_proxy_new_data_free (data); return; } - BusEngineProxy *engine = - bus_engine_proxy_new_internal (path, - data->desc, - g_dbus_proxy_get_connection ((GDBusProxy *)data->factory)); + engine = bus_engine_proxy_new_internal ( + path, + data->desc, + g_dbus_proxy_get_connection ((GDBusProxy *)data->factory), + &error); g_free (path); + if (!engine) { + g_task_return_error (data->task, error); + engine_proxy_new_data_free (data); + return; + } /* FIXME: set destroy callback ? */ g_task_return_pointer (data->task, engine, NULL); diff --git a/bus/panelproxy.c b/bus/panelproxy.c index e6001ebf..00828fbc 100644 --- a/bus/panelproxy.c +++ b/bus/panelproxy.c @@ -122,6 +122,8 @@ bus_panel_proxy_new (BusConnection *connection, const gchar *path = NULL; GObject *obj; BusPanelProxy *panel; + GError *error = NULL; + const gchar *message; g_assert (BUS_IS_CONNECTION (connection)); @@ -138,7 +140,7 @@ bus_panel_proxy_new (BusConnection *connection, obj = g_initable_new (BUS_TYPE_PANEL_PROXY, NULL, - NULL, + &error, "g-object-path", path, "g-interface-name", IBUS_INTERFACE_PANEL, "g-connection", bus_connection_get_dbus_connection (connection), @@ -146,6 +148,11 @@ bus_panel_proxy_new (BusConnection *connection, "g-flags", G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START | G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, NULL); + if (error) { + /* TODO: rhbz#2213445 Why does this issue happen? */ + message = error->message; + g_critical ("Failed to generate BusPanelProxy: %s", message); + } panel = BUS_PANEL_PROXY (obj); panel->panel_type = panel_type; return panel; diff --git a/client/x11/main.c b/client/x11/main.c index b7eb5961..3075d5d0 100644 --- a/client/x11/main.c +++ b/client/x11/main.c @@ -45,6 +45,7 @@ #include #include #include +#include #include @@ -69,6 +70,7 @@ typedef struct _X11ICONN X11ICONN; typedef struct _X11IC X11IC; struct _X11IC { IBusInputContext *context; + gboolean ibus_connected; Window client_window; Window focus_window; gint32 input_style; @@ -327,6 +329,18 @@ _xim_store_ic_values (X11IC *x11ic, IMChangeICStruct *call_data) return 1; } +static void +ibus_ic_connection_closed_cb (GDBusConnection *connection, + gboolean remote_peer_vanished, + GError *error, + X11IC *x11ic) +{ + /* rhbz#2195895 The moment of the IBusBus disconnection would be + * different from the moment of XIM_DISCONNECT. + */ + x11ic->ibus_connected = FALSE; +} + static int xim_create_ic (XIMS xims, IMChangeICStruct *call_data) @@ -334,6 +348,7 @@ xim_create_ic (XIMS xims, IMChangeICStruct *call_data) static int base_icid = 1; X11IC *x11ic; guint32 capabilities = IBUS_CAP_FOCUS; + GDBusConnection *connection; call_data->icid = base_icid ++; @@ -345,8 +360,9 @@ xim_create_ic (XIMS xims, IMChangeICStruct *call_data) x11ic->icid = call_data->icid; x11ic->connect_id = call_data->connect_id; - x11ic->conn = (X11ICONN *)g_hash_table_lookup (_connections, - GINT_TO_POINTER ((gint) call_data->connect_id)); + x11ic->conn = (X11ICONN *)g_hash_table_lookup ( + _connections, + GINT_TO_POINTER ((gint) call_data->connect_id)); if (x11ic->conn == NULL) { g_slice_free (X11IC, x11ic); g_return_val_if_reached (0); @@ -376,6 +392,10 @@ xim_create_ic (XIMS xims, IMChangeICStruct *call_data) G_CALLBACK (_context_enabled_cb), x11ic); g_signal_connect (x11ic->context, "disabled", G_CALLBACK (_context_disabled_cb), x11ic); + connection = g_dbus_proxy_get_connection (G_DBUS_PROXY (x11ic->context)); + x11ic->ibus_connected = !g_dbus_connection_is_closed (connection); + g_signal_connect (connection, "closed", + G_CALLBACK (ibus_ic_connection_closed_cb), x11ic); if (x11ic->input_style & XIMPreeditCallbacks) @@ -400,11 +420,19 @@ xim_destroy_ic (XIMS xims, IMChangeICStruct *call_data) LOG (1, "XIM_DESTROY_IC ic=%d connect_id=%d", call_data->icid, call_data->connect_id); - x11ic = (X11IC *)g_hash_table_lookup (_x11_ic_table, - GINT_TO_POINTER ((gint) call_data->icid)); + x11ic = (X11IC *)g_hash_table_lookup ( + _x11_ic_table, + GINT_TO_POINTER ((gint) call_data->icid)); g_return_val_if_fail (x11ic != NULL, 0); if (x11ic->context) { + GDBusConnection *connection = + g_dbus_proxy_get_connection (G_DBUS_PROXY (x11ic->context)); + x11ic->ibus_connected = FALSE; + g_signal_handlers_disconnect_by_func ( + connection, + (GCallback)ibus_ic_connection_closed_cb, + x11ic); ibus_proxy_destroy ((IBusProxy *)x11ic->context); g_object_unref (x11ic->context); x11ic->context = NULL; @@ -412,7 +440,8 @@ xim_destroy_ic (XIMS xims, IMChangeICStruct *call_data) g_hash_table_remove (_x11_ic_table, GINT_TO_POINTER ((gint) call_data->icid)); - x11ic->conn->clients = g_list_remove (x11ic->conn->clients, (gconstpointer)x11ic); + x11ic->conn->clients = g_list_remove (x11ic->conn->clients, + (gconstpointer)x11ic); g_free (x11ic->preedit_string); x11ic->preedit_string = NULL; @@ -439,6 +468,8 @@ xim_set_ic_focus (XIMS xims, IMChangeFocusStruct *call_data) x11ic = (X11IC *) g_hash_table_lookup (_x11_ic_table, GINT_TO_POINTER ((gint) call_data->icid)); g_return_val_if_fail (x11ic != NULL, 0); + if (!x11ic->ibus_connected) + return 1; ibus_input_context_focus_in (x11ic->context); _xim_set_cursor_location (x11ic); @@ -458,6 +489,8 @@ xim_unset_ic_focus (XIMS xims, IMChangeFocusStruct *call_data) x11ic = (X11IC *) g_hash_table_lookup (_x11_ic_table, GINT_TO_POINTER ((gint) call_data->icid)); g_return_val_if_fail (x11ic != NULL, 0); + if (!x11ic->ibus_connected) + return 1; ibus_input_context_focus_out (x11ic->context); @@ -712,6 +745,8 @@ xim_forward_event (XIMS xims, IMForwardEventStruct *call_data) _x11_ic_table, GINT_TO_POINTER ((gint) call_data->icid)); g_return_val_if_fail (x11ic != NULL, 0); + if (!x11ic->ibus_connected) + return 0; xevent = (XKeyEvent*) &(call_data->event); @@ -870,6 +905,8 @@ _xim_set_cursor_location (X11IC *x11ic) } } + if (!x11ic->ibus_connected) + return; ibus_input_context_set_cursor_location (x11ic->context, preedit_area.x, preedit_area.y, @@ -950,6 +987,8 @@ xim_reset_ic (XIMS xims, IMResetICStruct *call_data) x11ic = (X11IC *) g_hash_table_lookup (_x11_ic_table, GINT_TO_POINTER ((gint) call_data->icid)); g_return_val_if_fail (x11ic != NULL, 0); + if (!x11ic->ibus_connected) + return 1; ibus_input_context_reset (x11ic->context); @@ -1309,7 +1348,12 @@ _atexit_cb () static void _sighandler (int sig) { - exit(EXIT_FAILURE); + /* rhbz#1767691 _sighandler() is called with SIGTERM + * and exit() causes SEGV during calling atexit functions. + * _atexit_cb() might be broken. _exit() does not call + * atexit functions. + */ + _exit(EXIT_FAILURE); } static void diff --git a/portal/portal.c b/portal/portal.c index 5cd38779..5110baad 100644 --- a/portal/portal.c +++ b/portal/portal.c @@ -92,6 +92,11 @@ static void portal_context_g_signal (GDBusProxy *proxy, GVariant *parameters, IBusPortalContext *portal_context); +#define IBUS_TYPE_PORTAL_CONTEXT \ + (ibus_portal_context_get_type ()) +#define IBUS_IS_PORTAL_CONTEXT(obj) \ + (G_TYPE_CHECK_INSTANCE_TYPE ((obj), IBUS_TYPE_PORTAL_CONTEXT)) + G_DEFINE_TYPE_WITH_CODE (IBusPortalContext, ibus_portal_context, IBUS_DBUS_TYPE_INPUT_CONTEXT_SKELETON, @@ -492,11 +497,6 @@ ibus_portal_context_new (IBusInputContext *context, g_strdup_printf (IBUS_PATH_INPUT_CONTEXT, portal_context->id); portal_context->service = ibus_dbus_service_skeleton_new (); - g_signal_connect (portal_context->service, - "handle-destroy", - G_CALLBACK (ibus_portal_context_handle_destroy), - portal_context); - if (!g_dbus_interface_skeleton_export ( G_DBUS_INTERFACE_SKELETON (portal_context->service), connection, portal_context->object_path, @@ -509,8 +509,17 @@ ibus_portal_context_new (IBusInputContext *context, return NULL; } + /* rhbz#2239633 g_list_prepend() needs to be callsed before + * ibus_portal_context_handle_destroy() is connected + * for g_list_remove() in ibus_portal_context_finalize(). + */ all_contexts = g_list_prepend (all_contexts, portal_context); + g_signal_connect (portal_context->service, + "handle-destroy", + G_CALLBACK (ibus_portal_context_handle_destroy), + portal_context); + return portal_context; } @@ -667,6 +676,12 @@ name_owner_changed (GDBusConnection *connection, IBusPortalContext *portal_context = l->data; next = l->next; + /* rhbz#2151344 portal_context might not be finalized? */ + if (!G_LIKELY (IBUS_IS_PORTAL_CONTEXT (portal_context))) { + g_warn_message (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, + "portal_context is not IBusPortalContext"); + continue; + } if (g_strcmp0 (portal_context->owner, name) == 0) { g_object_unref (portal_context); } diff --git a/src/ibusbus.c b/src/ibusbus.c index 0e6d67f1..fcc742b6 100644 --- a/src/ibusbus.c +++ b/src/ibusbus.c @@ -742,6 +742,12 @@ ibus_bus_destroy (IBusObject *object) _bus = NULL; if (bus->priv->monitor) { + /* rhbz#1795499 _changed_cb() causes SEGV because of no bus->priv + * after ibus_bus_destroy() is called. + */ + g_signal_handlers_disconnect_by_func (bus->priv->monitor, + (GCallback) _changed_cb, bus); + g_file_monitor_cancel (bus->priv->monitor); g_object_unref (bus->priv->monitor); bus->priv->monitor = NULL; } diff --git a/ui/gtk3/extension.vala b/ui/gtk3/extension.vala index a6f2e8e6..b7a04081 100644 --- a/ui/gtk3/extension.vala +++ b/ui/gtk3/extension.vala @@ -73,6 +73,10 @@ class ExtensionGtk : Gtk.Application { string signal_name, Variant parameters) { debug("signal_name = %s", signal_name); + /* rhbz#1797120 Fix assert(bus.is_connected()) in + * panel_binding_construct() + */ + return_if_fail(m_bus.is_connected()); m_panel = new PanelBinding(m_bus, this); m_panel.load_settings(); } diff --git a/ui/gtk3/switcher.vala b/ui/gtk3/switcher.vala index 26bded99..21ede7be 100644 --- a/ui/gtk3/switcher.vala +++ b/ui/gtk3/switcher.vala @@ -176,8 +176,8 @@ class Switcher : Gtk.Window { IBus.EngineDesc[] engines, int index, string input_context_path) { - assert (m_loop == null); - assert (index < engines.length); + assert(m_loop == null); + assert(index < engines.length); if (m_is_running) return index; @@ -236,16 +236,18 @@ class Switcher : Gtk.Window { null, event, null); - if (status != Gdk.GrabStatus.SUCCESS) + if (status != Gdk.GrabStatus.SUCCESS) { warning("Grab keyboard failed! status = %d", status); - status = seat.grab(get_window(), - Gdk.SeatCapabilities.POINTER, - true, - null, - event, - null); - if (status != Gdk.GrabStatus.SUCCESS) - warning("Grab pointer failed! status = %d", status); + } else { + status = seat.grab(get_window(), + Gdk.SeatCapabilities.POINTER, + true, + null, + event, + null); + if (status != Gdk.GrabStatus.SUCCESS) + warning("Grab pointer failed! status = %d", status); + } #else Gdk.Device device = event.get_device(); if (device == null) { @@ -281,30 +283,41 @@ class Switcher : Gtk.Window { Gdk.EventMask.KEY_RELEASE_MASK, null, Gdk.CURRENT_TIME); - if (status != Gdk.GrabStatus.SUCCESS) + if (status != Gdk.GrabStatus.SUCCESS) { warning("Grab keyboard failed! status = %d", status); - // Grab all pointer events - status = pointer.grab(get_window(), - Gdk.GrabOwnership.NONE, - true, - Gdk.EventMask.BUTTON_PRESS_MASK | - Gdk.EventMask.BUTTON_RELEASE_MASK, - null, - Gdk.CURRENT_TIME); - if (status != Gdk.GrabStatus.SUCCESS) - warning("Grab pointer failed! status = %d", status); + } else { + // Grab all pointer events + status = pointer.grab(get_window(), + Gdk.GrabOwnership.NONE, + true, + Gdk.EventMask.BUTTON_PRESS_MASK | + Gdk.EventMask.BUTTON_RELEASE_MASK, + null, + Gdk.CURRENT_TIME); + if (status != Gdk.GrabStatus.SUCCESS) + warning("Grab pointer failed! status = %d", status); + } #endif - // Probably we can delete m_popup_delay_time in 1.6 - pointer.get_position_double(null, - out m_mouse_init_x, - out m_mouse_init_y); - m_mouse_moved = false; + /* Fix RHBZ #1771238 assert(m_loop == null) + * Grabbing keyboard can be failed when the second Super-e is typed + * before Switcher dialog is focused. And m_loop could not be released + * if the failed Super-e would call m_loop.run() below and could not + * call key_release_event(). And m_loop == null would be false in the + * third Super-e. + */ + if (status == Gdk.GrabStatus.SUCCESS) { + // Probably we can delete m_popup_delay_time in 1.6 + pointer.get_position_double(null, + out m_mouse_init_x, + out m_mouse_init_y); + m_mouse_moved = false; - m_loop = new GLib.MainLoop(); - m_loop.run(); - m_loop = null; + m_loop = new GLib.MainLoop(); + m_loop.run(); + m_loop = null; + } #if VALA_0_34 seat.ungrab(); -- 2.45.0