From dd1de3efbff2badb3d3c5ae0813dcd7bf6c80036 Mon Sep 17 00:00:00 2001 From: Tamino Bauknecht Date: Mon, 23 Oct 2023 01:14:52 +0200 Subject: [PATCH] Revert "Revert "Fix potential memory leaks"" This reverts commit 2d33c20231a5baf3a427837ceead9046d5152f96 and reapplies various patches for memory leaks. The reason for the revert was a bug for a maximum duration interval which caused sleep_for() to cause unpredictable behavior. --- include/modules/upower/upower.hpp | 2 +- include/modules/upower/upower_tooltip.hpp | 3 ++- include/util/scope_guard.hpp | 21 ++++++++++++++++ src/ALabel.cpp | 2 +- src/modules/bluetooth.cpp | 8 +++++- src/modules/cpu_usage/bsd.cpp | 7 +++++- src/modules/custom.cpp | 8 +++++- src/modules/mpris/mpris.cpp | 30 +++++++++++++++-------- src/modules/sni/host.cpp | 16 +++++++++--- src/modules/sni/watcher.cpp | 8 +++++- src/modules/upower/upower.cpp | 8 +++--- src/modules/upower/upower_tooltip.cpp | 3 +-- src/util/prepare_for_sleep.cpp | 3 +-- 13 files changed, 90 insertions(+), 29 deletions(-) create mode 100644 include/util/scope_guard.hpp diff --git a/include/modules/upower/upower.hpp b/include/modules/upower/upower.hpp index 446d1f53..9c1a1830 100644 --- a/include/modules/upower/upower.hpp +++ b/include/modules/upower/upower.hpp @@ -69,7 +69,7 @@ class UPower : public AModule { UpDevice *displayDevice; guint login1_id; GDBusConnection *login1_connection; - UPowerTooltip *upower_tooltip; + std::unique_ptr upower_tooltip; std::string lastStatus; bool showAltText; bool upowerRunning; diff --git a/include/modules/upower/upower_tooltip.hpp b/include/modules/upower/upower_tooltip.hpp index 05e9dcb3..bc99abed 100644 --- a/include/modules/upower/upower_tooltip.hpp +++ b/include/modules/upower/upower_tooltip.hpp @@ -2,6 +2,7 @@ #include +#include #include #include "gtkmm/box.h" @@ -16,7 +17,7 @@ class UPowerTooltip : public Gtk::Window { const std::string getDeviceIcon(UpDeviceKind& kind); - Gtk::Box* contentBox; + std::unique_ptr contentBox; uint iconSize; uint tooltipSpacing; diff --git a/include/util/scope_guard.hpp b/include/util/scope_guard.hpp new file mode 100644 index 00000000..5c717704 --- /dev/null +++ b/include/util/scope_guard.hpp @@ -0,0 +1,21 @@ +#pragma once + +#include + +namespace waybar::util { + +template +class scope_guard { + public: + explicit scope_guard(Func&& exit_function) : f{std::forward(exit_function)} {} + scope_guard(const scope_guard&) = delete; + scope_guard(scope_guard&&) = default; + scope_guard& operator=(const scope_guard&) = delete; + scope_guard& operator=(scope_guard&&) = default; + ~scope_guard() { f(); } + + private: + Func f; +}; + +} // namespace waybar::util diff --git a/src/ALabel.cpp b/src/ALabel.cpp index 4d8b2218..c87e3228 100644 --- a/src/ALabel.cpp +++ b/src/ALabel.cpp @@ -12,7 +12,7 @@ ALabel::ALabel(const Json::Value& config, const std::string& name, const std::st : AModule(config, name, id, config["format-alt"].isString() || enable_click, enable_scroll), format_(config_["format"].isString() ? config_["format"].asString() : format), interval_(config_["interval"] == "once" - ? std::chrono::seconds(100000000) + ? std::chrono::seconds::max() : std::chrono::seconds( config_["interval"].isUInt() ? config_["interval"].asUInt() : interval)), default_format_(format_) { diff --git a/src/modules/bluetooth.cpp b/src/modules/bluetooth.cpp index 9e207507..6b6cb7ef 100644 --- a/src/modules/bluetooth.cpp +++ b/src/modules/bluetooth.cpp @@ -6,12 +6,19 @@ #include #include +#include "util/scope_guard.hpp" + namespace { using GDBusManager = std::unique_ptr; auto generateManager() -> GDBusManager { GError* error = nullptr; + waybar::util::scope_guard error_deleter([error]() { + if (error) { + g_error_free(error); + } + }); GDBusObjectManager* manager = g_dbus_object_manager_client_new_for_bus_sync( G_BUS_TYPE_SYSTEM, GDBusObjectManagerClientFlags::G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_DO_NOT_AUTO_START, @@ -19,7 +26,6 @@ auto generateManager() -> GDBusManager { if (error) { spdlog::error("g_dbus_object_manager_client_new_for_bus_sync() failed: {}", error->message); - g_error_free(error); } auto destructor = [](GDBusObjectManager* manager) { diff --git a/src/modules/cpu_usage/bsd.cpp b/src/modules/cpu_usage/bsd.cpp index c987a770..663bc0a7 100644 --- a/src/modules/cpu_usage/bsd.cpp +++ b/src/modules/cpu_usage/bsd.cpp @@ -9,6 +9,7 @@ #include // malloc #include "modules/cpu_usage.hpp" +#include "util/scope_guard.hpp" #if defined(__NetBSD__) || defined(__OpenBSD__) #include @@ -33,6 +34,11 @@ std::vector> waybar::modules::CpuUsage::parseCpuinfo( int ncpu = sysconf(_SC_NPROCESSORS_CONF); size_t sz = CPUSTATES * (ncpu + 1) * sizeof(pcp_time_t); pcp_time_t *cp_time = static_cast(malloc(sz)), *pcp_time = cp_time; + waybar::util::scope_guard cp_time_deleter([cp_time]() { + if (cp_time) { + free(cp_time); + } + }); #if defined(__NetBSD__) int mib[] = { CTL_KERN, @@ -97,6 +103,5 @@ std::vector> waybar::modules::CpuUsage::parseCpuinfo( } cpuinfo.emplace_back(single_cp_time[CP_IDLE], total); } - free(cp_time); return cpuinfo; } diff --git a/src/modules/custom.cpp b/src/modules/custom.cpp index 4889b7a3..d0a39802 100644 --- a/src/modules/custom.cpp +++ b/src/modules/custom.cpp @@ -2,6 +2,8 @@ #include +#include "util/scope_guard.hpp" + waybar::modules::Custom::Custom(const std::string& name, const std::string& id, const Json::Value& config) : ALabel(config, "custom-" + name, id, "{}"), @@ -57,6 +59,11 @@ void waybar::modules::Custom::continuousWorker() { } thread_ = [this, cmd] { char* buff = nullptr; + waybar::util::scope_guard buff_deleter([buff]() { + if (buff) { + free(buff); + } + }); size_t len = 0; if (getline(&buff, &len, fp_) == -1) { int exit_code = 1; @@ -90,7 +97,6 @@ void waybar::modules::Custom::continuousWorker() { output_ = {0, output}; dp.emit(); } - free(buff); }; } diff --git a/src/modules/mpris/mpris.cpp b/src/modules/mpris/mpris.cpp index 9c17f1e8..02138af7 100644 --- a/src/modules/mpris/mpris.cpp +++ b/src/modules/mpris/mpris.cpp @@ -6,6 +6,8 @@ #include #include +#include "util/scope_guard.hpp" + extern "C" { #include } @@ -117,6 +119,11 @@ Mpris::Mpris(const std::string& id, const Json::Value& config) } GError* error = nullptr; + waybar::util::scope_guard error_deleter([error]() { + if (error) { + g_error_free(error); + } + }); manager = playerctl_player_manager_new(&error); if (error) { throw std::runtime_error(fmt::format("unable to create MPRIS client: {}", error->message)); @@ -136,9 +143,7 @@ Mpris::Mpris(const std::string& id, const Json::Value& config) } else { GList* players = playerctl_list_players(&error); if (error) { - auto e = fmt::format("unable to list players: {}", error->message); - g_error_free(error); - throw std::runtime_error(e); + throw std::runtime_error(fmt::format("unable to list players: {}", error->message)); } for (auto p = players; p != NULL; p = p->next) { @@ -410,8 +415,7 @@ auto Mpris::onPlayerNameAppeared(PlayerctlPlayerManager* manager, PlayerctlPlaye return; } - GError* error = nullptr; - mpris->player = playerctl_player_new_from_name(player_name, &error); + mpris->player = playerctl_player_new_from_name(player_name, NULL); g_object_connect(mpris->player, "signal::play", G_CALLBACK(onPlayerPlay), mpris, "signal::pause", G_CALLBACK(onPlayerPause), mpris, "signal::stop", G_CALLBACK(onPlayerStop), mpris, "signal::stop", G_CALLBACK(onPlayerStop), mpris, "signal::metadata", @@ -478,6 +482,11 @@ auto Mpris::getPlayerInfo() -> std::optional { } GError* error = nullptr; + waybar::util::scope_guard error_deleter([error]() { + if (error) { + g_error_free(error); + } + }); char* player_status = nullptr; auto player_playback_status = PLAYERCTL_PLAYBACK_STATUS_STOPPED; @@ -487,9 +496,7 @@ auto Mpris::getPlayerInfo() -> std::optional { if (player_name == "playerctld") { GList* players = playerctl_list_players(&error); if (error) { - auto e = fmt::format("unable to list players: {}", error->message); - g_error_free(error); - throw std::runtime_error(e); + throw std::runtime_error(fmt::format("unable to list players: {}", error->message)); } // > get the list of players [..] in order of activity // https://github.com/altdesktop/playerctl/blob/b19a71cb9dba635df68d271bd2b3f6a99336a223/playerctl/playerctl-common.c#L248-L249 @@ -568,12 +575,16 @@ auto Mpris::getPlayerInfo() -> std::optional { errorexit: spdlog::error("mpris[{}]: {}", info.name, error->message); - g_error_free(error); return std::nullopt; } bool Mpris::handleToggle(GdkEventButton* const& e) { GError* error = nullptr; + waybar::util::scope_guard error_deleter([error]() { + if (error) { + g_error_free(error); + } + }); auto info = getPlayerInfo(); if (!info) return false; @@ -603,7 +614,6 @@ bool Mpris::handleToggle(GdkEventButton* const& e) { if (error) { spdlog::error("mpris[{}]: error running builtin on-click action: {}", (*info).name, error->message); - g_error_free(error); return false; } return true; diff --git a/src/modules/sni/host.cpp b/src/modules/sni/host.cpp index 0bbd4d2f..136c2941 100644 --- a/src/modules/sni/host.cpp +++ b/src/modules/sni/host.cpp @@ -2,6 +2,8 @@ #include +#include "util/scope_guard.hpp" + namespace waybar::modules::SNI { Host::Host(const std::size_t id, const Json::Value& config, const Bar& bar, @@ -57,17 +59,20 @@ void Host::nameVanished(const Glib::RefPtr& conn, const G void Host::proxyReady(GObject* src, GAsyncResult* res, gpointer data) { GError* error = nullptr; + waybar::util::scope_guard error_deleter([error]() { + if (error != nullptr) { + g_error_free(error); + } + }); SnWatcher* watcher = sn_watcher_proxy_new_finish(res, &error); if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { spdlog::error("Host: {}", error->message); - g_error_free(error); return; } auto host = static_cast(data); host->watcher_ = watcher; if (error != nullptr) { spdlog::error("Host: {}", error->message); - g_error_free(error); return; } sn_watcher_call_register_host(host->watcher_, host->object_path_.c_str(), host->cancellable_, @@ -76,16 +81,19 @@ void Host::proxyReady(GObject* src, GAsyncResult* res, gpointer data) { void Host::registerHost(GObject* src, GAsyncResult* res, gpointer data) { GError* error = nullptr; + waybar::util::scope_guard error_deleter([error]() { + if (error != nullptr) { + g_error_free(error); + } + }); sn_watcher_call_register_host_finish(SN_WATCHER(src), res, &error); if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { spdlog::error("Host: {}", error->message); - g_error_free(error); return; } auto host = static_cast(data); if (error != nullptr) { spdlog::error("Host: {}", error->message); - g_error_free(error); return; } g_signal_connect(host->watcher_, "item-registered", G_CALLBACK(&Host::itemRegistered), data); diff --git a/src/modules/sni/watcher.cpp b/src/modules/sni/watcher.cpp index dfd076ef..22434709 100644 --- a/src/modules/sni/watcher.cpp +++ b/src/modules/sni/watcher.cpp @@ -2,6 +2,8 @@ #include +#include "util/scope_guard.hpp" + using namespace waybar::modules::SNI; Watcher::Watcher() @@ -29,6 +31,11 @@ Watcher::~Watcher() { void Watcher::busAcquired(const Glib::RefPtr& conn, Glib::ustring name) { GError* error = nullptr; + waybar::util::scope_guard error_deleter([error]() { + if (error) { + g_error_free(error); + } + }); g_dbus_interface_skeleton_export(G_DBUS_INTERFACE_SKELETON(watcher_), conn->gobj(), "/StatusNotifierWatcher", &error); if (error != nullptr) { @@ -36,7 +43,6 @@ void Watcher::busAcquired(const Glib::RefPtr& conn, Glib: if (error->code != 2) { spdlog::error("Watcher: {}", error->message); } - g_error_free(error); return; } g_signal_connect_swapped(watcher_, "handle-register-item", diff --git a/src/modules/upower/upower.cpp b/src/modules/upower/upower.cpp index f2bc621d..e3b3981a 100644 --- a/src/modules/upower/upower.cpp +++ b/src/modules/upower/upower.cpp @@ -63,7 +63,7 @@ UPower::UPower(const std::string& id, const Json::Value& config) box_.set_has_tooltip(tooltip_enabled); if (tooltip_enabled) { // Sets the window to use when showing the tooltip - upower_tooltip = new UPowerTooltip(iconSize, tooltip_spacing, tooltip_padding); + upower_tooltip = std::make_unique(iconSize, tooltip_spacing, tooltip_padding); box_.set_tooltip_window(*upower_tooltip); box_.signal_query_tooltip().connect(sigc::mem_fun(*this, &UPower::show_tooltip_callback)); } @@ -72,14 +72,13 @@ UPower::UPower(const std::string& id, const Json::Value& config) G_BUS_NAME_WATCHER_FLAGS_AUTO_START, upowerAppear, upowerDisappear, this, NULL); - GError* error = NULL; - client = up_client_new_full(NULL, &error); + client = up_client_new_full(NULL, NULL); if (client == NULL) { throw std::runtime_error("Unable to create UPower client!"); } // Connect to Login1 PrepareForSleep signal - login1_connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, &error); + login1_connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, NULL); if (!login1_connection) { throw std::runtime_error("Unable to connect to the SYSTEM Bus!..."); } else { @@ -99,6 +98,7 @@ UPower::UPower(const std::string& id, const Json::Value& config) } UPower::~UPower() { + if (displayDevice != NULL) g_object_unref(displayDevice); if (client != NULL) g_object_unref(client); if (login1_id > 0) { g_dbus_connection_signal_unsubscribe(login1_connection, login1_id); diff --git a/src/modules/upower/upower_tooltip.cpp b/src/modules/upower/upower_tooltip.cpp index 45544bbc..1a653f85 100644 --- a/src/modules/upower/upower_tooltip.cpp +++ b/src/modules/upower/upower_tooltip.cpp @@ -9,11 +9,10 @@ namespace waybar::modules::upower { UPowerTooltip::UPowerTooltip(uint iconSize_, uint tooltipSpacing_, uint tooltipPadding_) : Gtk::Window(), + contentBox(std::make_unique(Gtk::ORIENTATION_VERTICAL)), iconSize(iconSize_), tooltipSpacing(tooltipSpacing_), tooltipPadding(tooltipPadding_) { - contentBox = new Gtk::Box(Gtk::ORIENTATION_VERTICAL); - // Sets the Tooltip Padding contentBox->set_margin_top(tooltipPadding); contentBox->set_margin_bottom(tooltipPadding); diff --git a/src/util/prepare_for_sleep.cpp b/src/util/prepare_for_sleep.cpp index 218c1e29..661285a2 100644 --- a/src/util/prepare_for_sleep.cpp +++ b/src/util/prepare_for_sleep.cpp @@ -7,8 +7,7 @@ namespace { class PrepareForSleep { private: PrepareForSleep() { - GError *error = NULL; - login1_connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, &error); + login1_connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, NULL); if (!login1_connection) { spdlog::warn("Unable to connect to the SYSTEM Bus!..."); } else {