From a578493dbaefcb03c8162bbed72fbf150f4c64ea Mon Sep 17 00:00:00 2001
From: pixl <pixlone@pm.me>
Date: Tue, 2 May 2023 14:30:47 -0400
Subject: [PATCH] Require HID++ devices to be made as shared_ptrs

Binds event handlers and tasks to lifetime of object. Ensures no race
conditions occur during destruction of HID++ devices.
---
 src/logid/Device.cpp                          | 33 ++++-----
 src/logid/Device.h                            |  2 +-
 src/logid/DeviceManager.cpp                   |  7 +-
 src/logid/Receiver.cpp                        |  8 ++-
 src/logid/backend/hidpp/Device.cpp            | 14 ++--
 src/logid/backend/hidpp/Device.h              | 67 +++++++++++++++----
 src/logid/backend/hidpp10/Device.cpp          |  3 -
 src/logid/backend/hidpp10/Device.h            | 33 ++++++---
 src/logid/backend/hidpp10/Receiver.cpp        |  3 +
 src/logid/backend/hidpp10/Receiver.h          | 20 +++++-
 src/logid/backend/hidpp10/ReceiverMonitor.cpp |  2 +-
 src/logid/backend/hidpp20/Device.cpp          |  9 ---
 src/logid/backend/hidpp20/Device.h            | 35 ++++++----
 13 files changed, 157 insertions(+), 79 deletions(-)

diff --git a/src/logid/Device.cpp b/src/logid/Device.cpp
index 1a617f4..53c59fe 100644
--- a/src/logid/Device.cpp
+++ b/src/logid/Device.cpp
@@ -93,10 +93,10 @@ std::shared_ptr<Device> Device::make(
 
 Device::Device(std::string path, backend::hidpp::DeviceIndex index,
                const std::shared_ptr<DeviceManager>& manager) :
-        _hidpp20(path, index, manager,
-                 manager->config()->io_timeout.value_or(defaults::io_timeout)),
+        _hidpp20(hidpp20::Device::make(path, index, manager,
+                 manager->config()->io_timeout.value_or(defaults::io_timeout))),
         _path(std::move(path)), _index(index),
-        _config(_getConfig(manager, _hidpp20.name())),
+        _config(_getConfig(manager, _hidpp20->name())),
         _receiver(nullptr),
         _manager(manager),
         _nickname(manager),
@@ -107,10 +107,11 @@ Device::Device(std::string path, backend::hidpp::DeviceIndex index,
 
 Device::Device(std::shared_ptr<backend::raw::RawDevice> raw_device,
                hidpp::DeviceIndex index, const std::shared_ptr<DeviceManager>& manager) :
-        _hidpp20(std::move(raw_device), index,
-                 manager->config()->io_timeout.value_or(defaults::io_timeout)),
+        _hidpp20(hidpp20::Device::make(
+                std::move(raw_device), index,
+                manager->config()->io_timeout.value_or(defaults::io_timeout))),
         _path(raw_device->rawPath()), _index(index),
-        _config(_getConfig(manager, _hidpp20.name())),
+        _config(_getConfig(manager, _hidpp20->name())),
         _receiver(nullptr),
         _manager(manager),
         _nickname(manager),
@@ -121,11 +122,11 @@ Device::Device(std::shared_ptr<backend::raw::RawDevice> raw_device,
 
 Device::Device(Receiver* receiver, hidpp::DeviceIndex index,
                const std::shared_ptr<DeviceManager>& manager) :
-        _hidpp20(receiver->rawReceiver(), index,
-                 manager->config()->io_timeout.value_or(
-                         defaults::io_timeout)),
+        _hidpp20(hidpp20::Device::make(
+                receiver->rawReceiver(), index,
+                manager->config()->io_timeout.value_or(defaults::io_timeout))),
         _path(receiver->path()), _index(index),
-        _config(_getConfig(manager, _hidpp20.name())),
+        _config(_getConfig(manager, _hidpp20->name())),
         _receiver(receiver),
         _manager(manager),
         _nickname(manager),
@@ -159,11 +160,11 @@ void Device::_init() {
 }
 
 std::string Device::name() {
-    return _hidpp20.name();
+    return _hidpp20->name();
 }
 
 uint16_t Device::pid() {
-    return _hidpp20.pid();
+    return _hidpp20->pid();
 }
 
 void Device::sleep() {
@@ -223,15 +224,15 @@ const config::Profile& Device::activeProfile() const {
 }
 
 hidpp20::Device& Device::hidpp20() {
-    return _hidpp20;
+    return *_hidpp20;
 }
 
 void Device::_makeResetMechanism() {
     try {
-        hidpp20::Reset reset(&_hidpp20);
+        hidpp20::Reset reset(_hidpp20.get());
         _reset_mechanism = std::make_unique<std::function<void()>>(
-                [dev = &this->_hidpp20] {
-                    hidpp20::Reset reset(dev);
+                [dev = _hidpp20] {
+                    hidpp20::Reset reset(dev.get());
                     reset.reset(reset.getProfile());
                 });
     } catch (hidpp20::UnsupportedFeature& e) {
diff --git a/src/logid/Device.h b/src/logid/Device.h
index a356673..a071d42 100644
--- a/src/logid/Device.h
+++ b/src/logid/Device.h
@@ -134,7 +134,7 @@ namespace logid {
             }
         }
 
-        backend::hidpp20::Device _hidpp20;
+        std::shared_ptr<backend::hidpp20::Device> _hidpp20;
         std::string _path;
         backend::hidpp::DeviceIndex _index;
         std::map<std::string, std::shared_ptr<features::DeviceFeature>>
diff --git a/src/logid/DeviceManager.cpp b/src/logid/DeviceManager.cpp
index 2328b03..aee2129 100644
--- a/src/logid/DeviceManager.cpp
+++ b/src/logid/DeviceManager.cpp
@@ -95,9 +95,10 @@ void DeviceManager::addDevice(std::string path) {
     }
 
     try {
-        hidpp::Device device(path, hidpp::DefaultDevice, _self.lock(),
-                             config()->io_timeout.value_or(defaults::io_timeout));
-        isReceiver = device.version() == std::make_tuple(1, 0);
+        auto device = hidpp::Device::make(
+                path, hidpp::DefaultDevice, _self.lock(),
+                config()->io_timeout.value_or(defaults::io_timeout));
+        isReceiver = device->version() == std::make_tuple(1, 0);
     } catch (hidpp20::Error& e) {
         if (e.code() != hidpp20::Error::UnknownDevice)
             throw DeviceNotReady();
diff --git a/src/logid/Receiver.cpp b/src/logid/Receiver.cpp
index 56c94a2..cc6cdc9 100644
--- a/src/logid/Receiver.cpp
+++ b/src/logid/Receiver.cpp
@@ -111,10 +111,10 @@ void Receiver::addDevice(hidpp::DeviceConnectionEvent event) {
         if (!event.linkEstablished)
             return;
 
-        hidpp::Device hidpp_device(receiver(), event,
-                                   manager->config()->io_timeout.value_or(defaults::io_timeout));
+        auto hidpp_device = hidpp::Device::make(
+                receiver(), event, manager->config()->io_timeout.value_or(defaults::io_timeout));
 
-        auto version = hidpp_device.version();
+        auto version = hidpp_device->version();
 
         if (std::get<0>(version) < 2) {
             logPrintf(INFO, "Unsupported HID++ 1.0 device on %s:%d connected.",
@@ -122,6 +122,8 @@ void Receiver::addDevice(hidpp::DeviceConnectionEvent event) {
             return;
         }
 
+        hidpp_device.reset();
+
         auto device = Device::make(this, event.index, manager);
         std::lock_guard<std::mutex> manager_lock(manager->mutex());
         _devices.emplace(event.index, device);
diff --git a/src/logid/backend/hidpp/Device.cpp b/src/logid/backend/hidpp/Device.cpp
index 4eba5f4..e1dbd51 100644
--- a/src/logid/backend/hidpp/Device.cpp
+++ b/src/logid/backend/hidpp/Device.cpp
@@ -55,7 +55,6 @@ Device::Device(const std::string& path, DeviceIndex index,
                 duration<double, std::milli>(timeout))),
         _raw_device(std::make_shared<raw::RawDevice>(path, monitor)),
         _receiver(nullptr), _path(path), _index(index) {
-    _setupReportsAndInit();
 }
 
 Device::Device(std::shared_ptr<raw::RawDevice> raw_device, DeviceIndex index,
@@ -64,7 +63,6 @@ Device::Device(std::shared_ptr<raw::RawDevice> raw_device, DeviceIndex index,
                 duration<double, std::milli>(timeout))),
         _raw_device(std::move(raw_device)), _receiver(nullptr),
         _path(_raw_device->rawPath()), _index(index) {
-    _setupReportsAndInit();
 }
 
 Device::Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
@@ -81,7 +79,6 @@ Device::Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
         _pid = event.pid;
     else
         _pid = receiver->getPairingInfo(_index).pid;
-    _setupReportsAndInit();
 }
 
 Device::Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
@@ -92,7 +89,6 @@ Device::Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
         _receiver(receiver), _path(receiver->rawDevice()->rawPath()),
         _index(index) {
     _pid = receiver->getPairingInfo(_index).pid;
-    _setupReportsAndInit();
 }
 
 const std::string& Device::devicePath() const {
@@ -124,9 +120,10 @@ void Device::_setupReportsAndInit() {
                     return (report[Offset::Type] == Report::Type::Short ||
                             report[Offset::Type] == Report::Type::Long);
                 },
-                 [this](const std::vector<uint8_t>& report) -> void {
+                 [self_weak = _self](const std::vector<uint8_t>& report) -> void {
                      Report _report(report);
-                     this->handleEvent(_report);
+                     if(auto self = self_weak.lock())
+                        self->handleEvent(_report);
                  }});
 
         try {
@@ -154,9 +151,10 @@ void Device::_setupReportsAndInit() {
                         report[Offset::Type] == Report::Type::Long) &&
                        (report[Offset::DeviceIndex] == index);
             },
-             [this](const std::vector<uint8_t>& report) -> void {
+             [self_weak = _self](const std::vector<uint8_t>& report) -> void {
                  Report _report(report);
-                 this->handleEvent(_report);
+                 if(auto self = self_weak.lock())
+                     self->handleEvent(_report);
              }});
 
     _init();
diff --git a/src/logid/backend/hidpp/Device.h b/src/logid/backend/hidpp/Device.h
index de0de92..9646381 100644
--- a/src/logid/backend/hidpp/Device.h
+++ b/src/logid/backend/hidpp/Device.h
@@ -38,7 +38,24 @@ namespace logid::backend::hidpp10 {
 namespace logid::backend::hidpp {
     struct DeviceConnectionEvent;
 
+    namespace {
+        template <typename T>
+        class DeviceWrapper : public T {
+            friend class Device;
+        public:
+            template <typename... Args>
+            explicit DeviceWrapper(Args... args) : T(std::forward<Args>(args)...) { }
+
+            template <typename... Args>
+            static std::shared_ptr<T> make(Args... args) {
+                return std::make_shared<DeviceWrapper>(std::forward<Args>(args)...);
+            }
+        };
+    }
+
     class Device {
+        template <typename T>
+        friend class DeviceWrapper;
     public:
         struct EventHandler {
             std::function<bool(Report&)> condition;
@@ -64,18 +81,6 @@ namespace logid::backend::hidpp {
             Reason _reason;
         };
 
-        Device(const std::string& path, DeviceIndex index,
-               const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout);
-
-        Device(std::shared_ptr<raw::RawDevice> raw_device, DeviceIndex index,
-               double timeout);
-
-        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
-               hidpp::DeviceConnectionEvent event, double timeout);
-
-        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
-               DeviceIndex index, double timeout);
-
         [[nodiscard]] const std::string& devicePath() const;
 
         [[nodiscard]] DeviceIndex deviceIndex() const;
@@ -96,7 +101,23 @@ namespace logid::backend::hidpp {
 
         [[nodiscard]] const std::shared_ptr<raw::RawDevice>& rawDevice() const;
 
+        Device(const Device&) = delete;
+        Device(Device&&) = delete;
+        virtual ~Device() = default;
+
     protected:
+        Device(const std::string& path, DeviceIndex index,
+               const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout);
+
+        Device(std::shared_ptr<raw::RawDevice> raw_device, DeviceIndex index,
+               double timeout);
+
+        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
+               hidpp::DeviceConnectionEvent event, double timeout);
+
+        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
+               DeviceIndex index, double timeout);
+
         // Returns whether the report is a response
         virtual bool responseReport(const Report& report);
 
@@ -108,6 +129,11 @@ namespace logid::backend::hidpp {
 
         void reportFixup(Report& report) const;
 
+        template <typename T>
+        [[nodiscard]] std::weak_ptr<T> self() const {
+            return std::dynamic_pointer_cast<T>(_self);
+        }
+
         const std::chrono::milliseconds io_timeout;
         uint8_t supported_reports{};
 
@@ -136,6 +162,23 @@ namespace logid::backend::hidpp {
         std::optional<uint8_t> _sent_sub_id{};
 
         std::shared_ptr<EventHandlerList<Device>> _event_handlers;
+
+        std::weak_ptr<Device> _self;
+
+    protected:
+        template <typename T, typename... Args>
+        static std::shared_ptr<T> makeDerived(Args... args) {
+            auto device = DeviceWrapper<T>::make(std::forward<Args>(args)...);
+            device->_self = device;
+            device->_setupReportsAndInit();
+            return device;
+        }
+
+    public:
+        template <typename... Args>
+        static std::shared_ptr<Device> make(Args... args) {
+            return makeDerived<Device>(std::forward<Args>(args)...);
+        }
     };
 
     typedef Device::EventHandler EventHandler;
diff --git a/src/logid/backend/hidpp10/Device.cpp b/src/logid/backend/hidpp10/Device.cpp
index 023abe0..f2972c9 100644
--- a/src/logid/backend/hidpp10/Device.cpp
+++ b/src/logid/backend/hidpp10/Device.cpp
@@ -27,19 +27,16 @@ using namespace logid::backend::hidpp10;
 Device::Device(const std::string& path, hidpp::DeviceIndex index,
                const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout) :
         hidpp::Device(path, index, monitor, timeout) {
-    assert(version() == std::make_tuple(1, 0));
 }
 
 Device::Device(std::shared_ptr<raw::RawDevice> raw_dev, hidpp::DeviceIndex index,
                double timeout) : hidpp::Device(std::move(raw_dev), index, timeout) {
-    assert(version() == std::make_tuple(1, 0));
 }
 
 Device::Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
                hidpp::DeviceIndex index,
                double timeout)
         : hidpp::Device(receiver, index, timeout) {
-    assert(version() == std::make_tuple(1, 0));
 }
 
 void Device::ResponseSlot::reset() {
diff --git a/src/logid/backend/hidpp10/Device.h b/src/logid/backend/hidpp10/Device.h
index 9e46c27..5ae3fd6 100644
--- a/src/logid/backend/hidpp10/Device.h
+++ b/src/logid/backend/hidpp10/Device.h
@@ -28,14 +28,6 @@
 namespace logid::backend::hidpp10 {
     class Device : public hidpp::Device {
     public:
-        Device(const std::string& path, hidpp::DeviceIndex index,
-               const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout);
-
-        Device(std::shared_ptr<raw::RawDevice> raw_dev,
-               hidpp::DeviceIndex index, double timeout);
-
-        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
-               hidpp::DeviceIndex index, double timeout);
 
         hidpp::Report sendReport(const hidpp::Report& report) final;
 
@@ -48,6 +40,15 @@ namespace logid::backend::hidpp10 {
                                          hidpp::Report::Type type);
 
     protected:
+        Device(const std::string& path, hidpp::DeviceIndex index,
+               const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout);
+
+        Device(std::shared_ptr<raw::RawDevice> raw_dev,
+               hidpp::DeviceIndex index, double timeout);
+
+        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
+               hidpp::DeviceIndex index, double timeout);
+
         bool responseReport(const hidpp::Report& report) final;
 
     private:
@@ -61,6 +62,22 @@ namespace logid::backend::hidpp10 {
 
         std::vector<uint8_t> accessRegister(
                 uint8_t sub_id, uint8_t address, const std::vector<uint8_t>& params);
+
+    protected:
+        template <typename T, typename... Args>
+        static std::shared_ptr<T> makeDerived(Args... args) {
+            auto device = hidpp::Device::makeDerived<T>(std::forward<Args>(args)...);
+
+            if (std::get<0>(device->version()) != 1)
+                throw std::invalid_argument("not a hid++ 1.0 device");
+
+            return device;
+        }
+    public:
+        template <typename... Args>
+        static std::shared_ptr<Device> make(Args... args) {
+            return makeDerived<Device>(std::forward<Args>(args)...);
+        }
     };
 }
 
diff --git a/src/logid/backend/hidpp10/Receiver.cpp b/src/logid/backend/hidpp10/Receiver.cpp
index 3762260..65ee05c 100644
--- a/src/logid/backend/hidpp10/Receiver.cpp
+++ b/src/logid/backend/hidpp10/Receiver.cpp
@@ -28,6 +28,9 @@ const char* InvalidReceiver::what() const noexcept {
 
 Receiver::Receiver(const std::string& path, const std::shared_ptr<raw::DeviceMonitor>& monitor,
                    double timeout) : Device(path, hidpp::DefaultDevice, monitor, timeout) {
+}
+
+void Receiver::_receiverCheck() {
     // Check if the device is a receiver
     try {
         getNotificationFlags();
diff --git a/src/logid/backend/hidpp10/Receiver.h b/src/logid/backend/hidpp10/Receiver.h
index ea3933d..91a7038 100644
--- a/src/logid/backend/hidpp10/Receiver.h
+++ b/src/logid/backend/hidpp10/Receiver.h
@@ -89,9 +89,6 @@ namespace logid::backend::hidpp10 {
 
     class Receiver : public Device {
     public:
-        Receiver(const std::string& path,
-                 const std::shared_ptr<raw::DeviceMonitor>& monitor,
-                 double timeout);
 
         /* The following functions deal with HID++ 1.0 features.
          * While these are not technically DJ functions, it is redundant
@@ -199,8 +196,25 @@ namespace logid::backend::hidpp10 {
 
         static std::string passkeyEvent(const hidpp::Report& report);
 
+    protected:
+        Receiver(const std::string& path,
+                 const std::shared_ptr<raw::DeviceMonitor>& monitor,
+                 double timeout);
+
     private:
+        void _receiverCheck();
+
         bool _is_bolt = false;
+
+    public:
+        template <typename... Args>
+        static std::shared_ptr<Receiver> make(Args... args) {
+            auto receiver = makeDerived<Receiver>(std::forward<Args>(args)...);
+
+            receiver->_receiverCheck();
+
+            return receiver;
+        }
     };
 }
 
diff --git a/src/logid/backend/hidpp10/ReceiverMonitor.cpp b/src/logid/backend/hidpp10/ReceiverMonitor.cpp
index 4d0fe94..adc27e2 100644
--- a/src/logid/backend/hidpp10/ReceiverMonitor.cpp
+++ b/src/logid/backend/hidpp10/ReceiverMonitor.cpp
@@ -25,7 +25,7 @@ using namespace logid::backend::hidpp;
 
 ReceiverMonitor::ReceiverMonitor(const std::string& path,
                                  const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout)
-        : _receiver(std::make_shared<Receiver>(path, monitor, timeout)) {
+        : _receiver(Receiver::make(path, monitor, timeout)) {
 
     Receiver::NotificationFlags notification_flags{true, true, true};
     _receiver->setNotifications(notification_flags);
diff --git a/src/logid/backend/hidpp20/Device.cpp b/src/logid/backend/hidpp20/Device.cpp
index ebb847b..adbcc27 100644
--- a/src/logid/backend/hidpp20/Device.cpp
+++ b/src/logid/backend/hidpp20/Device.cpp
@@ -27,30 +27,21 @@ using namespace logid::backend::hidpp20;
 Device::Device(const std::string& path, hidpp::DeviceIndex index,
                const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout) :
         hidpp::Device(path, index, monitor, timeout) {
-    // TODO: Fix version check
-    if (std::get<0>(version()) < 2)
-        throw std::runtime_error("Invalid HID++ version");
 }
 
 Device::Device(std::shared_ptr<raw::RawDevice> raw_device,
                hidpp::DeviceIndex index, double timeout) :
         hidpp::Device(std::move(raw_device), index, timeout) {
-    if (std::get<0>(version()) < 2)
-        throw std::runtime_error("Invalid HID++ version");
 }
 
 Device::Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
                hidpp::DeviceConnectionEvent event, double timeout) :
         hidpp::Device(receiver, event, timeout) {
-    if (std::get<0>(version()) < 2)
-        throw std::runtime_error("Invalid HID++ version");
 }
 
 Device::Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
                hidpp::DeviceIndex index, double timeout)
         : hidpp::Device(receiver, index, timeout) {
-    if (std::get<0>(version()) < 2)
-        throw std::runtime_error("Invalid HID++ version");
 }
 
 std::vector<uint8_t> Device::callFunction(uint8_t feature_index,
diff --git a/src/logid/backend/hidpp20/Device.h b/src/logid/backend/hidpp20/Device.h
index cc2fa73..a3e3103 100644
--- a/src/logid/backend/hidpp20/Device.h
+++ b/src/logid/backend/hidpp20/Device.h
@@ -28,18 +28,6 @@
 namespace logid::backend::hidpp20 {
     class Device : public hidpp::Device {
     public:
-        Device(const std::string& path, hidpp::DeviceIndex index,
-               const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout);
-
-        Device(std::shared_ptr<raw::RawDevice> raw_device,
-               hidpp::DeviceIndex index, double timeout);
-
-        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
-               hidpp::DeviceConnectionEvent event, double timeout);
-
-        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
-               hidpp::DeviceIndex index, double timeout);
-
         std::vector<uint8_t> callFunction(uint8_t feature_index,
                                           uint8_t function,
                                           std::vector<uint8_t>& params);
@@ -53,6 +41,18 @@ namespace logid::backend::hidpp20 {
         void sendReportNoACK(const hidpp::Report& report) final;
 
     protected:
+        Device(const std::string& path, hidpp::DeviceIndex index,
+               const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout);
+
+        Device(std::shared_ptr<raw::RawDevice> raw_device,
+               hidpp::DeviceIndex index, double timeout);
+
+        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
+               hidpp::DeviceConnectionEvent event, double timeout);
+
+        Device(const std::shared_ptr<hidpp10::Receiver>& receiver,
+               hidpp::DeviceIndex index, double timeout);
+
         bool responseReport(const hidpp::Report& report) final;
 
     private:
@@ -65,6 +65,17 @@ namespace logid::backend::hidpp20 {
 
         /* Multiplex responses on lower nibble of SubID, ignore upper nibble for space */
         std::array<ResponseSlot, 16> _responses;
+
+    public:
+        template <typename... Args>
+        static std::shared_ptr<Device> make(Args... args) {
+            auto device = makeDerived<Device>(std::forward<Args>(args)...);
+
+            if (std::get<0>(device->version()) < 2)
+                throw std::invalid_argument("not a hid++ 2.0 device");
+
+            return device;
+        }
     };
 }