From 66139e4440b626e0d92c4b902185ced10ce78df7 Mon Sep 17 00:00:00 2001 From: Alex Date: Sun, 5 Jul 2026 10:11:59 +0200 Subject: [PATCH] fix(tray): stop reorderItems from re-adding items (iterator UAF + double add) The item-ordering feature made Host::reorderItems() re-run the full remove/add path over items_ via std::ranges::for_each(on_remove_/on_add_). This caused two confirmed bugs: BUG 1 (iterator invalidation / UAF): on_add_ (Tray::onAdd) calls Host::checkIgnoreList, which erases from items_ while for_each is still iterating items_, invalidating iterators/pointers. Triggered by a non-empty ignore-list matching an item with >=2 items present. BUG 2 (double add): reorderItems runs while an item's Id is resolved in proxyReady, i.e. before setReady(). It added the not-yet-ready item (re-parenting its event_box, pushing into Tray::items_, connecting signal_show/hide), then setReady() -> itemReady -> onAdd added it again: GTK 'widget already has a parent' critical, duplicate Item* and signal handlers that accumulated unbounded. Fixes: - reorderItems() now only reorders already-added GTK box children via a dedicated on_reorder_ callback (Tray::reorderBox), never re-adding or removing. reorderBox stable-sorts items_ by order_ and repositions children with gtk_box_reorder_child (honouring reverse-direction). - Tray::onAdd is idempotent (guards against an already-added item) and positions the new widget via reorderBox before the ignore-list check. - signal_show/signal_hide connections are stored per item and disconnected in Tray::onRemove; onRemove is a no-op for items that were never added. --- include/modules/sni/host.hpp | 7 +++++- include/modules/sni/tray.hpp | 10 ++++++++ src/modules/sni/host.cpp | 17 ++++++++----- src/modules/sni/tray.cpp | 49 +++++++++++++++++++++++++++++++++--- 4 files changed, 72 insertions(+), 11 deletions(-) diff --git a/include/modules/sni/host.hpp b/include/modules/sni/host.hpp index 49771fac..4dad1f94 100644 --- a/include/modules/sni/host.hpp +++ b/include/modules/sni/host.hpp @@ -16,7 +16,8 @@ class Host { public: Host(std::size_t id, const Json::Value&, const Bar&, const std::vector&, const std::function&)>&, - const std::function&)>&, const std::function&); + const std::function&)>&, const std::function&, + const std::function&); ~Host(); void checkIgnoreList(const std::vector& ignore_list, @@ -55,6 +56,10 @@ class Host { const std::vector ignore_list_; const std::function&)> on_add_; const std::function&)> on_remove_; + // Re-applies the configured ordering to the already-added tray widgets. This + // must NOT re-run the add path (which would re-parent widgets and reconnect + // signals); it only reorders existing children. + const std::function on_reorder_; ItemOrderMap orders_; const std::function on_update_; diff --git a/include/modules/sni/tray.hpp b/include/modules/sni/tray.hpp index a2fd08eb..c3ac4e34 100644 --- a/include/modules/sni/tray.hpp +++ b/include/modules/sni/tray.hpp @@ -1,6 +1,10 @@ #pragma once #include +#include + +#include +#include #include "AModule.hpp" #include "bar.hpp" @@ -19,6 +23,9 @@ class Tray : public AModule { private: void onAdd(std::unique_ptr& item); void onRemove(std::unique_ptr& item); + // Reorders the already-added tray widgets by their configured order. Does not + // add or remove any widget. + void reorderBox(); void checkIgnoreList(std::unique_ptr* item); std::vector parseIgnoreList(const Json::Value& config); void queueUpdate(); @@ -29,6 +36,9 @@ class Tray : public AModule { std::vector ignore_list_; SNI::Host host_; std::vector items_; + // signal_show/signal_hide connections owned per added item, so they can be + // disconnected on removal instead of leaking and accumulating. + std::unordered_map> item_connections_; }; } // namespace waybar::modules::SNI diff --git a/src/modules/sni/host.cpp b/src/modules/sni/host.cpp index 272e05aa..c55c8f2e 100644 --- a/src/modules/sni/host.cpp +++ b/src/modules/sni/host.cpp @@ -16,7 +16,7 @@ Host::Host(std::size_t id, const Json::Value& config, const Bar& bar, const std::vector& ignore_list, const std::function&)>& on_add, const std::function&)>& on_remove, - const std::function& on_update) + const std::function& on_reorder, const std::function& on_update) : bus_name_("org.kde.StatusNotifierHost-" + std::to_string(getpid()) + "-" + std::to_string(id)), object_path_("/StatusNotifierHost/" + std::to_string(id)), @@ -27,6 +27,7 @@ Host::Host(std::size_t id, const Json::Value& config, const Bar& bar, ignore_list_(ignore_list), on_add_(on_add), on_remove_(on_remove), + on_reorder_(on_reorder), on_update_(on_update) { auto orders = config["orders"]; if (!orders.isNull()) { @@ -292,11 +293,15 @@ void Host::addRegisteredItem(const std::string& service) { } void Host::reorderItems() { - std::ranges::for_each(items_, on_remove_); - std::ranges::sort(items_, [](std::unique_ptr& item1, std::unique_ptr& item2) { - return item1->order_ < item2->order_; - }); - std::ranges::for_each(items_, on_add_); + // Re-apply the configured ordering to the tray. This is invoked while an + // item's Id/order is first resolved (from Item::setCustomIcon), which happens + // *before* the item is marked ready and added. It must therefore only reorder + // the widgets that have already been added; re-running the full add path here + // would (a) re-parent widgets and reconnect signals for every item and (b) + // mutate items_ from within checkIgnoreList while it is being iterated, + // invalidating iterators/pointers. Delegating to on_reorder_ keeps this to a + // pure reordering of existing children. + on_reorder_(); } } // namespace waybar::modules::SNI diff --git a/src/modules/sni/tray.cpp b/src/modules/sni/tray.cpp index f3e413a8..285c103f 100644 --- a/src/modules/sni/tray.cpp +++ b/src/modules/sni/tray.cpp @@ -38,7 +38,7 @@ Tray::Tray(const std::string& id, const Bar& bar, const Json::Value& config) host_((initIconsConfig(config), nb_hosts_), config, bar, ignore_list_, std::bind(&Tray::onAdd, this, std::placeholders::_1), std::bind(&Tray::onRemove, this, std::placeholders::_1), - std::bind(&Tray::queueUpdate, this)) { + std::bind(&Tray::reorderBox, this), std::bind(&Tray::queueUpdate, this)) { box_.set_name("tray"); event_box_.add(box_); if (!id.empty()) { @@ -63,6 +63,16 @@ void Tray::onAdd(std::unique_ptr& item) { spdlog::info("Tray::onAdd - item bus_name='{}', category='{}', icon_name='{}', title='{}'", item->bus_name, item->category, item->icon_name, item->title); + // Idempotency guard: onAdd can be reached more than once for the same item + // (e.g. an item is processed while its Id/order is resolved and then again + // when it becomes ready). Re-adding would re-parent the event_box (GTK + // "widget already has a parent" critical), push a duplicate pointer into + // items_ and leak extra signal connections. + if (std::find(items_.begin(), items_.end(), item.get()) != items_.end()) { + spdlog::debug("Tray::onAdd - item already added, skipping"); + return; + } + if (config_["reverse-direction"].isBool() && config_["reverse-direction"].asBool()) { box_.pack_end(item->event_box); } else { @@ -70,8 +80,13 @@ void Tray::onAdd(std::unique_ptr& item) { } items_.push_back(item.get()); - item->event_box.signal_show().connect([this] { dp.emit(); }); - item->event_box.signal_hide().connect([this] { dp.emit(); }); + auto show_conn = item->event_box.signal_show().connect([this] { dp.emit(); }); + auto hide_conn = item->event_box.signal_hide().connect([this] { dp.emit(); }); + item_connections_[item.get()] = {show_conn, hide_conn}; + + // Position the freshly added widget according to the configured order. This + // must happen before the ignore-list check below, which may erase `item`. + reorderBox(); // After this point `item` may be erased/invalidated by the ignore-list check; // do not touch it again below. @@ -82,11 +97,37 @@ void Tray::onAdd(std::unique_ptr& item) { } void Tray::onRemove(std::unique_ptr& item) { + // May be called for items that were never added (e.g. the ignore-list check + // runs over items that are not yet ready). Only touch state we actually own. + auto it = std::find(items_.begin(), items_.end(), item.get()); + if (it == items_.end()) { + return; + } + + auto conn_it = item_connections_.find(item.get()); + if (conn_it != item_connections_.end()) { + conn_it->second.first.disconnect(); + conn_it->second.second.disconnect(); + item_connections_.erase(conn_it); + } + box_.remove(item->event_box); - items_.erase(std::remove(items_.begin(), items_.end(), item.get()), items_.end()); + items_.erase(it); dp.emit(); } +void Tray::reorderBox() { + const bool reverse = + config_["reverse-direction"].isBool() && config_["reverse-direction"].asBool(); + // Stable sort keeps insertion order among items sharing the same order value. + std::stable_sort(items_.begin(), items_.end(), + [](const Item* a, const Item* b) { return a->order_ < b->order_; }); + for (std::size_t i = 0; i < items_.size(); ++i) { + const int pos = reverse ? static_cast(items_.size() - 1 - i) : static_cast(i); + box_.reorder_child(items_[i]->event_box, pos); + } +} + auto Tray::update() -> void { // Check if any items should be ignored now that properties have loaded if (!ignore_list_.empty()) {