IOListener is deleted twice

[jrenken]

The IOListener is owned by iodriver_base::Task but is deleted in both the task and the driver. Removing it from the driver seems to prevent a possible crash.

Driver::~Driver()
{
    delete[] internal_buffer;
    delete m_stream;
//    for (set<IOListener*>::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
//        delete *it;
}

[doudou]

Would make IMO more sense to transfer ownership to the driver. What do you think ?

[jrenken]

I agree, but the task needs still needs to keep a reference to the listener if it wants to remove it from the driver. Maybe using shared_ptr is more safe?

diff --git a/src/Driver.cpp b/src/Driver.cpp
index a15e59b..c803c77 100644
--- a/src/Driver.cpp
+++ b/src/Driver.cpp
@@ -98,4 +98,2 @@ Driver::~Driver()
     delete m_stream;
-    for (set<IOListener*>::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
-        delete *it;
 }
@@ -108,3 +106,3 @@ void Driver::setMainStream(IOStream* stream)


-void Driver::addListener(IOListener* listener)
+void Driver::addListener(boost::shared_ptr<IOListener> listener)
 {
@@ -113,3 +111,3 @@ void Driver::addListener(IOListener* listener)

-void Driver::removeListener(IOListener* listener)
+void Driver::removeListener(boost::shared_ptr<IOListener> listener)
 {
@@ -623,3 +621,3 @@ pair<int, bool> Driver::readPacketInternal(uint8_t* buffer, int out_buffer_size)
         if (c > 0) {
-            for (set<IOListener*>::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
+            for (set<boost::shared_ptr<IOListener> >::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
                 (*it)->readData(internal_buffer + internal_buffer_size, c);
@@ -779,3 +777,3 @@ bool Driver::writePacket(uint8_t const* buffer, int buffer_size, int timeout)
         int c = m_stream->write(buffer + written, buffer_size - written);
-        for (set<IOListener*>::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
+        for (set<boost::shared_ptr<IOListener> >::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
             (*it)->writeData(buffer + written, c);
diff --git a/src/Driver.hpp b/src/Driver.hpp
index 8c79ab7..5be337d 100644
--- a/src/Driver.hpp
+++ b/src/Driver.hpp
@@ -12,3 +12,3 @@
 #include <set>
-
+#include <boost/shared_ptr.hpp>
 struct addrinfo;
@@ -91,3 +91,3 @@ protected:
      */
-    std::set<IOListener*> m_listeners;
+    std::set<boost::shared_ptr<IOListener> > m_listeners;

@@ -439,3 +439,3 @@ public:
      */
-    void addListener(IOListener* stream);
+    void addListener(boost::shared_ptr<IOListener> listener);

@@ -444,3 +444,3 @@ public:
      */
-    void removeListener(IOListener* stream);
+    void removeListener(boost::shared_ptr<IOListener> listener);
jrenken@elektron-mobil:~/marum/rock/hrov-dev/drivers/iodrivers_base$ git diff -U1 > d.diff
jrenken@elektron-mobil:~/marum/rock/hrov-dev/drivers/iodrivers_base$ cat d.diff 
diff --git a/src/Driver.cpp b/src/Driver.cpp
index a15e59b..c803c77 100644
--- a/src/Driver.cpp
+++ b/src/Driver.cpp
@@ -98,4 +98,2 @@ Driver::~Driver()
     delete m_stream;
-    for (set<IOListener*>::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
-        delete *it;
 }
@@ -108,3 +106,3 @@ void Driver::setMainStream(IOStream* stream)

-void Driver::addListener(IOListener* listener)
+void Driver::addListener(boost::shared_ptr<IOListener> listener)
 {
@@ -113,3 +111,3 @@ void Driver::addListener(IOListener* listener)

-void Driver::removeListener(IOListener* listener)
+void Driver::removeListener(boost::shared_ptr<IOListener> listener)
 {
@@ -623,3 +621,3 @@ pair<int, bool> Driver::readPacketInternal(uint8_t* buffer, int out_buffer_size)
         if (c > 0) {
-            for (set<IOListener*>::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
+            for (set<boost::shared_ptr<IOListener> >::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
                 (*it)->readData(internal_buffer + internal_buffer_size, c);
@@ -779,3 +777,3 @@ bool Driver::writePacket(uint8_t const* buffer, int buffer_size, int timeout)
         int c = m_stream->write(buffer + written, buffer_size - written);
-        for (set<IOListener*>::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
+        for (set<boost::shared_ptr<IOListener> >::iterator it = m_listeners.begin(); it != m_listeners.end(); ++it)
             (*it)->writeData(buffer + written, c);
diff --git a/src/Driver.hpp b/src/Driver.hpp
index 8c79ab7..5be337d 100644
--- a/src/Driver.hpp
+++ b/src/Driver.hpp
@@ -12,3 +12,3 @@
 #include <set>
-
+#include <boost/shared_ptr.hpp>
 struct addrinfo;
@@ -91,3 +91,3 @@ protected:
      */
-    std::set<IOListener*> m_listeners;
+    std::set<boost::shared_ptr<IOListener> > m_listeners;

@@ -439,3 +439,3 @@ public:
      */
-    void addListener(IOListener* stream);
+    void addListener(boost::shared_ptr<IOListener> listener);

@@ -444,3 +444,3 @@ public:
      */
-    void removeListener(IOListener* stream);
+    void removeListener(boost::shared_ptr<IOListener> listener);

and

diff --git a/tasks/Task.cpp b/tasks/Task.cpp
index ebb8d69..a0334a7 100644
--- a/tasks/Task.cpp
+++ b/tasks/Task.cpp
@@ -30,3 +30,2 @@ Task::~Task()
 {
-    delete mListener;
 }
diff --git a/tasks/Task.hpp b/tasks/Task.hpp
index 0e791cc..acd04af 100644
--- a/tasks/Task.hpp
+++ b/tasks/Task.hpp
@@ -6,2 +6,3 @@
 #include "iodrivers_base/TaskBase.hpp"
+#include <boost/shared_ptr.hpp>

@@ -22,3 +23,3 @@ namespace iodrivers_base {
         /** IOListener object that outputs the driver's communication to ports */
-        PortListener* mListener;
+        boost::shared_ptr<PortListener> mListener;

[jmachowinski]

Sounds good,
please create a typedef like
PortListenerShrPtr
this will relax the transition to std::shared_ptr that will happen in
the not to far away future.

[doudou]

I'm very much against it. Making it a shared pointer removes the double-free, but would not remove the "use listener that the driver is not using anymore" which makes no sense.

iodrivers_base should take ownership of the listener, make it clear in the documentation and do not delete the listener from the task.