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.
This commit is contained in:
André Aparício 2023-06-28 23:28:38 +01:00
parent 91588fb8bb
commit a1cd0acac5
8 changed files with 58 additions and 18 deletions

12
include/util/gtk_icon.hpp Normal file
View File

@ -0,0 +1,12 @@
#pragma once
#include <string>
#include <mutex>
#include <gtkmm/icontheme.h>
class DefaultGtkIconThemeWrapper {
private:
static std::mutex default_theme_mutex;
public:
static bool has_icon(const std::string&);
static Glib::RefPtr<Gdk::Pixbuf> load_icon(const char*, int, Gtk::IconLookupFlags);
};

View File

@ -173,7 +173,8 @@ src_files = files(
'src/util/prepare_for_sleep.cpp', 'src/util/prepare_for_sleep.cpp',
'src/util/ustring_clen.cpp', 'src/util/ustring_clen.cpp',
'src/util/sanitize_str.cpp', 'src/util/sanitize_str.cpp',
'src/util/rewrite_string.cpp' 'src/util/rewrite_string.cpp',
'src/util/gtk_icon.cpp'
) )
inc_dirs = ['include'] inc_dirs = ['include']

View File

@ -16,10 +16,11 @@
#include "glibmm/ustring.h" #include "glibmm/ustring.h"
#include "glibmm/variant.h" #include "glibmm/variant.h"
#include "glibmm/varianttype.h" #include "glibmm/varianttype.h"
#include "gtkmm/icontheme.h"
#include "gtkmm/label.h" #include "gtkmm/label.h"
#include "gtkmm/tooltip.h" #include "gtkmm/tooltip.h"
#include "util/gtk_icon.hpp"
namespace waybar::modules { namespace waybar::modules {
Gamemode::Gamemode(const std::string& id, const Json::Value& config) Gamemode::Gamemode(const std::string& id, const Json::Value& config)
: AModule(config, "gamemode", id), box_(Gtk::ORIENTATION_HORIZONTAL, 0), icon_(), label_() { : AModule(config, "gamemode", id), box_(Gtk::ORIENTATION_HORIZONTAL, 0), icon_(), label_() {
@ -224,7 +225,7 @@ auto Gamemode::update() -> void {
label_.set_markup(str); label_.set_markup(str);
if (useIcon) { if (useIcon) {
if (!Gtk::IconTheme::get_default()->has_icon(iconName)) { if (!DefaultGtkIconThemeWrapper::has_icon(iconName)) {
iconName = DEFAULT_ICON_NAME; iconName = DEFAULT_ICON_NAME;
} }
icon_.set_from_icon_name(iconName, Gtk::ICON_SIZE_INVALID); icon_.set_from_icon_name(iconName, Gtk::ICON_SIZE_INVALID);

View File

@ -9,6 +9,7 @@
#include <map> #include <map>
#include "util/format.hpp" #include "util/format.hpp"
#include "util/gtk_icon.hpp"
template <> template <>
struct fmt::formatter<Glib::VariantBase> : formatter<std::string> { struct fmt::formatter<Glib::VariantBase> : formatter<std::string> {
@ -379,9 +380,7 @@ Glib::RefPtr<Gdk::Pixbuf> Item::getIconByName(const std::string& name, int reque
return icon_theme->load_icon(name.c_str(), tmp_size, return icon_theme->load_icon(name.c_str(), tmp_size,
Gtk::IconLookupFlags::ICON_LOOKUP_FORCE_SIZE); Gtk::IconLookupFlags::ICON_LOOKUP_FORCE_SIZE);
} }
Glib::RefPtr<Gtk::IconTheme> default_theme = Gtk::IconTheme::get_default(); return DefaultGtkIconThemeWrapper::load_icon(name.c_str(), tmp_size,
default_theme->rescan_if_needed();
return default_theme->load_icon(name.c_str(), tmp_size,
Gtk::IconLookupFlags::ICON_LOOKUP_FORCE_SIZE); Gtk::IconLookupFlags::ICON_LOOKUP_FORCE_SIZE);
} }

View File

@ -5,7 +5,6 @@
#include <glibmm/keyfile.h> #include <glibmm/keyfile.h>
#include <glibmm/miscutils.h> #include <glibmm/miscutils.h>
#include <gtkmm/enums.h> #include <gtkmm/enums.h>
#include <gtkmm/icontheme.h>
#include <spdlog/spdlog.h> #include <spdlog/spdlog.h>
#include <filesystem> #include <filesystem>
@ -13,6 +12,7 @@
#include <string> #include <string>
#include "util/rewrite_string.hpp" #include "util/rewrite_string.hpp"
#include "util/gtk_icon.hpp"
namespace waybar::modules::sway { namespace waybar::modules::sway {
@ -81,13 +81,12 @@ std::optional<Glib::ustring> getIconName(const std::string& app_id, const std::s
if (!desktop_file_path.has_value()) { if (!desktop_file_path.has_value()) {
// Try some heuristics to find a matching icon // Try some heuristics to find a matching icon
const auto default_icon_theme = Gtk::IconTheme::get_default(); if (DefaultGtkIconThemeWrapper::has_icon(app_id)) {
if (default_icon_theme->has_icon(app_id)) {
return app_id; return app_id;
} }
const auto app_id_desktop = app_id + "-desktop"; 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; return app_id_desktop;
} }
@ -101,7 +100,7 @@ std::optional<Glib::ustring> getIconName(const std::string& app_id, const std::s
const auto first_space = app_id.find_first_of(' '); const auto first_space = app_id.find_first_of(' ');
if (first_space != std::string::npos) { if (first_space != std::string::npos) {
const auto first_word = to_lower(app_id.substr(0, first_space)); 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; return first_word;
} }
} }
@ -109,7 +108,7 @@ std::optional<Glib::ustring> getIconName(const std::string& app_id, const std::s
const auto first_dash = app_id.find_first_of('-'); const auto first_dash = app_id.find_first_of('-');
if (first_dash != std::string::npos) { if (first_dash != std::string::npos) {
const auto first_word = to_lower(app_id.substr(0, first_dash)); 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; return first_word;
} }
} }

View File

@ -5,9 +5,10 @@
#include <cstring> #include <cstring>
#include <string> #include <string>
#include "gtkmm/icontheme.h"
#include "gtkmm/tooltip.h" #include "gtkmm/tooltip.h"
#include "util/gtk_icon.hpp"
namespace waybar::modules::upower { namespace waybar::modules::upower {
UPower::UPower(const std::string& id, const Json::Value& config) UPower::UPower(const std::string& id, const Json::Value& config)
: AModule(config, "upower", id), : AModule(config, "upower", id),
@ -372,7 +373,7 @@ auto UPower::update() -> void {
label_.set_markup(onlySpaces ? "" : label_format); label_.set_markup(onlySpaces ? "" : label_format);
// Set icon // 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_name = (char*)"battery-missing-symbolic";
} }
icon_.set_from_icon_name(icon_name, Gtk::ICON_SIZE_INVALID); icon_.set_from_icon_name(icon_name, Gtk::ICON_SIZE_INVALID);

View File

@ -2,10 +2,11 @@
#include "gtkmm/box.h" #include "gtkmm/box.h"
#include "gtkmm/enums.h" #include "gtkmm/enums.h"
#include "gtkmm/icontheme.h"
#include "gtkmm/image.h" #include "gtkmm/image.h"
#include "gtkmm/label.h" #include "gtkmm/label.h"
#include "util/gtk_icon.hpp"
namespace waybar::modules::upower { namespace waybar::modules::upower {
UPowerTooltip::UPowerTooltip(uint iconSize_, uint tooltipSpacing_, uint tooltipPadding_) UPowerTooltip::UPowerTooltip(uint iconSize_, uint tooltipSpacing_, uint tooltipPadding_)
: Gtk::Window(), : Gtk::Window(),
@ -62,7 +63,7 @@ uint UPowerTooltip::updateTooltip(Devices& devices) {
std::string deviceIconName = getDeviceIcon(kind); std::string deviceIconName = getDeviceIcon(kind);
Gtk::Image* deviceIcon = new Gtk::Image(); Gtk::Image* deviceIcon = new Gtk::Image();
deviceIcon->set_pixel_size(iconSize); deviceIcon->set_pixel_size(iconSize);
if (!Gtk::IconTheme::get_default()->has_icon(deviceIconName)) { if (!DefaultGtkIconThemeWrapper::has_icon(deviceIconName)) {
deviceIconName = "battery-missing-symbolic"; deviceIconName = "battery-missing-symbolic";
} }
deviceIcon->set_from_icon_name(deviceIconName, Gtk::ICON_SIZE_INVALID); deviceIcon->set_from_icon_name(deviceIconName, Gtk::ICON_SIZE_INVALID);
@ -79,7 +80,7 @@ uint UPowerTooltip::updateTooltip(Devices& devices) {
// Set icon // Set icon
Gtk::Image* icon = new Gtk::Image(); Gtk::Image* icon = new Gtk::Image();
icon->set_pixel_size(iconSize); 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_name = (char*)"battery-missing-symbolic";
} }
icon->set_from_icon_name(icon_name, Gtk::ICON_SIZE_INVALID); icon->set_from_icon_name(icon_name, Gtk::ICON_SIZE_INVALID);

26
src/util/gtk_icon.cpp Normal file
View File

@ -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<std::mutex> lock(default_theme_mutex);
return Gtk::IconTheme::get_default()->has_icon(value);
}
Glib::RefPtr<Gdk::Pixbuf> DefaultGtkIconThemeWrapper::load_icon(const char *name, int tmp_size, Gtk::IconLookupFlags flags) {
const std::lock_guard<std::mutex> 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);
}