From 5749b4861ffcaf8083d97c6bf9858257373596ce Mon Sep 17 00:00:00 2001 From: fujiwarat Date: Wed, 25 Jan 2023 17:26:47 +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#1601577 rhbz#1797726 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. rhbz#1767691 SEGV in client/x11/main.c:_sighandler(). Do not call atexit functions in _sighandler(). 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 finalzed yet. 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 --- bus/dbusimpl.c | 47 ++++++++++++++++++++++++--- bus/engineproxy.c | 44 +++++++++++++++++++------ client/x11/main.c | 8 ++++- portal/portal.c | 11 +++++++ src/ibusbus.c | 6 ++++ ui/gtk3/extension.vala | 4 +++ ui/gtk3/switcher.vala | 73 +++++++++++++++++++++++++----------------- 7 files changed, 148 insertions(+), 45 deletions(-) diff --git a/bus/dbusimpl.c b/bus/dbusimpl.c index 59787a80..af2fbde2 100644 --- a/bus/dbusimpl.c +++ b/bus/dbusimpl.c @@ -610,6 +610,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; @@ -633,6 +634,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)); @@ -647,12 +652,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); @@ -1483,13 +1515,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 @@ -693,10 +693,12 @@ bus_engine_proxy_g_signal (GDBusProxy *proxy, g_return_if_reached (); } +#pragma GCC optimize ("O0") static BusEngineProxy * bus_engine_proxy_new_internal (const gchar *path, IBusEngineDesc *desc, - GDBusConnection *connection) + GDBusConnection *connection, + GError **error) { GDBusProxyFlags flags; BusEngineProxy *engine; @@ -706,12 +708,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 +729,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); @@ -756,6 +772,7 @@ bus_engine_proxy_new_internal (const gchar *path, return engine; } +#pragma GCC reset_options typedef struct { GTask *task; @@ -818,23 +835,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/client/x11/main.c b/client/x11/main.c index 905fd251..1abea0af 100644 --- a/client/x11/main.c +++ b/client/x11/main.c @@ -45,6 +45,7 @@ #include #include #include +#include #include @@ -1269,7 +1270,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 c2e4fc7f..a347fe6a 100644 --- a/portal/portal.c +++ b/portal/portal.c @@ -90,6 +90,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, @@ -624,6 +629,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 47400cb8..c9fbe492 100644 --- a/src/ibusbus.c +++ b/src/ibusbus.c @@ -708,6 +708,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 9400e9ba..2ecbdac1 100644 --- a/ui/gtk3/switcher.vala +++ b/ui/gtk3/switcher.vala @@ -140,8 +140,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; @@ -200,16 +200,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) { @@ -245,30 +247,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.38.1