Repository: ossia/libossia (language: python)

Issue:
mutliplex protocol thread safety: stop_expose_to and parameter push_value segfaults
Here is an example test that causes the crash:

https://github.com/Cycling74/libossia/blob/xnor/multiplex-test/tests/Network/MultiplexTest.cpp#L143-L227

When I run some code in a debugger I see a segfault when I `push_value` and the multiplex protocol iterates its protocols.


Discussion / hints:

Fix patch:
diff --git a/src/ossia/network/local/local.cpp b/src/ossia/network/local/local.cpp
index c9937039d87..fe8071107c8 100644
--- a/src/ossia/network/local/local.cpp
+++ b/src/ossia/network/local/local.cpp
@@ -6,6 +6,8 @@
 
 #include <ossia/detail/logger.hpp>
 
+using lock_guard = std::lock_guard<ossia::audio_spin_mutex>;
+
 namespace ossia::net
 {
 static void observe_rec(protocol_base& proto, ossia::net::node_base& n)
@@ -41,6 +43,7 @@ bool multiplex_protocol::pull(ossia::net::parameter_base& address)
 bool multiplex_protocol::push(const ossia::net::parameter_base& address, const ossia::value& v)
 {
   bool b = true;
+  lock_guard guard(m_protocols_mutex);
   for (auto& proto : m_protocols)
     b &= proto->push(address, v);
 
@@ -50,6 +53,7 @@ bool multiplex_protocol::push(const ossia::net::parameter_base& address, const o
 bool multiplex_protocol::push_raw(const full_parameter_data& dat)
 {
   bool b = true;
+  lock_guard guard(m_protocols_mutex);
   for (auto& proto : m_protocols)
     b &= proto->push_raw(dat);
 
@@ -60,6 +64,7 @@ bool multiplex_protocol::observe(
     ossia::net::parameter_base& address, bool enable)
 {
   bool b = true;
+  lock_guard guard(m_protocols_mutex);
   for (auto& proto : m_protocols)
     b &= proto->observe_quietly(address, enable);
 
@@ -74,6 +79,7 @@ bool multiplex_protocol::update(ossia::net::node_base& node)
 bool multiplex_protocol::echo_incoming_message(const message_origin_identifier& id, const parameter_base& param, const value& v)
 {
   bool b = true;
+  lock_guard guard(m_protocols_mutex);
   for (auto& proto : m_protocols)
     b &= proto->echo_incoming_message(id, param, v);
   return b;
@@ -81,6 +87,7 @@ bool multiplex_protocol::echo_incoming_message(const message_origin_identifier&
 
 void multiplex_protocol::stop()
 {
+  lock_guard guard(m_protocols_mutex);
   for (auto& proto : m_protocols)
     proto->stop();
 }
@@ -88,6 +95,7 @@ void multiplex_protocol::stop()
 void multiplex_protocol::set_device(device_base& dev)
 {
   m_device = &dev;
+  lock_guard guard(m_protocols_mutex);
   for(auto& p : m_protocols_to_register)
   {
     p->set_device(*m_device);
@@ -111,6 +119,7 @@ void multiplex_protocol::expose_to(std::unique_ptr<protocol_base> p)
        return;
     }
 
+    lock_guard guard(m_protocols_mutex);
     if(m_device)
     {
       p->set_device(*m_device);
@@ -129,12 +138,14 @@ void multiplex_protocol::expose_to(std::unique_ptr<protocol_base> p)
 
 void multiplex_protocol::stop_expose_to(const protocol_base& p)
 {
+  lock_guard guard(m_protocols_mutex);
   ossia::remove_erase_if(m_protocols_to_register, [&](const auto& ptr) { return ptr.get() == &p; });
   ossia::remove_erase_if(m_protocols, [&](const auto& ptr) { return ptr.get() == &p; });
 }
 
 void ossia::net::multiplex_protocol::clear()
 {
+  lock_guard guard(m_protocols_mutex);
   m_protocols.clear();
 }
 }
diff --git a/src/ossia/network/local/local.hpp b/src/ossia/network/local/local.hpp
index ea37c29012b..71c22a24d62 100644
--- a/src/ossia/network/local/local.hpp
+++ b/src/ossia/network/local/local.hpp
@@ -2,6 +2,7 @@
 
 #include <ossia/detail/algorithms.hpp>
 #include <ossia/network/base/protocol.hpp>
+#include <ossia/detail/audio_spin_mutex.hpp>
 
 #include <vector>
 
@@ -64,6 +65,7 @@ class OSSIA_EXPORT multiplex_protocol final : public ossia::net::protocol_base
 
 private:
   std::vector<std::unique_ptr<ossia::net::protocol_base>> m_protocols;
+  ossia::audio_spin_mutex m_protocols_mutex;
   std::vector<std::unique_ptr<ossia::net::protocol_base>> m_protocols_to_register;
   ossia::net::device_base* m_device{};
 };


Test patch:
diff --git a/tests/Network/MultiplexTest.cpp b/tests/Network/MultiplexTest.cpp
index 2c444c56f20..6feae4788ab 100644
--- a/tests/Network/MultiplexTest.cpp
+++ b/tests/Network/MultiplexTest.cpp
@@ -4,6 +4,7 @@
 #include <catch.hpp>
 
 #include <iostream>
+#include <thread>
 
 #include <ossia/context.hpp>
 #include <ossia/detail/config.hpp>
@@ -106,7 +107,7 @@ TEST_CASE ("test_oscquery_osc_out", "test_oscquery_osc_out")
 
 TEST_CASE ("test_oscquery_osc_large", "test_oscquery_osc_large")
 {
-  uint16_t shared = 9996;
+  uint16_t shared = 9997;
   auto ctx = std::make_shared<ossia::net::network_context>();
   using conf = ossia::net::osc_protocol_configuration;
 
@@ -139,4 +140,91 @@ TEST_CASE ("test_oscquery_osc_large", "test_oscquery_osc_large")
   ctx->context.run_for(std::chrono::milliseconds(100));
 }
 
+TEST_CASE ("test_multiplex_remove", "test_multiplex_remove")
+{
+  uint16_t shared = 9998;
+  auto ctx = std::make_shared<ossia::net::network_context>();
+  using conf = ossia::net::osc_protocol_configuration;
+
+  auto osc = ossia::net::make_osc_protocol(
+      ctx,
+      {
+        .mode = conf::HOST,
+        .version = conf::OSC1_1,
+        .transport = ossia::net::udp_configuration {{
+          .local = std::nullopt,
+          .remote = ossia::net::send_socket_configuration {{"127.0.0.1", shared}}
+        }}
+      }
+      );
+  auto oscquery = std::make_unique<ossia::oscquery_asio::oscquery_server_protocol>(ctx);
+
+  auto protou = std::make_unique<ossia::net::multiplex_protocol>(osc, oscquery);
+  //get the pointer so we can use the protocol after using it with the device
+  //is there a better way??
+  auto proto = protou.get();
+  generic_device device { std::move(protou), "my_device" };
+
+  device.set_echo(true);
+
+  ossia::net::generic_device remote{
+    ossia::net::make_osc_protocol(
+        ctx,
+        {
+          .mode = conf::MIRROR,
+          .version = conf::OSC1_1,
+          .transport = ossia::net::udp_configuration {{
+            .local = ossia::net::receive_socket_configuration {{"0.0.0.0", shared}},
+            .remote = std::nullopt
+          }}
+        }
+        ),
+      "test_osc"};
+
+
+  auto& bi = find_or_create_node(device, "/foo/bi");
+  auto bip = bi.create_parameter(ossia::val_type::INT);
+  bi.set(access_mode_attribute{}, access_mode::BI);
+  bool run = true;
+
+  //updating from another thread should  not crash
+  auto handle = std::thread([&run, bip]() mutable {
+      int v = 0;
+      while (run) {
+        bip->push_value(v++);
+      }
+  });
+
+  for (int i = 0; i < 10; i++) {
+    ctx->context.run_for(std::chrono::milliseconds(1));
+    for (auto& p: proto->get_protocols()) {
+      //i just want to remove the OSC protocol, but it is easier to match on not oscquery
+      auto o = dynamic_cast<ossia::oscquery_asio::oscquery_server_protocol*>(p.get());
+      if (!o) {
+        proto->stop_expose_to(*p);
+        break;
+      }
+    }
+
+    //create a new OSC connection
+    using conf = ossia::net::osc_protocol_configuration;
+    auto o = ossia::net::make_osc_protocol(
+        ctx,
+        {
+          .mode = conf::HOST,
+          .version = conf::OSC1_1,
+          .framing = conf::SLIP,
+          .transport = ossia::net::udp_configuration {{
+            .local = std::nullopt,
+            .remote = ossia::net::send_socket_configuration {{"127.0.0.1", shared}}
+          }}
+        }
+      );
+    proto->expose_to(std::move(o));
+  }
+
+  run = false;
+  handle.join();
+}
+
 #endif