From 2f65d415c2464716a303f5ee1a812e43e82f0aa4 Mon Sep 17 00:00:00 2001 From: Joel Winarske Date: Sun, 12 Jul 2026 13:12:01 -0700 Subject: [PATCH] Fix reliability, security, and performance issues across clients BlueZ game controllers (xbox/ps5/horipad): - Run the hidraw read loop on an owned thread joined in the destructor and interruptible via epoll+eventfd, instead of a blocking coroutine that ran inline and never returned. Fixes a use-after-free when a controller is unplugged and a hang that blocked shutdown and the D-Bus event loop. - Guard input_reader_ with a mutex; it is created on the D-Bus thread and reset on the udev thread. - Clamp the report memcpy to the bytes actually read, size the PS5 buffer to the largest report so report 0x31 is not truncated, and dump each HoriPad report array by its own size to stop a one-byte over-read. Data races: - Guard connman services_/technologies_ with a mutex across the constructor and the async callbacks. - Guard udisks2 manager_nvme_ with a mutex on add and remove. Error handling: - geoclue2: bail out when the manager returns no client object. - login1: contain sdbus errors from ListSeats/ListSessions/ListUsers in the async reply so they cannot terminate the process. - packagekit: wrap main() in try/catch. - timedate1: guard against a null localtime_r result before put_time. Performance: - Dispatch PropertiesChanged by iterating the changed properties once instead of scanning every known property with two map lookups and a per-property allocation (device1, adapter1, upower, gatt*, le_advertising_manager1). - Extract each Variant container once in the utils formatters. Resource use: - De-duplicate and cap systemd1 activeUnits_ growth. Signed-off-by: Joel Winarske --- src/bluez/adapter1.h | 90 +++----- src/bluez/device1.h | 102 ++++----- src/bluez/gatt_characteristic1.h | 43 ++-- src/bluez/gatt_descriptor1.h | 20 +- src/bluez/gatt_service1.h | 28 ++- src/bluez/horipad_steam/horipad_steam.cc | 2 + src/bluez/horipad_steam/horipad_steam.h | 4 + src/bluez/horipad_steam/input_reader.cc | 102 +++++++-- src/bluez/horipad_steam/input_reader.h | 23 +- src/bluez/le_advertising_manager1.h | 34 ++- src/bluez/ps5_dual_sense/dual_sense.cc | 2 + src/bluez/ps5_dual_sense/dual_sense.h | 4 + src/bluez/ps5_dual_sense/input_reader.cc | 107 ++++++++-- src/bluez/ps5_dual_sense/input_reader.h | 21 +- src/bluez/xbox_controller/input_reader.cc | 107 ++++++++-- src/bluez/xbox_controller/input_reader.h | 22 +- src/bluez/xbox_controller/xbox_controller.cc | 2 + src/bluez/xbox_controller/xbox_controller.h | 4 + src/connman/connman_client.cc | 14 +- src/connman/connman_client.h | 6 + src/geoclue2/main.cc | 5 + src/login1/login1_manager_client.cc | 51 +++-- src/packagekit/main.cc | 66 +++--- src/systemd1/systemd1_manager_client.cc | 13 +- src/timedate1/timedate1_client.cc | 7 +- src/udisks2/udisks2_manager.cc | 2 + src/udisks2/udisks2_manager.h | 1 + src/upower/upower_display_device.cc | 214 +++++++------------ src/utils/resource_limits.h | 1 + src/utils/utils.cc | 38 ++-- 30 files changed, 657 insertions(+), 478 deletions(-) diff --git a/src/bluez/adapter1.h b/src/bluez/adapter1.h index 5aaa761..b96f1e5 100644 --- a/src/bluez/adapter1.h +++ b/src/bluez/adapter1.h @@ -60,65 +60,43 @@ class Adapter1 final const sdbus::InterfaceName& interfaceName, const std::map& changedProperties, const std::vector& invalidatedProperties) override { - if (const auto key = sdbus::MemberName("Address"); - changedProperties.contains(key)) { - properties_.address = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("AddressType"); - changedProperties.contains(key)) { - properties_.address_type = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Alias"); - changedProperties.contains(key)) { - properties_.alias = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Class"); - changedProperties.contains(key)) { - properties_.class_type = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Discoverable"); - changedProperties.contains(key)) { - properties_.discoverable = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("DiscoverableTimeout"); - changedProperties.contains(key)) { - properties_.discoverable_timeout = - changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Discovering"); - changedProperties.contains(key)) { - properties_.discovering = changedProperties.at(key).get(); + // Iterate the changed properties once and dispatch on the key. A + // PropertiesChanged signal usually carries only a handful of properties, + // so this avoids scanning every known property (and the per-property map + // lookup + MemberName allocation) on every signal. + for (const auto& [key, value] : changedProperties) { + if (key == "Address") { + properties_.address = value.get(); + } else if (key == "AddressType") { + properties_.address_type = value.get(); + } else if (key == "Alias") { + properties_.alias = value.get(); + } else if (key == "Class") { + properties_.class_type = value.get(); + } else if (key == "Discoverable") { + properties_.discoverable = value.get(); + } else if (key == "DiscoverableTimeout") { + properties_.discoverable_timeout = value.get(); + } else if (key == "Discovering") { + properties_.discovering = value.get(); - if (!properties_.discovering) { - this->StartDiscovery(); + if (!properties_.discovering) { + this->StartDiscovery(); + } + } else if (key == "Modalias") { + properties_.modalias = value.get(); + } else if (key == "Name") { + properties_.name = value.get(); + } else if (key == "Pairable") { + properties_.pairable = value.get(); + } else if (key == "PairableTimeout") { + properties_.pairable_timeout = value.get(); + } else if (key == "Powered") { + properties_.powered = value.get(); + } else if (key == "UUIDs") { + properties_.uuids = value.get>(); } } - if (const auto key = sdbus::MemberName("Modalias"); - changedProperties.contains(key)) { - properties_.modalias = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Name"); - changedProperties.contains(key)) { - properties_.name = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Pairable"); - changedProperties.contains(key)) { - properties_.pairable = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("PairableTimeout"); - changedProperties.contains(key)) { - properties_.pairable_timeout = - changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Powered"); - changedProperties.contains(key)) { - properties_.powered = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("UUIDs"); - changedProperties.contains(key)) { - properties_.uuids = - changedProperties.at(key).get>(); - } } }; diff --git a/src/bluez/device1.h b/src/bluez/device1.h index 372cb7d..62e5782 100644 --- a/src/bluez/device1.h +++ b/src/bluez/device1.h @@ -85,70 +85,44 @@ class Device1 final : public sdbus::ProxyInterfaces& changedProperties, const std::vector& invalidatedProperties) override { - if (const auto key = sdbus::MemberName("Adapter"); - changedProperties.contains(key)) { - properties_.adapter = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Address"); - changedProperties.contains(key)) { - properties_.address = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("AddressType"); - changedProperties.contains(key)) { - properties_.address_type = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Bonded"); - changedProperties.contains(key)) { - properties_.bonded = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Blocked"); - changedProperties.contains(key)) { - properties_.blocked = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Connected"); - changedProperties.contains(key)) { - properties_.connected = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("LegacyPairing"); - changedProperties.contains(key)) { - properties_.legacy_pairing = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Paired"); - changedProperties.contains(key)) { - properties_.paired = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Modalias"); - changedProperties.contains(key)) { - properties_.modalias = - parse_modalias(changedProperties.at(key).get()); - } - if (const auto key = sdbus::MemberName("Name"); - changedProperties.contains(key)) { - properties_.name = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("ServiceData"); - changedProperties.contains(key)) { - properties_.service_data = - changedProperties.at(key) - .get>(); - } - if (const auto key = sdbus::MemberName("RSSI"); - changedProperties.contains(key)) { - properties_.rssi = changedProperties.at(key).get(); - LOG_DEBUG("RSSI: {}", properties_.rssi); - } - if (const auto key = sdbus::MemberName("ServicesResolved"); - changedProperties.contains(key)) { - properties_.services_resolved = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Trusted"); - changedProperties.contains(key)) { - properties_.trusted = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("UUIDs"); - changedProperties.contains(key)) { - properties_.uuids = - changedProperties.at(key).get>(); + // Iterate the changed properties once and dispatch on the key. A + // PropertiesChanged signal usually carries only a handful of properties, + // so this avoids scanning every known property (and the per-property map + // lookup + MemberName allocation) on every signal. + for (const auto& [key, value] : changedProperties) { + if (key == "Adapter") { + properties_.adapter = value.get(); + } else if (key == "Address") { + properties_.address = value.get(); + } else if (key == "AddressType") { + properties_.address_type = value.get(); + } else if (key == "Bonded") { + properties_.bonded = value.get(); + } else if (key == "Blocked") { + properties_.blocked = value.get(); + } else if (key == "Connected") { + properties_.connected = value.get(); + } else if (key == "LegacyPairing") { + properties_.legacy_pairing = value.get(); + } else if (key == "Paired") { + properties_.paired = value.get(); + } else if (key == "Modalias") { + properties_.modalias = parse_modalias(value.get()); + } else if (key == "Name") { + properties_.name = value.get(); + } else if (key == "ServiceData") { + properties_.service_data = + value.get>(); + } else if (key == "RSSI") { + properties_.rssi = value.get(); + LOG_DEBUG("RSSI: {}", properties_.rssi); + } else if (key == "ServicesResolved") { + properties_.services_resolved = value.get(); + } else if (key == "Trusted") { + properties_.trusted = value.get(); + } else if (key == "UUIDs") { + properties_.uuids = value.get>(); + } } #if 0 Utils::print_changed_properties(interfaceName, changedProperties, diff --git a/src/bluez/gatt_characteristic1.h b/src/bluez/gatt_characteristic1.h index 5ab4d6c..346594b 100644 --- a/src/bluez/gatt_characteristic1.h +++ b/src/bluez/gatt_characteristic1.h @@ -35,29 +35,30 @@ class GattCharacteristic1 final const sdbus::ObjectPath(&objectPath), const std::map& properties) : ProxyInterfaces{connection, destination, objectPath} { - if (const auto key = sdbus::MemberName("Flags"); properties.contains(key)) { - properties_.flags = properties.at(key).get>(); - } - if (isNotifyFlagSet()) { - if (const auto key = sdbus::MemberName("NotifyAcquired"); - properties.contains(key)) { - properties_.notify_acquired = properties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Notifying"); - properties.contains(key)) { - properties_.notifying = properties.at(key).get(); + // Iterate the provided properties once and dispatch on the key. This avoids + // scanning every known property (and the per-property map lookup + + // MemberName allocation). "Flags" sorts before "NotifyAcquired"/"Notifying" + // in the map, so it is assigned before those guarded assignments are + // reached, preserving the original isNotifyFlagSet() behavior. + for (const auto& [key, value] : properties) { + if (key == "Flags") { + properties_.flags = value.get>(); + } else if (key == "NotifyAcquired") { + if (isNotifyFlagSet()) { + properties_.notify_acquired = value.get(); + } + } else if (key == "Notifying") { + if (isNotifyFlagSet()) { + properties_.notifying = value.get(); + } + } else if (key == "Service") { + properties_.service = value.get(); + } else if (key == "UUID") { + properties_.uuid = value.get(); + } else if (key == "Value") { + properties_.value = value.get>(); } } - if (const auto key = sdbus::MemberName("Service"); - properties.contains(key)) { - properties_.service = properties.at(key).get(); - } - if (const auto key = sdbus::MemberName("UUID"); properties.contains(key)) { - properties_.uuid = properties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Value"); properties.contains(key)) { - properties_.value = properties.at(key).get>(); - } } [[nodiscard]] bool isNotifyFlagSet() const { diff --git a/src/bluez/gatt_descriptor1.h b/src/bluez/gatt_descriptor1.h index c542939..c12a8ec 100644 --- a/src/bluez/gatt_descriptor1.h +++ b/src/bluez/gatt_descriptor1.h @@ -31,15 +31,17 @@ class GattDescriptor1 final const sdbus::ObjectPath(&objectPath), const std::map& properties) : ProxyInterfaces{connection, destination, objectPath} { - if (const auto key = sdbus::MemberName("Characteristic"); - properties.contains(key)) { - properties_.characteristic = properties.at(key).get(); - } - if (const auto key = sdbus::MemberName("UUID"); properties.contains(key)) { - properties_.uuid = properties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Value"); properties.contains(key)) { - properties_.value = properties.at(key).get>(); + // Iterate the provided properties once and dispatch on the key. This avoids + // scanning every known property (and the per-property map lookup + + // MemberName allocation). + for (const auto& [key, value] : properties) { + if (key == "Characteristic") { + properties_.characteristic = value.get(); + } else if (key == "UUID") { + properties_.uuid = value.get(); + } else if (key == "Value") { + properties_.value = value.get>(); + } } } diff --git a/src/bluez/gatt_service1.h b/src/bluez/gatt_service1.h index b2aea33..d719c3a 100644 --- a/src/bluez/gatt_service1.h +++ b/src/bluez/gatt_service1.h @@ -32,21 +32,19 @@ class GattService1 const sdbus::ObjectPath(&objectPath), const std::map& properties) : ProxyInterfaces{connection, destination, objectPath} { - if (const auto key = sdbus::MemberName("Device"); - properties.contains(key)) { - properties_.device = properties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Includes"); - properties.contains(key)) { - properties_.includes = - properties.at(key).get>(); - } - if (const auto key = sdbus::MemberName("Primary"); - properties.contains(key)) { - properties_.primary = properties.at(key).get(); - } - if (const auto key = sdbus::MemberName("UUID"); properties.contains(key)) { - properties_.uuid = properties.at(key).get(); + // Iterate the provided properties once and dispatch on the key. This avoids + // scanning every known property (and the per-property map lookup + + // MemberName allocation). + for (const auto& [key, value] : properties) { + if (key == "Device") { + properties_.device = value.get(); + } else if (key == "Includes") { + properties_.includes = value.get>(); + } else if (key == "Primary") { + properties_.primary = value.get(); + } else if (key == "UUID") { + properties_.uuid = value.get(); + } } } diff --git a/src/bluez/horipad_steam/horipad_steam.cc b/src/bluez/horipad_steam/horipad_steam.cc index 72bd784..17b2bd8 100644 --- a/src/bluez/horipad_steam/horipad_steam.cc +++ b/src/bluez/horipad_steam/horipad_steam.cc @@ -45,6 +45,7 @@ HoripadSteam::HoripadSteam(sdbus::IConnection& connection) sub_system ? sub_system : ""); if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { + std::scoped_lock reader_lock(input_reader_mutex_); if (input_reader_) { input_reader_->stop(); input_reader_.reset(); @@ -155,6 +156,7 @@ void HoripadSteam::onInterfacesAdded( if (const std::string hidraw_device = FindHidDevice(hidraw_device_key); !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); + std::scoped_lock reader_lock(input_reader_mutex_); if (!input_reader_) { input_reader_ = std::make_unique(hidraw_device); input_reader_->start(); diff --git a/src/bluez/horipad_steam/horipad_steam.h b/src/bluez/horipad_steam/horipad_steam.h index cb1854d..7ad88d0 100644 --- a/src/bluez/horipad_steam/horipad_steam.h +++ b/src/bluez/horipad_steam/horipad_steam.h @@ -57,6 +57,10 @@ class HoripadSteam final std::mutex input1_mutex_; std::map> input1_; + // Guards input_reader_, which is created/started from onInterfacesAdded + // (D-Bus event-loop or main thread) and stopped/reset from the udev monitor + // worker thread. + std::mutex input_reader_mutex_; std::unique_ptr input_reader_; void onInterfacesAdded( diff --git a/src/bluez/horipad_steam/input_reader.cc b/src/bluez/horipad_steam/input_reader.cc index fcf34e9..8420bf2 100644 --- a/src/bluez/horipad_steam/input_reader.cc +++ b/src/bluez/horipad_steam/input_reader.cc @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include @@ -19,6 +20,7 @@ #include #include +#include #include #include "../../utils/logging.h" @@ -26,25 +28,43 @@ #include "input_reader.h" InputReader::InputReader(std::string device) - : device_(std::move(device)), stop_flag_(false) {} + : device_(std::move(device)), + stop_flag_(false), + stop_event_fd_(::eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK)) { + if (!stop_event_fd_.valid()) { + LOG_ERROR("Failed to create eventfd: {}", strerror(errno)); + } +} void InputReader::start() { LOG_DEBUG("InputReader start: {}", device_); + if (thread_.joinable()) { + return; // already running + } stop_flag_ = false; - read_input(); + thread_ = std::thread([this] { read_input(); }); } void InputReader::stop() { LOG_DEBUG("InputReader stop: {}", device_); stop_flag_ = true; + // Wake the blocking epoll_wait so the loop observes stop_flag_ immediately. + if (stop_event_fd_.valid()) { + constexpr std::uint64_t one = 1; + if (::write(stop_event_fd_.get(), &one, sizeof(one)) < 0) { + LOG_ERROR("Failed to signal stop eventfd: {}", strerror(errno)); + } + } } InputReader::~InputReader() { stop(); + if (thread_.joinable()) { + thread_.join(); + } } -// NOLINTNEXTLINE(readability-static-accessed-through-instance) -InputReader::Task InputReader::read_input() { +void InputReader::read_input() { LOG_DEBUG("hidraw device: {}", device_); const UniqueFd fd(open(device_.c_str(), O_RDWR)); @@ -114,13 +134,70 @@ InputReader::Task InputReader::read_input() { os << CustomHexdump<400, false>(std::data(rpt_desc.value), rpt_desc.size); LOG_INFO(os.str()); + // Wait on both the hidraw fd and the stop eventfd so a blocking read can + // be interrupted immediately when stop() is called from another thread. + const UniqueFd epoll_fd(epoll_create1(EPOLL_CLOEXEC)); + if (!epoll_fd.valid()) { + LOG_ERROR("epoll_create1 failed: {}", strerror(errno)); + break; + } + epoll_event ev{}; + ev.events = EPOLLIN; + ev.data.fd = fd.get(); + if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, fd.get(), &ev) == -1) { + LOG_ERROR("epoll_ctl(hidraw) failed: {}", strerror(errno)); + break; + } + if (stop_event_fd_.valid()) { + ev.data.fd = stop_event_fd_.get(); + if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, stop_event_fd_.get(), &ev) == + -1) { + LOG_ERROR("epoll_ctl(stop) failed: {}", strerror(errno)); + break; + } + } + while (!stop_flag_) { + std::array poll_events{}; + const int nfds = epoll_wait(epoll_fd.get(), poll_events.data(), + poll_events.size(), -1); + if (nfds == -1) { + if (errno == EINTR) { + continue; + } + LOG_ERROR("epoll_wait failed: {}", strerror(errno)); + break; + } + + bool stop_requested = false; + bool data_ready = false; + for (int i = 0; i < nfds; ++i) { + if (stop_event_fd_.valid() && + poll_events.at(i).data.fd == stop_event_fd_.get()) { + stop_requested = true; + } else if (poll_events.at(i).data.fd == fd.get()) { + data_ready = true; + } + } + if (stop_requested) { + break; + } + if (!data_ready) { + continue; + } + std::array buffer{}; - ssize_t result = 0; - if (result = read(fd.get(), buffer.data(), buffer.size()); result < 0) { + const ssize_t result = read(fd.get(), buffer.data(), buffer.size()); + if (result < 0) { + if (errno == EINTR || errno == EAGAIN) { + continue; + } LOG_ERROR("read failed: {}", strerror(errno)); break; } + if (result == 0) { + continue; + } if (raw_dev_info.product == 0x01ab || raw_dev_info.product == 0x0196) { if (const auto report_id = buffer.at(0); report_id == 7) { @@ -151,10 +228,7 @@ InputReader::Task InputReader::read_input() { break; } - // fd is automatically closed by UniqueFd destructor - stop(); - - co_return; // NOLINT(readability-static-accessed-through-instance) + // fd is automatically closed by UniqueFd destructor. } std::string InputReader::dpad_to_string(const Direction dpad) { @@ -213,20 +287,20 @@ void InputReader::PrintInputReport7(const inputReport07_t& input_report07) { void InputReader::PrintInputReport10(const inputReport10_t& input_report10) { std::ostringstream os; os << CustomHexdump<400, false>(std::data(input_report10.VEN_Gamepad0024), - sizeof(inputReport10_t)); + sizeof(input_report10.VEN_Gamepad0024)); LOG_INFO("Input Report 10: {}", os.str()); } void InputReader::PrintInputReport12(const inputReport12_t& input_report12) { std::ostringstream os; os << CustomHexdump<400, false>(std::data(input_report12.VEN_Gamepad0022), - sizeof(inputReport10_t)); - LOG_INFO("Input Report 10: {}", os.str()); + sizeof(input_report12.VEN_Gamepad0022)); + LOG_INFO("Input Report 12: {}", os.str()); } void InputReader::PrintInputReport14(const inputReport14_t& input_report14) { std::ostringstream os; os << CustomHexdump<400, false>(std::data(input_report14.VEN_Gamepad0026), - sizeof(inputReport10_t)); + sizeof(input_report14.VEN_Gamepad0026)); LOG_INFO("Input Report 14: {}", os.str()); } diff --git a/src/bluez/horipad_steam/input_reader.h b/src/bluez/horipad_steam/input_reader.h index 9ce18c2..800fa14 100644 --- a/src/bluez/horipad_steam/input_reader.h +++ b/src/bluez/horipad_steam/input_reader.h @@ -16,8 +16,9 @@ #define SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ #include -#include +#include +#include "../hidraw.hpp" #include "horipad_stream_01ab_0196.h" class InputReader { @@ -43,20 +44,16 @@ class InputReader { ~InputReader(); private: - struct Task { - struct promise_type { - static Task get_return_object() { return {}; } - static std::suspend_never initial_suspend() { return {}; } - static std::suspend_never final_suspend() noexcept { return {}; } - static void return_void() {} - static void unhandled_exception() { std::terminate(); } - }; - }; - std::string device_; std::atomic stop_flag_; - - Task read_input(); + // eventfd used to interrupt the blocking read loop immediately on stop(). + UniqueFd stop_event_fd_; + // Worker thread that owns the blocking read loop. Joined in the destructor + // before any other member is torn down, so the loop can never outlive this + // object (no use-after-free) and never blocks the D-Bus/main thread. + std::thread thread_; + + void read_input(); static std::string dpad_to_string(Direction dpad); diff --git a/src/bluez/le_advertising_manager1.h b/src/bluez/le_advertising_manager1.h index c165012..ef47fc3 100644 --- a/src/bluez/le_advertising_manager1.h +++ b/src/bluez/le_advertising_manager1.h @@ -49,25 +49,21 @@ class LEAdvertisingManager1 final const sdbus::InterfaceName& interfaceName, const std::map& changedProperties, const std::vector& invalidatedProperties) override { - if (const auto key = sdbus::MemberName("ActiveInstances"); - changedProperties.contains(key)) { - properties_.active_instances = - changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("SupportedIncludes"); - changedProperties.contains(key)) { - properties_.supported_includes = - changedProperties.at(key).get>(); - } - if (const auto key = sdbus::MemberName("SupportedInstances"); - changedProperties.contains(key)) { - properties_.supported_instances = - changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("SupportedSecondaryChannels"); - changedProperties.contains(key)) { - properties_.supported_secondary_channels = - changedProperties.at(key).get>(); + // Iterate the changed properties once and dispatch on the key. A + // PropertiesChanged signal usually carries only a handful of properties, + // so this avoids scanning every known property (and the per-property map + // lookup + MemberName allocation) on every signal. + for (const auto& [key, value] : changedProperties) { + if (key == "ActiveInstances") { + properties_.active_instances = value.get(); + } else if (key == "SupportedIncludes") { + properties_.supported_includes = value.get>(); + } else if (key == "SupportedInstances") { + properties_.supported_instances = value.get(); + } else if (key == "SupportedSecondaryChannels") { + properties_.supported_secondary_channels = + value.get>(); + } } } }; diff --git a/src/bluez/ps5_dual_sense/dual_sense.cc b/src/bluez/ps5_dual_sense/dual_sense.cc index f939533..eaa7ad1 100644 --- a/src/bluez/ps5_dual_sense/dual_sense.cc +++ b/src/bluez/ps5_dual_sense/dual_sense.cc @@ -44,6 +44,7 @@ DualSense::DualSense(sdbus::IConnection& connection) sub_system ? sub_system : ""); if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { + std::scoped_lock reader_lock(input_reader_mutex_); if (input_reader_) { input_reader_->stop(); input_reader_.reset(); @@ -157,6 +158,7 @@ void DualSense::onInterfacesAdded( if (const std::string hidraw_device = FindHidDevice(hidraw_device_key); !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); + std::scoped_lock reader_lock(input_reader_mutex_); if (!input_reader_) { input_reader_ = std::make_unique(hidraw_device); input_reader_->start(); diff --git a/src/bluez/ps5_dual_sense/dual_sense.h b/src/bluez/ps5_dual_sense/dual_sense.h index b06d75f..3034273 100644 --- a/src/bluez/ps5_dual_sense/dual_sense.h +++ b/src/bluez/ps5_dual_sense/dual_sense.h @@ -62,6 +62,10 @@ class DualSense final std::mutex upower_display_devices_mutex_; std::map> upower_clients_; + // Guards input_reader_, which is created/started from onInterfacesAdded + // (D-Bus event-loop or main thread) and stopped/reset from the udev monitor + // worker thread. + std::mutex input_reader_mutex_; std::unique_ptr input_reader_; void onInterfacesAdded( diff --git a/src/bluez/ps5_dual_sense/input_reader.cc b/src/bluez/ps5_dual_sense/input_reader.cc index 393d8d8..d4b8a17 100644 --- a/src/bluez/ps5_dual_sense/input_reader.cc +++ b/src/bluez/ps5_dual_sense/input_reader.cc @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include @@ -20,6 +21,7 @@ #include #include +#include #include #include "../../utils/logging.h" @@ -27,25 +29,43 @@ #include "input_reader.h" InputReader::InputReader(std::string device) - : device_(std::move(device)), stop_flag_(false) {} + : device_(std::move(device)), + stop_flag_(false), + stop_event_fd_(::eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK)) { + if (!stop_event_fd_.valid()) { + LOG_ERROR("Failed to create eventfd: {}", strerror(errno)); + } +} void InputReader::start() { LOG_DEBUG("InputReader start: {}", device_); + if (thread_.joinable()) { + return; // already running + } stop_flag_ = false; - read_input(); + thread_ = std::thread([this] { read_input(); }); } void InputReader::stop() { LOG_DEBUG("InputReader stop: {}", device_); stop_flag_ = true; + // Wake the blocking epoll_wait so the loop observes stop_flag_ immediately. + if (stop_event_fd_.valid()) { + constexpr std::uint64_t one = 1; + if (::write(stop_event_fd_.get(), &one, sizeof(one)) < 0) { + LOG_ERROR("Failed to signal stop eventfd: {}", strerror(errno)); + } + } } InputReader::~InputReader() { stop(); + if (thread_.joinable()) { + thread_.join(); + } } -// NOLINTNEXTLINE(readability-static-accessed-through-instance) -InputReader::Task InputReader::read_input() { +void InputReader::read_input() { LOG_DEBUG("hidraw device: {}", device_); const UniqueFd fd(open(device_.c_str(), O_RDWR)); @@ -121,24 +141,86 @@ InputReader::Task InputReader::read_input() { GetControllerMacAll(fd.get(), controller_and_host_mac_); GetControllerVersion(fd.get(), version_); + // Wait on both the hidraw fd and the stop eventfd so a blocking read can + // be interrupted immediately when stop() is called from another thread. + const UniqueFd epoll_fd(epoll_create1(EPOLL_CLOEXEC)); + if (!epoll_fd.valid()) { + LOG_ERROR("epoll_create1 failed: {}", strerror(errno)); + break; + } + epoll_event ev{}; + ev.events = EPOLLIN; + ev.data.fd = fd.get(); + if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, fd.get(), &ev) == -1) { + LOG_ERROR("epoll_ctl(hidraw) failed: {}", strerror(errno)); + break; + } + if (stop_event_fd_.valid()) { + ev.data.fd = stop_event_fd_.get(); + if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, stop_event_fd_.get(), &ev) == + -1) { + LOG_ERROR("epoll_ctl(stop) failed: {}", strerror(errno)); + break; + } + } + while (!stop_flag_) { - std::array buffer{}; - ssize_t result = 0; - if (result = read(fd.get(), buffer.data(), buffer.size()); result < 0) { - LOG_ERROR("GetInputReport4 failed: {}", strerror(errno)); + std::array poll_events{}; + const int nfds = epoll_wait(epoll_fd.get(), poll_events.data(), + poll_events.size(), -1); + if (nfds == -1) { + if (errno == EINTR) { + continue; + } + LOG_ERROR("epoll_wait failed: {}", strerror(errno)); + break; + } + + bool stop_requested = false; + bool data_ready = false; + for (int i = 0; i < nfds; ++i) { + if (stop_event_fd_.valid() && + poll_events.at(i).data.fd == stop_event_fd_.get()) { + stop_requested = true; + } else if (poll_events.at(i).data.fd == fd.get()) { + data_ready = true; + } + } + if (stop_requested) { + break; + } + if (!data_ready) { + continue; + } + + // Buffer sized to the largest report (ReportIn31) so no report is + // truncated on read. + std::array + buffer{}; + const ssize_t result = read(fd.get(), buffer.data(), buffer.size()); + if (result < 0) { + if (errno == EINTR || errno == EAGAIN) { + continue; + } + LOG_ERROR("read failed: {}", strerror(errno)); break; } + if (result == 0) { + continue; + } + const auto bytes_read = static_cast(result); if (raw_dev_info.product == 0x0CE6) { if (const auto report_id = buffer.at(0); report_id == 1) { USBGetStateData input_report01{}; std::memcpy(&input_report01, buffer.data(), - std::min(sizeof(USBGetStateData), buffer.size())); + std::min(sizeof(USBGetStateData), bytes_read)); PrintControllerStateUsb(input_report01, hw_cal_data_); } else if (report_id == 49) { ReportIn31 input_report31{}; std::memcpy(&input_report31, buffer.data(), - std::min(sizeof(ReportIn31), buffer.size())); + std::min(sizeof(ReportIn31), bytes_read)); if (input_report31.Data.HasHID) { LOG_INFO("[ReportIn31] Has HID"); PrintControllerStateUsb(input_report31.Data.State.StateData, @@ -154,10 +236,7 @@ InputReader::Task InputReader::read_input() { break; } - // fd is automatically closed by UniqueFd destructor - stop(); - - co_return; // NOLINT(readability-static-accessed-through-instance) + // fd is automatically closed by UniqueFd destructor. } int InputReader::GetControllerMacAll(const int fd, diff --git a/src/bluez/ps5_dual_sense/input_reader.h b/src/bluez/ps5_dual_sense/input_reader.h index 65a52ab..55b7f94 100644 --- a/src/bluez/ps5_dual_sense/input_reader.h +++ b/src/bluez/ps5_dual_sense/input_reader.h @@ -17,7 +17,7 @@ #include #include -#include +#include #include "../hidraw.hpp" #include "dual_sense_0ce6.h" @@ -33,16 +33,6 @@ class InputReader { ~InputReader(); private: - struct Task { - struct promise_type { - static Task get_return_object() { return {}; } - static std::suspend_never initial_suspend() { return {}; } - static std::suspend_never final_suspend() noexcept { return {}; } - static void return_void() {} - static void unhandled_exception() { std::terminate(); } - }; - }; - struct CalibrationData { std::int32_t abs_code; std::int16_t bias; @@ -57,12 +47,19 @@ class InputReader { std::string device_; std::atomic stop_flag_; + // eventfd used to interrupt the blocking read loop immediately on stop(). + UniqueFd stop_event_fd_; ReportFeatureInMacAll controller_and_host_mac_{}; ReportFeatureInVersion version_{}; HardwareCalibrationData hw_cal_data_{}; - Task read_input(); + // Worker thread that owns the blocking read loop. Joined in the destructor + // before any other member is torn down, so the loop can never outlive this + // object (no use-after-free) and never blocks the D-Bus/main thread. + std::thread thread_; + + void read_input(); static std::string dpad_to_string(Direction dpad); static std::string power_state_to_string(PowerState state); diff --git a/src/bluez/xbox_controller/input_reader.cc b/src/bluez/xbox_controller/input_reader.cc index ddc30b0..4b6c19c 100644 --- a/src/bluez/xbox_controller/input_reader.cc +++ b/src/bluez/xbox_controller/input_reader.cc @@ -12,14 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include -#include #include #include #include #include +#include #include #include "../../utils/logging.h" @@ -27,25 +28,43 @@ #include "input_reader.h" InputReader::InputReader(std::string device) - : device_(std::move(device)), stop_flag_(false) {} + : device_(std::move(device)), + stop_flag_(false), + stop_event_fd_(::eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK)) { + if (!stop_event_fd_.valid()) { + LOG_ERROR("Failed to create eventfd: {}", strerror(errno)); + } +} void InputReader::start() { LOG_DEBUG("InputReader start: {}", device_); + if (thread_.joinable()) { + return; // already running + } stop_flag_ = false; - read_input(); + thread_ = std::thread([this] { read_input(); }); } void InputReader::stop() { LOG_DEBUG("InputReader stop: {}", device_); stop_flag_ = true; + // Wake the blocking epoll_wait so the loop observes stop_flag_ immediately. + if (stop_event_fd_.valid()) { + constexpr std::uint64_t one = 1; + if (::write(stop_event_fd_.get(), &one, sizeof(one)) < 0) { + LOG_ERROR("Failed to signal stop eventfd: {}", strerror(errno)); + } + } } InputReader::~InputReader() { stop(); + if (thread_.joinable()) { + thread_.join(); + } } -// NOLINTNEXTLINE(readability-static-accessed-through-instance) -InputReader::Task InputReader::read_input() { +void InputReader::read_input() { LOG_DEBUG("hidraw device: {}", device_); const UniqueFd fd(open(device_.c_str(), O_RDWR)); @@ -115,26 +134,87 @@ InputReader::Task InputReader::read_input() { os << CustomHexdump<400, false>(std::data(rpt_desc.value), rpt_desc.size); LOG_INFO(os.str()); + // Wait on both the hidraw fd and the stop eventfd so a blocking read can + // be interrupted immediately when stop() is called from another thread. + const UniqueFd epoll_fd(epoll_create1(EPOLL_CLOEXEC)); + if (!epoll_fd.valid()) { + LOG_ERROR("epoll_create1 failed: {}", strerror(errno)); + break; + } + epoll_event ev{}; + ev.events = EPOLLIN; + ev.data.fd = fd.get(); + if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, fd.get(), &ev) == -1) { + LOG_ERROR("epoll_ctl(hidraw) failed: {}", strerror(errno)); + break; + } + if (stop_event_fd_.valid()) { + ev.data.fd = stop_event_fd_.get(); + if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, stop_event_fd_.get(), &ev) == + -1) { + LOG_ERROR("epoll_ctl(stop) failed: {}", strerror(errno)); + break; + } + } + while (!stop_flag_) { + std::array events{}; + const int nfds = + epoll_wait(epoll_fd.get(), events.data(), events.size(), -1); + if (nfds == -1) { + if (errno == EINTR) { + continue; + } + LOG_ERROR("epoll_wait failed: {}", strerror(errno)); + break; + } + + bool stop_requested = false; + bool data_ready = false; + for (int i = 0; i < nfds; ++i) { + if (stop_event_fd_.valid() && + events.at(i).data.fd == stop_event_fd_.get()) { + stop_requested = true; + } else if (events.at(i).data.fd == fd.get()) { + data_ready = true; + } + } + if (stop_requested) { + break; + } + if (!data_ready) { + continue; + } + std::array buffer{}; - ssize_t result = 0; - if (result = read(fd.get(), buffer.data(), buffer.size()); result < 0) { - LOG_ERROR("GetInputReport4 failed: {}", strerror(errno)); + const ssize_t result = read(fd.get(), buffer.data(), buffer.size()); + if (result < 0) { + if (errno == EINTR || errno == EAGAIN) { + continue; + } + LOG_ERROR("read failed: {}", strerror(errno)); break; } + if (result == 0) { + continue; + } + const auto bytes_read = static_cast(result); if (raw_dev_info.product == 0x02FD) { if (const auto report_id = buffer.at(0); report_id == 1) { inputReport01_t input_report01{}; - std::memcpy(&input_report01, buffer.data(), sizeof(inputReport01_t)); + std::memcpy(&input_report01, buffer.data(), + std::min(sizeof(inputReport01_t), bytes_read)); PrintInputReport1(input_report01); } else if (report_id == 2) { inputReport02_t input_report02{}; - std::memcpy(&input_report02, buffer.data(), sizeof(inputReport02_t)); + std::memcpy(&input_report02, buffer.data(), + std::min(sizeof(inputReport02_t), bytes_read)); PrintInputReport2(input_report02); } else if (report_id == 4) { inputReport04_t input_report04{}; - std::memcpy(&input_report04, buffer.data(), sizeof(inputReport04_t)); + std::memcpy(&input_report04, buffer.data(), + std::min(sizeof(inputReport04_t), bytes_read)); PrintInputReport4(input_report04); } else { LOG_ERROR("Unknown report id: {}", report_id); @@ -144,10 +224,7 @@ InputReader::Task InputReader::read_input() { break; } - // fd is automatically closed by UniqueFd destructor - stop(); - - co_return; // NOLINT(readability-static-accessed-through-instance) + // fd is automatically closed by UniqueFd destructor. } std::string InputReader::dpad_to_string(const Direction dpad) { diff --git a/src/bluez/xbox_controller/input_reader.h b/src/bluez/xbox_controller/input_reader.h index 6ef06be..f519525 100644 --- a/src/bluez/xbox_controller/input_reader.h +++ b/src/bluez/xbox_controller/input_reader.h @@ -16,7 +16,7 @@ #define SRC_BLUEZ_XBOX_CONTROLLER_INPUT_READER_HPP_ #include -#include +#include #include "../hidraw.hpp" #include "xbox_controller_02fd.h" @@ -44,20 +44,16 @@ class InputReader { ~InputReader(); private: - struct Task { - struct promise_type { - static Task get_return_object() { return {}; } - static std::suspend_never initial_suspend() { return {}; } - static std::suspend_never final_suspend() noexcept { return {}; } - static void return_void() {} - static void unhandled_exception() { std::terminate(); } - }; - }; - std::string device_; std::atomic stop_flag_; - - Task read_input(); + // eventfd used to interrupt the blocking read loop immediately on stop(). + UniqueFd stop_event_fd_; + // Worker thread that owns the blocking read loop. Joined in the destructor + // before any other member is torn down, so the loop can never outlive this + // object (no use-after-free) and never blocks the D-Bus/main thread. + std::thread thread_; + + void read_input(); static std::string dpad_to_string(Direction dpad); diff --git a/src/bluez/xbox_controller/xbox_controller.cc b/src/bluez/xbox_controller/xbox_controller.cc index 155586f..827251c 100644 --- a/src/bluez/xbox_controller/xbox_controller.cc +++ b/src/bluez/xbox_controller/xbox_controller.cc @@ -46,6 +46,7 @@ XboxController::XboxController(sdbus::IConnection& connection) sub_system ? sub_system : ""); if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { + std::scoped_lock reader_lock(input_reader_mutex_); if (input_reader_) { input_reader_->stop(); input_reader_.reset(); @@ -157,6 +158,7 @@ void XboxController::onInterfacesAdded( if (const std::string hidraw_device = FindHidDevice(hidraw_device_key); !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); + std::scoped_lock reader_lock(input_reader_mutex_); if (!input_reader_) { input_reader_ = std::make_unique(hidraw_device); input_reader_->start(); diff --git a/src/bluez/xbox_controller/xbox_controller.h b/src/bluez/xbox_controller/xbox_controller.h index 8159b25..0483e09 100644 --- a/src/bluez/xbox_controller/xbox_controller.h +++ b/src/bluez/xbox_controller/xbox_controller.h @@ -62,6 +62,10 @@ class XboxController final std::mutex upower_display_devices_mutex_; std::map> upower_clients_; + // Guards input_reader_, which is created/started from onInterfacesAdded + // (D-Bus event-loop or main thread) and stopped/reset from the udev monitor + // worker thread. + std::mutex input_reader_mutex_; std::unique_ptr input_reader_; void onInterfacesAdded( diff --git a/src/connman/connman_client.cc b/src/connman/connman_client.cc index f8830f6..0d4a07c 100644 --- a/src/connman/connman_client.cc +++ b/src/connman/connman_client.cc @@ -30,9 +30,16 @@ ConnmanManagerClient::ConnmanManagerClient(sdbus::IConnection& connection) registerProxy(); try { - // Load initial technologies + std::scoped_lock lock(maps_mutex_); + // Load initial technologies. Insert inline (rather than via + // onTechnologyAdded) so we don't re-acquire the non-recursive maps_mutex_. for (const auto& tech : GetTechnologies()) { - onTechnologyAdded(tech.get<0>(), tech.get<1>()); + const auto& path = tech.get<0>(); + LOG_INFO("Technology added: {}", path); + if (technologies_.find(path) == technologies_.end()) { + technologies_[path] = std::make_unique( + getProxy().getConnection(), path); + } } // Load initial services @@ -69,6 +76,7 @@ void ConnmanManagerClient::onTechnologyAdded( const std::map& properties) { (void)properties; try { + std::scoped_lock lock(maps_mutex_); LOG_INFO("Technology added: {}", path); if (technologies_.find(path) == technologies_.end()) { technologies_[path] = std::make_unique( @@ -81,6 +89,7 @@ void ConnmanManagerClient::onTechnologyAdded( void ConnmanManagerClient::onTechnologyRemoved(const sdbus::ObjectPath& path) { try { + std::scoped_lock lock(maps_mutex_); LOG_INFO("Technology removed: {}", path); technologies_.erase(path); } catch (const std::exception& e) { @@ -94,6 +103,7 @@ void ConnmanManagerClient::onServicesChanged( changed, const std::vector& removed) { try { + std::scoped_lock lock(maps_mutex_); for (const auto& service : changed) { const auto& path = service.get<0>(); const auto& props = service.get<1>(); diff --git a/src/connman/connman_client.h b/src/connman/connman_client.h index 2b39d5f..be1d09d 100644 --- a/src/connman/connman_client.h +++ b/src/connman/connman_client.h @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -62,6 +63,11 @@ class ConnmanManagerClient final const std::vector& removed) override; private: + // registerProxy() enables signal delivery on the async event-loop thread + // while the constructor still populates the maps on the calling thread, and + // onTechnology*/onServicesChanged mutate them from the event-loop thread. + // Guard all access with this mutex. + std::mutex maps_mutex_; std::map> technologies_; std::map> services_; diff --git a/src/geoclue2/main.cc b/src/geoclue2/main.cc index 308d7b4..4db855b 100644 --- a/src/geoclue2/main.cc +++ b/src/geoclue2/main.cc @@ -38,6 +38,11 @@ int main() { }); const auto& client = manager.Client(); + if (!client) { + LOG_ERROR("GeoClue2 did not return a client object; cannot continue"); + connection->leaveEventLoop(); + return 1; + } // `desktop id` must be set for Start to work client->DesktopId("org.example.geoclue2"); diff --git a/src/login1/login1_manager_client.cc b/src/login1/login1_manager_client.cc index ae0b84e..0829430 100644 --- a/src/login1/login1_manager_client.cc +++ b/src/login1/login1_manager_client.cc @@ -27,27 +27,36 @@ Login1ManagerClient::Login1ManagerClient(sdbus::IConnection& connection) if (!error) { onPropertiesChanged( sdbus::InterfaceName(Manager_proxy::INTERFACE_NAME), values, {}); - for (const auto seats = this->ListSeats(); - const auto& value : seats) { - // [('seat0', '/org/freedesktop/login1/seat/seat0')] - const auto& name = value.get<0>(); - const auto& path = value.get<1>(); - onSeatNew(name, path); - } - for (const auto sessions = this->ListSessions(); - const auto& value : sessions) { - // [('2', 1000, 'joel', 'seat0', - // '/org/freedesktop/login1/session/_32')] - const auto& user_name = value.get<2>(); - const auto& path = value.get<4>(); - onSessionNew(user_name, path); - } - for (const auto users = this->ListUsers(); - const auto& value : users) { - // [(1000, 'joel', '/org/freedesktop/login1/user/_1000')] - auto id = value.get<0>(); - const auto& path = value.get<2>(); - onUserNew(id, path); + // ListSeats/ListSessions/ListUsers are synchronous D-Bus calls; a + // throw here would escape the async reply handler into the event + // loop and terminate the process, so contain it. + try { + for (const auto seats = this->ListSeats(); + const auto& value : seats) { + // [('seat0', '/org/freedesktop/login1/seat/seat0')] + const auto& name = value.get<0>(); + const auto& path = value.get<1>(); + onSeatNew(name, path); + } + for (const auto sessions = this->ListSessions(); + const auto& value : sessions) { + // [('2', 1000, 'joel', 'seat0', + // '/org/freedesktop/login1/session/_32')] + const auto& user_name = value.get<2>(); + const auto& path = value.get<4>(); + onSessionNew(user_name, path); + } + for (const auto users = this->ListUsers(); + const auto& value : users) { + // [(1000, 'joel', '/org/freedesktop/login1/user/_1000')] + auto id = value.get<0>(); + const auto& path = value.get<2>(); + onUserNew(id, path); + } + } catch (const sdbus::Error& e) { + LOG_ERROR("[{}] enumerate failed: {} - {}", + Manager_proxy::INTERFACE_NAME, e.getName(), + e.getMessage()); } } else LOG_ERROR("[{}] {} - {}", Manager_proxy::INTERFACE_NAME, diff --git a/src/packagekit/main.cc b/src/packagekit/main.cc index e50354d..8153cf8 100644 --- a/src/packagekit/main.cc +++ b/src/packagekit/main.cc @@ -18,33 +18,43 @@ #include "../utils/utils.h" int main() { - const auto connection = sdbus::createSystemBusConnection(); - connection->enterEventLoopAsync(); - - { - PackageKitClient client(*connection); - std::promise> promise; - auto future = promise.get_future(); - - client.GetAllAsync( - PackageKitClient::INTERFACE_NAME, - [&](std::optional error, - std::map values) { - if (!error) { - promise.set_value(std::move(values)); - } else { - promise.set_exception(std::make_exception_ptr(std::move(*error))); - } - }); - - const auto properties = future.get(); - Utils::print_changed_properties( - sdbus::InterfaceName(PackageKitClient::INTERFACE_NAME), properties, {}); - auto state = client.GetDaemonState(); - LOG_INFO("Daemon {}", state); + try { + const auto connection = sdbus::createSystemBusConnection(); + connection->enterEventLoopAsync(); + + { + PackageKitClient client(*connection); + std::promise> promise; + auto future = promise.get_future(); + + client.GetAllAsync( + PackageKitClient::INTERFACE_NAME, + [&](std::optional error, + std::map values) { + if (!error) { + promise.set_value(std::move(values)); + } else { + promise.set_exception(std::make_exception_ptr(std::move(*error))); + } + }); + + const auto properties = future.get(); + Utils::print_changed_properties( + sdbus::InterfaceName(PackageKitClient::INTERFACE_NAME), properties, + {}); + auto state = client.GetDaemonState(); + LOG_INFO("Daemon {}", state); + } + + connection->leaveEventLoop(); + + return 0; + + } catch (const sdbus::Error& e) { + LOG_ERROR("D-Bus error: {} - {}", e.getName(), e.getMessage()); + return 1; + } catch (const std::exception& e) { + LOG_ERROR("Exception: {}", e.what()); + return 1; } - - connection->leaveEventLoop(); - - return 0; } diff --git a/src/systemd1/systemd1_manager_client.cc b/src/systemd1/systemd1_manager_client.cc index 3e93b3d..e5668ee 100644 --- a/src/systemd1/systemd1_manager_client.cc +++ b/src/systemd1/systemd1_manager_client.cc @@ -7,6 +7,7 @@ #include #include "../utils/logging.h" +#include "../utils/resource_limits.h" Systemd1ManagerClient::Systemd1ManagerClient(sdbus::IConnection& connection) : ProxyInterfaces{connection, sdbus::ServiceName(SERVICE_NAME), @@ -43,7 +44,17 @@ void Systemd1ManagerClient::onPropertiesChanged( void Systemd1ManagerClient::onUnitNew(const std::string& id, const sdbus::ObjectPath& unit) { - activeUnits_.push_back(id); + // systemd emits UnitNew for thousands of units and re-emits them over a long + // run; de-duplicate and cap growth so activeUnits_ cannot grow unbounded. + if (std::ranges::find(activeUnits_, id) == activeUnits_.end()) { + if (resource_limits::IsAtCapacity(activeUnits_.size(), + resource_limits::kMaxUnits)) { + LOG_WARN("[systemd1] Skipping UnitNew id={}: resource limit reached ({})", + id, resource_limits::kMaxUnits); + } else { + activeUnits_.push_back(id); + } + } LOG_INFO("[systemd1] UnitNew id={} path={}", id, static_cast(unit)); } diff --git a/src/timedate1/timedate1_client.cc b/src/timedate1/timedate1_client.cc index cc8cacd..069c6bc 100644 --- a/src/timedate1/timedate1_client.cc +++ b/src/timedate1/timedate1_client.cc @@ -63,8 +63,11 @@ void appendTimeUSecAsDate(const uint64_t timeUSec, std::ostringstream& ss) { std::chrono::microseconds(timeUSec)); const std::time_t timeT = std::chrono::system_clock::to_time_t(timePoint); std::tm tm_buf{}; - const std::tm* tm = localtime_r(&timeT, &tm_buf); - ss << "\tDate: " << std::put_time(tm, "%d-%m-%Y %H:%M:%S") << std::endl; + if (const std::tm* tm = localtime_r(&timeT, &tm_buf); tm != nullptr) { + ss << "\tDate: " << std::put_time(tm, "%d-%m-%Y %H:%M:%S") << std::endl; + } else { + ss << "\tDate: (invalid)" << std::endl; + } } void Timedate1Client::printTimedate1() const { diff --git a/src/udisks2/udisks2_manager.cc b/src/udisks2/udisks2_manager.cc index 582409e..951ed26 100644 --- a/src/udisks2/udisks2_manager.cc +++ b/src/udisks2/udisks2_manager.cc @@ -53,6 +53,7 @@ void UDisks2Manager::onInterfacesAdded( continue; } if ("org.freedesktop.UDisks2.Manager.NVMe" == interface) { + std::lock_guard lock(manager_nvme_mutex_); manager_nvme_ = std::make_unique( getProxy().getConnection(), objectPath); } else if ("org.freedesktop.UDisks2.Block" == interface) { @@ -115,6 +116,7 @@ void UDisks2Manager::onInterfacesRemoved( for (const auto& interface : interfaces) { LOG_INFO("[{}] Remove - {}", objectPath, interface); if ("org.freedesktop.UDisks2.Manager.NVMe" == interface) { + std::lock_guard lock(manager_nvme_mutex_); manager_nvme_.reset(); } else if ("org.freedesktop.UDisks2.Block" == interface) { std::lock_guard lock(block_mutex_); diff --git a/src/udisks2/udisks2_manager.h b/src/udisks2/udisks2_manager.h index 56a8d3c..33a3b5f 100644 --- a/src/udisks2/udisks2_manager.h +++ b/src/udisks2/udisks2_manager.h @@ -38,6 +38,7 @@ class UDisks2Manager final static constexpr auto OBJECT_PATH = "/org/freedesktop/UDisks2"; std::unique_ptr manager_nvme_; + std::mutex manager_nvme_mutex_; std::map> blocks_; std::mutex block_mutex_; diff --git a/src/upower/upower_display_device.cc b/src/upower/upower_display_device.cc index 543bf3d..02bf902 100644 --- a/src/upower/upower_display_device.cc +++ b/src/upower/upower_display_device.cc @@ -41,146 +41,80 @@ void UPowerDisplayDevice::onPropertiesChanged( const sdbus::InterfaceName& interfaceName, const std::map& changedProperties, const std::vector& invalidatedProperties) { - if (const auto key = sdbus::MemberName("BatteryLevel"); - changedProperties.contains(key)) { - properties_.battery_level = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Capacity"); - changedProperties.contains(key)) { - properties_.capacity = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("ChargeCycles"); - changedProperties.contains(key)) { - properties_.charge_cycles = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("ChargeEndThreshold"); - changedProperties.contains(key)) { - properties_.charge_end_threshold = - changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("ChargeStartThreshold"); - changedProperties.contains(key)) { - properties_.charge_start_threshold = - changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("ChargeThresholdEnabled"); - changedProperties.contains(key)) { - properties_.charge_threshold_enabled = - changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("ChargeThresholdSupported"); - changedProperties.contains(key)) { - properties_.charge_threshold_supported = - changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Energy"); - changedProperties.contains(key)) { - properties_.energy = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("EnergyEmpty"); - changedProperties.contains(key)) { - properties_.energy_empty = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("EnergyFull"); - changedProperties.contains(key)) { - properties_.energy_full = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("EnergyFullDesign"); - changedProperties.contains(key)) { - properties_.energy_full_design = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("EnergyRate"); - changedProperties.contains(key)) { - properties_.energy_rate = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("HasHistory"); - changedProperties.contains(key)) { - properties_.has_history = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("HasStatistics"); - changedProperties.contains(key)) { - properties_.has_statistics = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("IconName"); - changedProperties.contains(key)) { - properties_.icon_name = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("IsPresent"); - changedProperties.contains(key)) { - properties_.is_present = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("IsRechargeable"); - changedProperties.contains(key)) { - properties_.is_rechargeable = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Luminosity"); - changedProperties.contains(key)) { - properties_.luminosity = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Model"); - changedProperties.contains(key)) { - properties_.model = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("NativePath"); - changedProperties.contains(key)) { - properties_.native_path = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Online"); - changedProperties.contains(key)) { - properties_.online = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Percentage"); - changedProperties.contains(key)) { - properties_.percentage = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("PowerSupply"); - changedProperties.contains(key)) { - properties_.power_supply = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Serial"); - changedProperties.contains(key)) { - properties_.serial = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("State"); - changedProperties.contains(key)) { - properties_.state = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Technology"); - changedProperties.contains(key)) { - properties_.technology = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Temperature"); - changedProperties.contains(key)) { - properties_.temperature = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("TimeToEmpty"); - changedProperties.contains(key)) { - properties_.time_to_empty = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("TimeToFull"); - changedProperties.contains(key)) { - properties_.time_to_full = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Type"); - changedProperties.contains(key)) { - properties_.type = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("UpdateTime"); - changedProperties.contains(key)) { - properties_.update_time = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Vendor"); - changedProperties.contains(key)) { - properties_.vendor = changedProperties.at(key).get(); - } - if (const auto key = sdbus::MemberName("Voltage"); - changedProperties.contains(key)) { - properties_.voltage = changedProperties.at(key).get(); - } - - if (const auto key = sdbus::MemberName("WarningLevel"); - changedProperties.contains(key)) { - properties_.warning_level = changedProperties.at(key).get(); + // Iterate the changed properties once and dispatch on the key. A + // PropertiesChanged signal usually carries only a handful of properties, + // so this avoids scanning every known property (and the per-property map + // lookup + MemberName allocation) on every signal. + for (const auto& [key, value] : changedProperties) { + if (key == "BatteryLevel") { + properties_.battery_level = value.get(); + } else if (key == "Capacity") { + properties_.capacity = value.get(); + } else if (key == "ChargeCycles") { + properties_.charge_cycles = value.get(); + } else if (key == "ChargeEndThreshold") { + properties_.charge_end_threshold = value.get(); + } else if (key == "ChargeStartThreshold") { + properties_.charge_start_threshold = value.get(); + } else if (key == "ChargeThresholdEnabled") { + properties_.charge_threshold_enabled = value.get(); + } else if (key == "ChargeThresholdSupported") { + properties_.charge_threshold_supported = value.get(); + } else if (key == "Energy") { + properties_.energy = value.get(); + } else if (key == "EnergyEmpty") { + properties_.energy_empty = value.get(); + } else if (key == "EnergyFull") { + properties_.energy_full = value.get(); + } else if (key == "EnergyFullDesign") { + properties_.energy_full_design = value.get(); + } else if (key == "EnergyRate") { + properties_.energy_rate = value.get(); + } else if (key == "HasHistory") { + properties_.has_history = value.get(); + } else if (key == "HasStatistics") { + properties_.has_statistics = value.get(); + } else if (key == "IconName") { + properties_.icon_name = value.get(); + } else if (key == "IsPresent") { + properties_.is_present = value.get(); + } else if (key == "IsRechargeable") { + properties_.is_rechargeable = value.get(); + } else if (key == "Luminosity") { + properties_.luminosity = value.get(); + } else if (key == "Model") { + properties_.model = value.get(); + } else if (key == "NativePath") { + properties_.native_path = value.get(); + } else if (key == "Online") { + properties_.online = value.get(); + } else if (key == "Percentage") { + properties_.percentage = value.get(); + } else if (key == "PowerSupply") { + properties_.power_supply = value.get(); + } else if (key == "Serial") { + properties_.serial = value.get(); + } else if (key == "State") { + properties_.state = value.get(); + } else if (key == "Technology") { + properties_.technology = value.get(); + } else if (key == "Temperature") { + properties_.temperature = value.get(); + } else if (key == "TimeToEmpty") { + properties_.time_to_empty = value.get(); + } else if (key == "TimeToFull") { + properties_.time_to_full = value.get(); + } else if (key == "Type") { + properties_.type = value.get(); + } else if (key == "UpdateTime") { + properties_.update_time = value.get(); + } else if (key == "Vendor") { + properties_.vendor = value.get(); + } else if (key == "Voltage") { + properties_.voltage = value.get(); + } else if (key == "WarningLevel") { + properties_.warning_level = value.get(); + } } Utils::print_changed_properties(interfaceName, changedProperties, invalidatedProperties); diff --git a/src/utils/resource_limits.h b/src/utils/resource_limits.h index 6f6505e..ac1be3f 100644 --- a/src/utils/resource_limits.h +++ b/src/utils/resource_limits.h @@ -27,6 +27,7 @@ inline constexpr std::size_t kMaxGattDescriptors = 32768; inline constexpr std::size_t kMaxBatteryEntries = 1024; inline constexpr std::size_t kMaxInputEntries = 1024; inline constexpr std::size_t kMaxUPowerClients = 256; +inline constexpr std::size_t kMaxUnits = 16384; inline bool IsAtCapacity(const std::size_t current_size, const std::size_t limit) { diff --git a/src/utils/utils.cc b/src/utils/utils.cc index a907829..6a998b0 100644 --- a/src/utils/utils.cc +++ b/src/utils/utils.cc @@ -131,10 +131,8 @@ const std::unordered_map< }}, {"s", [](const sdbus::Variant& v, std::ostringstream& os) { - os << (v.get().empty() - ? "\"\"" - : v.get()) - << std::endl; + const auto s = v.get(); + os << (s.empty() ? "\"\"" : s) << std::endl; }}, {"x", [](const sdbus::Variant& v, std::ostringstream& os) { @@ -148,62 +146,64 @@ const std::unordered_map< }}, {"as", [](const sdbus::Variant& v, std::ostringstream& os) { + const auto vec = v.get>(); os << std::endl; - for (const auto& s : - v.get>()) { + for (const auto& s : vec) { os << "\t" << s << std::endl; } - if (v.get>().empty()) + if (vec.empty()) os << "\t" << "\"\"" << std::endl; }}, {"au", [](const sdbus::Variant& v, std::ostringstream& os) { + const auto vec = v.get>(); os << std::endl; - for (const auto& s : - v.get>()) { + for (const auto& s : vec) { os << "\t" << std::to_string(s) << std::endl; } - if (v.get>().empty()) + if (vec.empty()) os << "\t" << "\"\"" << std::endl; else os << std::endl; }}, {"aau", [](const sdbus::Variant& v, std::ostringstream& os) { + const auto vec = + v.get>>(); os << std::endl; - for (const auto& it : - v.get>>()) { + for (const auto& it : vec) { for (const auto& address : it) { os << "\t" << std::to_string(address) << std::endl; } } - if (v.get>>() - .empty()) + if (vec.empty()) os << "\t" << "\"\"" << std::endl; else os << std::endl; }}, {"ay", [](const sdbus::Variant& v, std::ostringstream& os) { - for (const auto& b : v.get>()) { + const auto vec = v.get>(); + for (const auto& b : vec) { os << std::hex << std::setw(2) << std::setfill('0') << static_cast(b) << " "; } - if (v.get>().empty()) + if (vec.empty()) os << "\t" << "\"\"" << std::endl; else os << std::endl; }}, {"aay", [](const sdbus::Variant& v, std::ostringstream& os) { - for (const auto& it : - v.get>>()) { + const auto vec = + v.get>>(); + for (const auto& it : vec) { for (const auto& b : it) { os << std::hex << std::setw(2) << std::setfill('0') << static_cast(b) << " "; } } - if (v.get>>().empty()) + if (vec.empty()) os << "\t" << "\"\"" << std::endl; else os << std::endl;