From: Davin McCall Date: Tue, 14 Jun 2016 22:01:22 +0000 (+0100) Subject: Make control socket handling safer and fix an inverted logic bug. X-Git-Tag: v0.03~20 X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=8d24cdee2fe633242652eea3fd2ac14623d0a068;p=oweals%2Fdinit.git 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 825e8f3..f716f39 100644 --- 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 &&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 &&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 260c10d..5f420b2 100644 --- 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 &&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(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; } }