Problem: Active poller double add mutates handler

Solution: Check if socket already added before storing.

Author: gummif

tests/active_poller.cpp

@@ -14,9 +14,9 @@ TEST_CASE("create destroy", "[active_poller]")
 }
 
 static_assert(!std::is_copy_constructible<zmq::active_poller_t>::value,
-              "active_active_poller_t should not be copy-constructible");
+              "active_poller_t should not be copy-constructible");
 static_assert(!std::is_copy_assignable<zmq::active_poller_t>::value,
-              "active_active_poller_t should not be copy-assignable");
+              "active_poller_t should not be copy-assignable");
 
 static const zmq::active_poller_t::handler_type no_op_handler =
   [](zmq::event_flags) {};
@@ -115,12 +115,19 @@ TEST_CASE("add handler invalid events type", "[active_poller]")
 
 TEST_CASE("add handler twice throws", "[active_poller]")
 {
-    zmq::context_t context;
-    zmq::socket_t socket{context, zmq::socket_type::router};
+    common_server_client_setup s;
+
+    CHECK(s.client.send(zmq::message_t{}, zmq::send_flags::none));
+
     zmq::active_poller_t active_poller;
-    active_poller.add(socket, zmq::event_flags::pollin, no_op_handler);
+    bool message_received = false;
+    active_poller.add(
+      s.server, zmq::event_flags::pollin,
+      [&message_received](zmq::event_flags) { message_received = true; });
     CHECK_THROWS_ZMQ_ERROR(
-      EINVAL, active_poller.add(socket, zmq::event_flags::pollin, no_op_handler));
+      EINVAL, active_poller.add(s.server, zmq::event_flags::pollin, no_op_handler));
+    CHECK(1 == active_poller.wait(std::chrono::milliseconds{-1}));
+    CHECK(message_received); // handler unmodified
 }
 
 TEST_CASE("wait with no handlers throws", "[active_poller]")

zmq_addon.hpp

@@ -675,20 +675,17 @@ class active_poller_t
     {
         if (!handler)
             throw std::invalid_argument("null handler in active_poller_t::add");
-        auto it = decltype(handlers)::iterator{};
-        auto inserted = bool{};
-        std::tie(it, inserted) = handlers.emplace(
+        auto ret = handlers.emplace(
           socket, std::make_shared<handler_type>(std::move(handler)));
+        if (!ret.second)
+            throw error_t(EINVAL); // already added
         try {
-            base_poller.add(socket, events,
-                            inserted && *(it->second) ? it->second.get() : nullptr);
-            need_rebuild |= inserted;
+            base_poller.add(socket, events, ret.first->second.get());
+            need_rebuild = true;
         }
-        catch (const zmq::error_t &) {
+        catch (...) {
             // rollback
-            if (inserted) {
-                handlers.erase(socket);
-            }
+            handlers.erase(socket);
             throw;
         }
     }
@@ -720,8 +717,8 @@ class active_poller_t
         std::for_each(poller_events.begin(),
                       poller_events.begin() + static_cast<ptrdiff_t>(count),
                       [](decltype(base_poller)::event_type &event) {
-                          if (event.user_data != nullptr)
-                              (*event.user_data)(event.events);
+                          assert(event.user_data != nullptr);
+                          (*event.user_data)(event.events);
                       });
         return count;
     }