From a1cd0acac5d35659fb062923da2eb3771fd79c56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Apar=C3=ADcio?= Date: Wed, 28 Jun 2023 23:28:38 +0100 Subject: [PATCH] Fix random segfault on GTK icon functions The segfaults were happening on GTK icon theme functions, which are called via the C++ interface functions such as Gtk::IconTheme::has_icon. There are multiple modules and threads using this functions on the default icon theme by calling Gtk::IconTheme::get_default(), which returns the same object for all callers, and was causing concurrent access to the same internal data structures on the GTK lib. Even a seemingly read-only function such as has_icon can cause writes due to the internal icon cache being updated. To avoid this issues, a program wide global mutex must be used to ensure a single thread is accessing the default icon theme instance. This commit implements wrappers for the existing IconTheme function calls, ensuring the global lock is held while calling the underling GTK functions. --- include/util/gtk_icon.hpp | 12 ++++++++++++ meson.build | 3 ++- src/modules/gamemode.cpp | 5 +++-- src/modules/sni/item.cpp | 7 +++---- src/modules/sway/window.cpp | 11 +++++------ src/modules/upower/upower.cpp | 5 +++-- src/modules/upower/upower_tooltip.cpp | 7 ++++--- src/util/gtk_icon.cpp | 26 ++++++++++++++++++++++++++ 8 files changed, 58 insertions(+), 18 deletions(-) create mode 100644 include/util/gtk_icon.hpp create mode 100644 src/util/gtk_icon.cpp diff --git a/include/util/gtk_icon.hpp b/include/util/gtk_icon.hpp new file mode 100644 index 0000000..67261c1 --- /dev/null +++ b/include/util/gtk_icon.hpp @@ -0,0 +1,12 @@ +#pragma once +#include +#include +#include + +class DefaultGtkIconThemeWrapper { + private: + static std::mutex default_theme_mutex; + public: + static bool has_icon(const std::string&); + static Glib::RefPtr load_icon(const char*, int, Gtk::IconLookupFlags); +}; diff --git a/meson.build b/meson.build index 87c03bf..aa250b7 100644 --- a/meson.build +++ b/meson.build @@ -173,7 +173,8 @@ src_files = files( 'src/util/prepare_for_sleep.cpp', 'src/util/ustring_clen.cpp', 'src/util/sanitize_str.cpp', - 'src/util/rewrite_string.cpp' + 'src/util/rewrite_string.cpp', + 'src/util/gtk_icon.cpp' ) inc_dirs = ['include'] diff --git a/src/modules/gamemode.cpp b/src/modules/gamemode.cpp index 3b2213e..10a7ec7 100644 --- a/src/modules/gamemode.cpp +++ b/src/modules/gamemode.cpp @@ -16,10 +16,11 @@ #include "glibmm/ustring.h" #include "glibmm/variant.h" #include "glibmm/varianttype.h" -#include "gtkmm/icontheme.h" #include "gtkmm/label.h" #include "gtkmm/tooltip.h" +#include "util/gtk_icon.hpp" + namespace waybar::modules { Gamemode::Gamemode(const std::string& id, const Json::Value& config) : AModule(config, "gamemode", id), box_(Gtk::ORIENTATION_HORIZONTAL, 0), icon_(), label_() { @@ -224,7 +225,7 @@ auto Gamemode::update() -> void { label_.set_markup(str); if (useIcon) { - if (!Gtk::IconTheme::get_default()->has_icon(iconName)) { + if (!DefaultGtkIconThemeWrapper::has_icon(iconName)) { iconName = DEFAULT_ICON_NAME; } icon_.set_from_icon_name(iconName, Gtk::ICON_SIZE_INVALID); diff --git a/src/modules/sni/item.cpp b/src/modules/sni/item.cpp index e5bd6ab..9d3fc4b 100644 --- a/src/modules/sni/item.cpp +++ b/src/modules/sni/item.cpp @@ -9,6 +9,7 @@ #include #include "util/format.hpp" +#include "util/gtk_icon.hpp" template <> struct fmt::formatter : formatter { @@ -379,10 +380,8 @@ Glib::RefPtr Item::getIconByName(const std::string& name, int reque return icon_theme->load_icon(name.c_str(), tmp_size, Gtk::IconLookupFlags::ICON_LOOKUP_FORCE_SIZE); } - Glib::RefPtr default_theme = Gtk::IconTheme::get_default(); - default_theme->rescan_if_needed(); - return default_theme->load_icon(name.c_str(), tmp_size, - Gtk::IconLookupFlags::ICON_LOOKUP_FORCE_SIZE); + return DefaultGtkIconThemeWrapper::load_icon(name.c_str(), tmp_size, + Gtk::IconLookupFlags::ICON_LOOKUP_FORCE_SIZE); } double Item::getScaledIconSize() { diff --git a/src/modules/sway/window.cpp b/src/modules/sway/window.cpp index 030f93b..6f97b2e 100644 --- a/src/modules/sway/window.cpp +++ b/src/modules/sway/window.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #include #include @@ -13,6 +12,7 @@ #include #include "util/rewrite_string.hpp" +#include "util/gtk_icon.hpp" namespace waybar::modules::sway { @@ -81,13 +81,12 @@ std::optional getIconName(const std::string& app_id, const std::s if (!desktop_file_path.has_value()) { // Try some heuristics to find a matching icon - const auto default_icon_theme = Gtk::IconTheme::get_default(); - if (default_icon_theme->has_icon(app_id)) { + if (DefaultGtkIconThemeWrapper::has_icon(app_id)) { return app_id; } const auto app_id_desktop = app_id + "-desktop"; - if (default_icon_theme->has_icon(app_id_desktop)) { + if (DefaultGtkIconThemeWrapper::has_icon(app_id_desktop)) { return app_id_desktop; } @@ -101,7 +100,7 @@ std::optional getIconName(const std::string& app_id, const std::s const auto first_space = app_id.find_first_of(' '); if (first_space != std::string::npos) { const auto first_word = to_lower(app_id.substr(0, first_space)); - if (default_icon_theme->has_icon(first_word)) { + if (DefaultGtkIconThemeWrapper::has_icon(first_word)) { return first_word; } } @@ -109,7 +108,7 @@ std::optional getIconName(const std::string& app_id, const std::s const auto first_dash = app_id.find_first_of('-'); if (first_dash != std::string::npos) { const auto first_word = to_lower(app_id.substr(0, first_dash)); - if (default_icon_theme->has_icon(first_word)) { + if (DefaultGtkIconThemeWrapper::has_icon(first_word)) { return first_word; } } diff --git a/src/modules/upower/upower.cpp b/src/modules/upower/upower.cpp index 7aafdc6..ea03934 100644 --- a/src/modules/upower/upower.cpp +++ b/src/modules/upower/upower.cpp @@ -5,9 +5,10 @@ #include #include -#include "gtkmm/icontheme.h" #include "gtkmm/tooltip.h" +#include "util/gtk_icon.hpp" + namespace waybar::modules::upower { UPower::UPower(const std::string& id, const Json::Value& config) : AModule(config, "upower", id), @@ -372,7 +373,7 @@ auto UPower::update() -> void { label_.set_markup(onlySpaces ? "" : label_format); // Set icon - if (icon_name == NULL || !Gtk::IconTheme::get_default()->has_icon(icon_name)) { + if (icon_name == NULL || !DefaultGtkIconThemeWrapper::has_icon(icon_name)) { icon_name = (char*)"battery-missing-symbolic"; } icon_.set_from_icon_name(icon_name, Gtk::ICON_SIZE_INVALID); diff --git a/src/modules/upower/upower_tooltip.cpp b/src/modules/upower/upower_tooltip.cpp index 7dd5d10..fddb688 100644 --- a/src/modules/upower/upower_tooltip.cpp +++ b/src/modules/upower/upower_tooltip.cpp @@ -2,10 +2,11 @@ #include "gtkmm/box.h" #include "gtkmm/enums.h" -#include "gtkmm/icontheme.h" #include "gtkmm/image.h" #include "gtkmm/label.h" +#include "util/gtk_icon.hpp" + namespace waybar::modules::upower { UPowerTooltip::UPowerTooltip(uint iconSize_, uint tooltipSpacing_, uint tooltipPadding_) : Gtk::Window(), @@ -62,7 +63,7 @@ uint UPowerTooltip::updateTooltip(Devices& devices) { std::string deviceIconName = getDeviceIcon(kind); Gtk::Image* deviceIcon = new Gtk::Image(); deviceIcon->set_pixel_size(iconSize); - if (!Gtk::IconTheme::get_default()->has_icon(deviceIconName)) { + if (!DefaultGtkIconThemeWrapper::has_icon(deviceIconName)) { deviceIconName = "battery-missing-symbolic"; } deviceIcon->set_from_icon_name(deviceIconName, Gtk::ICON_SIZE_INVALID); @@ -79,7 +80,7 @@ uint UPowerTooltip::updateTooltip(Devices& devices) { // Set icon Gtk::Image* icon = new Gtk::Image(); icon->set_pixel_size(iconSize); - if (icon_name == NULL || !Gtk::IconTheme::get_default()->has_icon(icon_name)) { + if (icon_name == NULL || !DefaultGtkIconThemeWrapper::has_icon(icon_name)) { icon_name = (char*)"battery-missing-symbolic"; } icon->set_from_icon_name(icon_name, Gtk::ICON_SIZE_INVALID); diff --git a/src/util/gtk_icon.cpp b/src/util/gtk_icon.cpp new file mode 100644 index 0000000..70aeec2 --- /dev/null +++ b/src/util/gtk_icon.cpp @@ -0,0 +1,26 @@ +#include "util/gtk_icon.hpp" + +/* We need a global mutex for accessing the object returned by Gtk::IconTheme::get_default() + * because it always returns the same object across different threads, and concurrent + * access can cause data corruption and lead to invalid memory access and crashes. + * Even concurrent calls that seem read only such as has_icon can cause issues because + * the GTK lib may update the internal icon cache on this calls. +*/ + +std::mutex DefaultGtkIconThemeWrapper::default_theme_mutex; + +bool DefaultGtkIconThemeWrapper::has_icon(const std::string& value) { + + const std::lock_guard lock(default_theme_mutex); + + return Gtk::IconTheme::get_default()->has_icon(value); +} + +Glib::RefPtr DefaultGtkIconThemeWrapper::load_icon(const char *name, int tmp_size, Gtk::IconLookupFlags flags) { + + const std::lock_guard lock(default_theme_mutex); + + auto default_theme = Gtk::IconTheme::get_default(); + default_theme->rescan_if_needed(); + return default_theme->load_icon(name, tmp_size, flags); +}