Make control socket handling safer and fix an inverted logic bug.

queuePacket() now no longer deletes the connection, meaning its
use the service listener is now safe.

diff --git a/src/control.cc b/src/control.cc
index 825e8f386fbc6456d59c7f837a91a2c6e7eb1d2a..f716f398903c3411e89e26f2496c2dd8820e4e32 100644 (file)
--- a/src/control.cc
+++ b/src/control.cc
@@ -277,12 +277,10 @@ bool ControlConn::queuePacket(const char *pkt, unsigned size) noexcept
         int wr = write(iob.fd, pkt, size);
         if (wr == -1) {
             if (errno == EPIPE) {
-                delete this;
                 return false;
             }
             if (errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR) {
                 // TODO log error
-                delete this;
                 return false;
             }
             // EAGAIN etc: fall through to below
@@ -312,7 +310,6 @@ bool ControlConn::queuePacket(const char *pkt, unsigned size) noexcept
             // We can't send out-of-memory response as we already wrote as much as we
             // could above. Neither can we later send the response since we have currently
             // sent an incomplete packet. All we can do is close the connection.
-            delete this;
             return false;
         }
         else {
@@ -335,12 +332,10 @@ bool ControlConn::queuePacket(std::vector<char> &&pkt) noexcept
         int wr = write(iob.fd, pkt.data(), pkt.size());
         if (wr == -1) {
             if (errno == EPIPE) {
-                delete this;
                 return false;
             }
             if (errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR) {
                 // TODO log error
-                delete this;
                 return false;
             }
             // EAGAIN etc: fall through to below
@@ -368,7 +363,6 @@ bool ControlConn::queuePacket(std::vector<char> &&pkt) noexcept
             // We can't send out-of-memory response as we already wrote as much as we
             // could above. Neither can we later send the response since we have currently
             // sent an incomplete packet. All we can do is close the connection.
-            delete this;
             return false;
         }
         else {
@@ -394,21 +388,19 @@ bool ControlConn::dataReady() noexcept
     if (r == -1) {
         if (errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR) {
             // TODO log error
-            delete this;
             return true;
         }
         return false;
     }
     
     if (r == 0) {
-        delete this;
         return true;
     }
     
     // complete packet?
     if (rbuf.get_length() >= chklen) {
         try {
-            return processPacket();
+            return !processPacket();
         }
         catch (std::bad_alloc &baexc) {
             doOomClose();
@@ -438,7 +430,6 @@ bool ControlConn::sendData() noexcept
             char oomBuf[] = { DINIT_RP_OOM };
             write(iob.fd, oomBuf, 1);
         }
-        delete this;
         return true;
     }
     
@@ -448,7 +439,6 @@ bool ControlConn::sendData() noexcept
     if (written == -1) {
         if (errno == EPIPE) {
             // read end closed
-            delete this;
             return true;
         }
         else if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) {
@@ -456,7 +446,6 @@ bool ControlConn::sendData() noexcept
         }
         else {
             log(LogLevel::ERROR, "Error writing to control connection: ", strerror(errno));
-            delete this;
             return true;
         }
         return false;
@@ -472,7 +461,6 @@ bool ControlConn::sendData() noexcept
                 iob.setWatchFlags(in_events);
             }
             else {
-                delete this;
                 return true;
             }
         }

diff --git a/src/control.h b/src/control.h
index 260c10d107c51a979ec7bee27dd5e2d94d8dc66d..5f420b203d87c59ec8c5ccf5453c625e40d8fb6e 100644 (file)
--- a/src/control.h
+++ b/src/control.h
@@ -117,18 +117,21 @@ class ControlConn : private ServiceListener
     unsigned outpkt_index = 0;
     
     // Queue a packet to be sent
-    //  Returns:  false if the packet could not be queued and the connection has been
-    //              destroyed;
+    //  Returns:  false if the packet could not be queued and a suitable error packet
+    //              could not be sent/queued (the connection should be closed);
     //            true (with bad_conn_close == false) if the packet was successfully
     //              queued;
     //            true (with bad_conn_close == true) if the packet was not successfully
-    //              queued.
+    //              queued (but a suitable error packate has been queued).
     // The in/out watch enabled state will also be set appropriately.
     bool queuePacket(vector<char> &&v) noexcept;
     bool queuePacket(const char *pkt, unsigned size) noexcept;
 
-    // Process a packet. Can cause the ControlConn to be deleted iff there are no
-    // outgoing packets queued (returns false).
+    // Process a packet.
+    //  Returns:  true (with bad_conn_close == false) if successful
+    //            true (with bad_conn_close == true) if an error packet was queued
+    //            false if an error occurred but no error packet could be queued
+    //                (connection should be closed).
     // Throws:
     //    std::bad_alloc - if an out-of-memory condition prevents processing
     bool processPacket();
@@ -142,8 +145,8 @@ class ControlConn : private ServiceListener
     // Process an UNPINSERVICE packet. May throw std::bad_alloc.
     bool processUnpinService();
 
-    // Notify that data is ready to be read from the socket. Returns true if the connection was
-    // deleted.
+    // Notify that data is ready to be read from the socket. Returns true if the connection should
+    // be closed.
     bool dataReady() noexcept;
     
     bool sendData() noexcept;
@@ -170,6 +173,8 @@ class ControlConn : private ServiceListener
     }
     
     // Process service event broadcast.
+    // Note that this can potentially be called during packet processing (upon issuing
+    // service start/stop orders etc).
     void serviceEvent(ServiceRecord * service, ServiceEvent event) noexcept final override
     {
         // For each service handle corresponding to the event, send an information packet.
@@ -219,12 +224,13 @@ static dasync::Rearm control_conn_cb(EventLoop_t * loop, ControlConnWatcher * wa
     ControlConn *conn = reinterpret_cast<ControlConn *>(cc_addr);
     if (revents & in_events) {
         if (conn->dataReady()) {
-            // ControlConn was deleted
+            delete conn;
             return Rearm::REMOVED;
         }
     }
     if (revents & out_events) {
         if (conn->sendData()) {
+            delete conn;
             return Rearm::REMOVED;
         }
     }