Issue 133 race in proxy destruct (#177)

* fix construct/destruct order of mutex/data in AsnycCalls

Destroying a mutex which might still be owned leads to undefined behaviour.
It wasn't the case here but using the right order is more resistant to future bugs.

* proxy AsnycCalls: don't leave map in unspecified state

* fix 133: use thread-safe container for proxy interfaces list

* proxy callback payload uses concrete signalData

* proxy: revert adding the wrapper class InterfaceContainer

It's no longer required and simplifies the locking logic.

* Proxy: avoid additional lambda wrapper around signal callbacks

As proposed in
https://github.com/Kistler-Group/sdbus-cpp/pull/177#issuecomment-834439707
option 3.

Still TODO: Avoid relying on std::map's non-invalidatable by adding just
std::unique_ptr<Proxy::InterfaceData::SignalData> to the container.

* proxy: add missing underscore prefix

* proxy: put InterfaceData::SignalData into a unique_ptr

This ways, we avoid relying on std::map's non-invalidatable references.

* proxy: code style: get raw pointer directly

* style: fix code style

Co-authored-by: Stanislav Angelovic <stanislav.angelovic@siemens.com>

src/Proxy.cpp

@@ -34,7 +34,7 @@
 #include <systemd/sd-bus.h>
 #include <cassert>
 #include <chrono>
-#include <thread>
+#include <utility>
 
 namespace sdbus::internal {
 
@@ -160,7 +160,7 @@ void Proxy::registerSignalHandler( const std::string& interfaceName
 
     auto& interface = interfaces_[interfaceName];
 
-    InterfaceData::SignalData signalData{std::move(signalHandler), nullptr};
+    auto signalData = std::make_unique<InterfaceData::SignalData>(*this, std::move(signalHandler), nullptr);
     auto insertionResult = interface.signals_.emplace(signalName, std::move(signalData));
 
     auto inserted = insertionResult.second;
@@ -182,13 +182,13 @@ void Proxy::registerSignalHandlers(sdbus::internal::IConnection& connection)
         for (auto& signalItem : signalsOnInterface)
         {
             const auto& signalName = signalItem.first;
-            auto& slot = signalItem.second.slot_;
-            slot = connection.registerSignalHandler( destination_
-                                                   , objectPath_
-                                                   , interfaceName
-                                                   , signalName
-                                                   , &Proxy::sdbus_signal_handler
-                                                   , this );
+            auto* signalData = signalItem.second.get();
+            signalData->slot = connection.registerSignalHandler( destination_
+                                                               , objectPath_
+                                                               , interfaceName
+                                                               , signalName
+                                                               , &Proxy::sdbus_signal_handler
+                                                               , signalData);
         }
     }
 }
@@ -245,17 +245,12 @@ int Proxy::sdbus_async_reply_handler(sd_bus_message *sdbusMessage, void *userDat
 
 int Proxy::sdbus_signal_handler(sd_bus_message *sdbusMessage, void *userData, sd_bus_error */*retError*/)
 {
-    auto* proxy = static_cast<Proxy*>(userData);
-    assert(proxy != nullptr);
-
-    auto message = Message::Factory::create<Signal>(sdbusMessage, &proxy->connection_->getSdBusInterface());
-
-    // Note: The lookup can be optimized by using sorted vectors instead of associative containers
-    auto& callback = proxy->interfaces_[message.getInterfaceName()].signals_[message.getMemberName()].callback_;
-    assert(callback);
-
-    callback(message);
+    auto* signalData = static_cast<InterfaceData::SignalData*>(userData);
+    assert(signalData != nullptr);
+    assert(signalData->callback);
 
+    auto message = Message::Factory::create<Signal>(sdbusMessage, &signalData->proxy.connection_->getSdBusInterface());
+    signalData->callback(message);
     return 0;
 }
 

src/Proxy.h

@@ -98,10 +98,20 @@ namespace sdbus::internal {
             using SignalName = std::string;
             struct SignalData
             {
-                signal_handler callback_;
-                SlotPtr slot_;
+                SignalData(Proxy& proxy, signal_handler callback, SlotPtr slot)
+                    : proxy(proxy)
+                    , callback(std::move(callback))
+                    , slot(std::move(slot))
+                {}
+                Proxy& proxy;
+                signal_handler callback;
+                // slot_ must be listed after callback_ to ensure that slot_ is destructed first.
+                // Destructing the slot_ will sd_bus_slot_unref() the callback.
+                // Only after sd_bus_slot_unref(), we can safely delete the callback. The bus mutex (SdBus::sdbusMutex_)
+                // ensures that sd_bus_slot_unref() and the callback execute sequentially.
+                SlotPtr slot;
             };
-            std::map<SignalName, SignalData> signals_;
+            std::map<SignalName, std::unique_ptr<SignalData>> signals_;
         };
         std::map<InterfaceName, InterfaceData> interfaces_;
 
@@ -150,6 +160,7 @@ namespace sdbus::internal {
             {
                 std::unique_lock lock(mutex_);
                 auto asyncCallSlots = std::move(calls_);
+                calls_ = {};
                 lock.unlock();
 
                 // Releasing call slot pointer acquires global sd-bus mutex. We have to perform the release
@@ -159,8 +170,8 @@ namespace sdbus::internal {
             }
 
         private:
-            std::unordered_map<void*, std::shared_ptr<CallData>> calls_;
             std::mutex mutex_;
+            std::unordered_map<void*, std::shared_ptr<CallData>> calls_;
         } pendingAsyncCalls_;
     };