Fix use after free

Change-Id: Id42e7514a0d29b91df25b28964481b4e66ad95cf

diff --git a/src/service/sockets/SocketManager.cpp b/src/service/sockets/SocketManager.cpp
index 64ee2f21be245557ddaa56bd87066d0d115ddb60..86afcc0aa7f070785fad46362d02948b36c7fc7f 100644 (file)
--- a/src/service/sockets/SocketManager.cpp
+++ b/src/service/sockets/SocketManager.cpp
@@ -252,15 +252,18 @@ void SocketManager::mainLoop() {
         // Some sockets might have been marked readable again and may already have serialized
         // requests in the buffer.
         for (int fd : m_fdsToCheckForReadButNotProcessedRequests) {
-            switch (handleRead(fd, RawBuffer{})) {
-            case HandleReadResult::NEED_MORE_DATA:
-            case HandleReadResult::SUSPENDED_AND_MORE_REQUESTS_MAY_BE_IN_THE_BUFFER:
-                break;
-            case HandleReadResult::ERROR:
-                // We assume we can drop already generated but not yet sent responses to previous
-                // requests.
-                closeSocket(fd);
-                break;
+            // fd might become closed due to hangup event for it or write error.
+            if (isFdUsed(fd)) {
+                switch (handleRead(fd, RawBuffer{})) {
+                case HandleReadResult::NEED_MORE_DATA:
+                case HandleReadResult::SUSPENDED_AND_MORE_REQUESTS_MAY_BE_IN_THE_BUFFER:
+                    break;
+                case HandleReadResult::ERROR:
+                    // We assume we can drop already generated but not yet sent responses to
+                    // previous requests.
+                    closeSocket(fd);
+                    break;
+                }
             }
         }
         m_fdsToCheckForReadButNotProcessedRequests.clear();
@@ -281,12 +284,13 @@ void SocketManager::mainLoop() {
         // will be used somewhere else in this function.
         if (m_needToDisconnectAllClients.exchange(false)) {
             LOGD("SocketManager disconnecting all clients");
+            // m_fds.size() may change during iteration of the loop (closeSocket() calls
+            // shrinkFds())
             for (size_t fd = 0; fd < m_fds.size(); ++fd) {
-                const auto& desc = m_fds[fd];
+                const auto& desc = m_fds[fd]; // safe as fd < m_fds.size()
                 if (desc.isUsed() && desc.isClient() && !desc.isListen())
                     closeSocket(fd);
             }
-            shrinkFds();
         }
     }
 }
@@ -308,8 +312,8 @@ SocketManager::handleNonReadOnlyRequestResults() {
             auto& res = std::get<NonReadOnlyRequestResult>(resV);
             LOGD("main thread: handling NonReadOnlyRequestResult for socket fd [%i] with generation"
                  " [%i]", res.socketFd, res.socketFdGeneration);
-            auto& desc = m_fds[res.socketFd];
-            if (desc.isUsed() && desc.getGeneration() == res.socketFdGeneration) {
+            if (isFdUsed(res.socketFd) &&
+                m_fds[res.socketFd].getGeneration() == res.socketFdGeneration) {
                 // Descriptor was not closed and was not reused.
                 // There may be some requests from the socket that are already read into
                 // the buffer but not processed. Schedule reading them.
@@ -330,6 +334,8 @@ SocketManager::ReadingResult SocketManager::readyForRead(int fd) {
         LOGD("SocketManager readyForRead on fd [%d] end", fd);
     }};
 
+    assert(isFdUsed(fd));
+
     if (fd == m_nonReadOnlyRequestResultsNumEventFd) {
         LOGD("SocketManager m_nonReadOnlyRequestResultsNumEventFd is ready for read");
         switch (handleNonReadOnlyRequestResults()) {
@@ -339,8 +345,7 @@ SocketManager::ReadingResult SocketManager::readyForRead(int fd) {
         }
     }
 
-    auto &desc = m_fds[fd];
-    if (desc.isListen()) {
+    if (m_fds[fd].isListen()) {
         readyForAccept(fd);
         return ReadingResult::OK;
     }
@@ -386,8 +391,9 @@ SocketManager::WritingResult SocketManager::readyForWrite(int fd) {
         LOGD("SocketManager readyForWrite on fd [%d] end", fd);
     }};
 
-    auto &desc = m_fds[fd];
-    auto &buffer = desc.prepareWriteBuffer();
+    assert(isFdUsed(fd));
+
+    auto &buffer = m_fds[fd].prepareWriteBuffer();
     size_t size = buffer.size();
     ssize_t result = send(fd, buffer.data(), size, MSG_NOSIGNAL);
     if (result == -1) {
@@ -417,6 +423,8 @@ void SocketManager::readyForAccept(int fd) {
         LOGD("SocketManager readyForAccept on fd [%d] end", fd);
     }};
 
+    assert(isFdUsed(fd));
+
     if (fdsUsageIsHigh()) {
         LOGD("SocketManager readyForAccept on fd [%d]: high memory usage -> stop listening", fd);
         m_listenSocketsDisabledBecauseOfHighFdUsage.emplace(fd);
@@ -451,7 +459,7 @@ void SocketManager::readyForAccept(int fd) {
             logStats();
 #endif
 
-    auto &desc = createDescriptorWatchedForRead(clientFd, m_fds[fd].isClient());
+    auto &desc = createDescriptorWatchedForRead(clientFd, m_fds[fd].isClient()); // changes m_fds
     desc.setListen(false);
     desc.setProtocol(m_fds[fd].protocol()->clone());
     desc.setReadOnlyProtocol(m_fds[fd].protocol()->clone());
@@ -463,6 +471,8 @@ void SocketManager::closeSocket(int fd) {
         LOGD("SocketManager closeSocket fd [%d] end", fd);
     }};
 
+    assert(isFdUsed(fd));
+
     m_epoll.stopWatching(fd);
 
     Descriptor &desc = m_fds[fd];
@@ -496,6 +506,7 @@ SocketManager::HandleReadResult SocketManager::handleRead(int fd, const RawBuffe
         LOGD("SocketManager handleRead on fd [%d] end", fd);
     }};
 
+    assert(isFdUsed(fd));
     auto &desc = m_fds[fd];
     desc.pushReadBuffer(readBuffer);
 

diff --git a/src/service/sockets/SocketManager.h b/src/service/sockets/SocketManager.h
index 4f4a9549c68b1ebcdbb26654f2a720835863cd6c..950db119c6ed745070afa5f20ae4a34975962b6b 100644 (file)
--- a/src/service/sockets/SocketManager.h
+++ b/src/service/sockets/SocketManager.h
@@ -211,6 +211,10 @@ private:
         return m_stats.openConnections() > std::max<size_t>( m_openFdsLimit, 70) - 50;
     }
 
+    [[nodiscard]] bool isFdUsed(int fd) const noexcept {
+        return static_cast<size_t>(fd) < m_fds.size() && m_fds[fd].isUsed();
+    }
+
     void init();
     void mainLoop();