ServerSocket shall not throw (EOL) on accept returning nullptr; Simpl…

…ify accept. Have `ServerSocket::accept` determine whether to `Util::forcedExit(EX_SOFTWARE)` on unrecoverable errors: - Exit application if `::accept` returns an unrecoverable `errno` (see below) - tolerate `Socket` ctor exceptions - tolerate `::accept` recoverable errors (see below) - Note, the following are `no throw` - `::close` - `::inet_ntop` Also reject exceeding external connections right within `ServerSocket::accept`, avoiding creation of `Socket` objects - hence saving resources. Recoverable `::accept` `errno` values following [accept(2)](https://www.man7.org/linux/man-pages/man2/accept.2.html) are - EAGAIN // == EWOULDBLOCK - ENETDOWN // man accept(2): to be treated like EAGAIN - EPROTO // man accept(2): to be treated like EAGAIN - ENOPROTOOPT // man accept(2): to be treated like EAGAIN - EHOSTDOWN // man accept(2): to be treated like EAGAIN - ENONET // man accept(2): to be treated like EAGAIN (undef on BSD, Apple, ..) - EHOSTUNREACH // man accept(2): to be treated like EAGAIN - EOPNOTSUPP // man accept(2): to be treated like EAGAIN - ENETUNREACH // man accept(2): to be treated like EAGAIN - ECONNABORTED // OK - ETIMEDOUT // OK, should not happen due ppoll We also allow retrying on temporary EMFILE, ENFILE, ENOBUFS and ENOMEM errors, which may also be candidates for later fine grained control in case of resource degradation. Unit testing hooks for failure injection is provided via - UnitWSD::simulateExternalAcceptError() - UnitWSD::simulateExternalSocketCtorException() and utilized in - test/UnitServerSocketAcceptFailure1.cpp - test/UnitStreamSocketCtorFailure1.cpp Signed-off-by: Sven Göthel <[email protected]> Change-Id: I939df8e8462d2c29d19214a9b5fb70ad37dc7500
CollaboraOnline · Nov 13, 2024 · 82409cc · 82409cc
1 parent 558a4fd
commit 82409cc
Show file tree

Hide file tree

Showing 6 changed files with 433 additions and 45 deletions.
diff --git a/common/Unit.hpp b/common/Unit.hpp
@@ -503,6 +503,15 @@ class UnitWSD : public UnitBase
         return false;
     }
 
+    // ---------------- ServerSocket hooks ----------------
+    /// Simulate `::accept` errors for external `ServerSocket::accept`. Implement unrecoverable errors by throwing an exception.
+    virtual bool simulateExternalAcceptError()
+    {
+        return false;
+    }
+    /// Simulate exceptions during `StreamSocket` constructor for external `ServerSocket::accept`.
+    virtual void simulateExternalSocketCtorException(std::shared_ptr<Socket>& /*socket*/) { }
+
     // ---------------- TileCache hooks ----------------
     /// Called before the lookupTile call returns. Should always be called to fire events.
     virtual void lookupTile(int part, int mode, int width, int height, int tilePosX, int tilePosY,

diff --git a/net/ServerSocket.hpp b/net/ServerSocket.hpp
@@ -29,6 +29,8 @@ class SocketFactory
 class ServerSocket : public Socket
 {
 public:
+    static bool isUnrecoverableAcceptError(const int cause);
+
     ServerSocket(Socket::Type type,
                  std::chrono::steady_clock::time_point creationTime,
                  SocketPoll& clientPoller, std::shared_ptr<SocketFactory> sockFactory) :
@@ -104,18 +106,11 @@ class ServerSocket : public Socket
         if (events & POLLIN)
         {
             std::shared_ptr<Socket> clientSocket = accept();
-            if (!clientSocket)
-            {
-                const std::string msg = "Failed to accept. (errno: ";
-                throw std::runtime_error(msg + std::strerror(errno) + ')');
-            }
-            const size_t extConnCount = StreamSocket::getExternalConnectionCount();
-            if( 0 == net::Defaults.maxExtConnections || extConnCount <= net::Defaults.maxExtConnections )
+            if (clientSocket)
             {
-                LOG_TRC("Accepted client #" << clientSocket->getFD());
+                LOGA_TRC(Socket, "Accepted client #" << clientSocket->getFD() << ", " << *clientSocket);
                 _clientPoller.insertNewSocket(std::move(clientSocket));
-            } else
-                LOG_WRN("Limiter rejected extConn[" << extConnCount << "/" << net::Defaults.maxExtConnections << "]: " << *clientSocket);
+            }
         }
     }
 

diff --git a/net/Socket.cpp b/net/Socket.cpp
@@ -15,6 +15,7 @@
 #include "TraceEvent.hpp"
 #include "Util.hpp"
 
+#include <cerrno>
 #include <chrono>
 #include <cstring>
 #include <cctype>
@@ -26,6 +27,7 @@
 #include <cstdio>
 #include <string>
 #include <unistd.h>
+#include <sysexits.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/un.h>
@@ -1149,63 +1151,115 @@ bool ServerSocket::bind([[maybe_unused]] Type type, [[maybe_unused]] int port)
 #endif
 }
 
+bool ServerSocket::isUnrecoverableAcceptError(const int cause)
+{
+    constexpr const char * messagePrefix = "Failed to accept. (errno: ";
+    switch(cause)
+    {
+        case EINTR:
+        case EAGAIN:        // == EWOULDBLOCK
+        case ENETDOWN:
+        case EPROTO:
+        case ENOPROTOOPT:
+        case EHOSTDOWN:
+#ifdef ENONET
+        case ENONET:
+#endif
+        case EHOSTUNREACH:
+        case EOPNOTSUPP:
+        case ENETUNREACH:
+        case ECONNABORTED:
+        case ETIMEDOUT:
+        case EMFILE:
+        case ENFILE:
+        case ENOMEM:
+        case ENOBUFS:
+        {
+            LOG_DBG(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
+            return false;
+        }
+        default:
+        {
+            LOG_FTL(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
+            return true;
+        }
+    }
+}
+
 std::shared_ptr<Socket> ServerSocket::accept()
 {
     // Accept a connection (if any) and set it to non-blocking.
     // There still need the client's address to filter request from POST(call from REST) here.
 #if !MOBILEAPP
     assert(_type != Socket::Type::Unix);
 
+    UnitWSD* const unitWsd = UnitWSD::isUnitTesting() ? &UnitWSD::get() : nullptr;
+    if (unitWsd && unitWsd->simulateExternalAcceptError())
+        return nullptr; // Recoverable error, ignore to retry
+
     struct sockaddr_in6 clientInfo;
     socklen_t addrlen = sizeof(clientInfo);
     const int rc = ::accept4(getFD(), (struct sockaddr *)&clientInfo, &addrlen, SOCK_NONBLOCK | SOCK_CLOEXEC);
 #else
     const int rc = fakeSocketAccept4(getFD());
 #endif
-    LOG_TRC("Accepted socket #" << rc << ", creating socket object.");
-    try
+    if (rc < 0)
     {
-        // Create a socket object using the factory.
-        if (rc != -1)
-        {
+        if (isUnrecoverableAcceptError(errno))
+            Util::forcedExit(EX_SOFTWARE);
+        return nullptr;
+    }
+    LOG_TRC("Accepted socket #" << rc << ", creating socket object.");
+
 #if !MOBILEAPP
-            char addrstr[INET6_ADDRSTRLEN];
+    char addrstr[INET6_ADDRSTRLEN];
 
-            Socket::Type type;
-            const void *inAddr;
-            if (clientInfo.sin6_family == AF_INET)
-            {
-                struct sockaddr_in *ipv4 = (struct sockaddr_in *)&clientInfo;
-                inAddr = &(ipv4->sin_addr);
-                type = Socket::Type::IPv4;
-            }
-            else
-            {
-                struct sockaddr_in6 *ipv6 = &clientInfo;
-                inAddr = &(ipv6->sin6_addr);
-                type = Socket::Type::IPv6;
-            }
+    Socket::Type type;
+    const void *inAddr;
+    if (clientInfo.sin6_family == AF_INET)
+    {
+        struct sockaddr_in *ipv4 = (struct sockaddr_in *)&clientInfo;
+        inAddr = &(ipv4->sin_addr);
+        type = Socket::Type::IPv4;
+    }
+    else
+    {
+        struct sockaddr_in6 *ipv6 = &clientInfo;
+        inAddr = &(ipv6->sin6_addr);
+        type = Socket::Type::IPv6;
+    }
+    ::inet_ntop(clientInfo.sin6_family, inAddr, addrstr, sizeof(addrstr));
 
-            std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, type);
+    const size_t extConnCount = StreamSocket::getExternalConnectionCount();
+    if (net::Defaults.maxExtConnections > 0 && extConnCount >= net::Defaults.maxExtConnections)
+    {
+        LOG_WRN("Limiter rejected extConn[" << extConnCount << "/" << net::Defaults.maxExtConnections << "]: #"
+                << rc << " has family "
+                << clientInfo.sin6_family << ", address " << addrstr << ":" << clientInfo.sin6_port);
+        ::close(rc);
+        return nullptr;
+    }
+    try
+    {
+        // Create a socket object using the factory.
+        std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, type);
+        if (unitWsd)
+            unitWsd->simulateExternalSocketCtorException(_socket);
 
-            ::inet_ntop(clientInfo.sin6_family, inAddr, addrstr, sizeof(addrstr));
-            _socket->setClientAddress(addrstr, clientInfo.sin6_port);
+        _socket->setClientAddress(addrstr, clientInfo.sin6_port);
 
-            LOG_TRC("Accepted socket #" << _socket->getFD() << " has family "
-                                        << clientInfo.sin6_family << ", " << *_socket);
-#else
-            std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, Socket::Type::Unix);
-#endif
-            return _socket;
-        }
-        return std::shared_ptr<Socket>(nullptr);
+        LOG_TRC("Accepted socket #" << _socket->getFD() << " has family "
+                                    << clientInfo.sin6_family << ", " << *_socket);
+        return _socket;
     }
     catch (const std::exception& ex)
     {
         LOG_ERR("Failed to create client socket #" << rc << ". Error: " << ex.what());
     }
-
     return nullptr;
+#else
+    return createSocketFromAccept(rc, Socket::Type::Unix);
+#endif
 }
 
 #if !MOBILEAPP
@@ -1251,11 +1305,15 @@ bool Socket::isLocal() const
 std::shared_ptr<Socket> LocalServerSocket::accept()
 {
     const int rc = ::accept4(getFD(), nullptr, nullptr, SOCK_NONBLOCK | SOCK_CLOEXEC);
+    if (rc < 0)
+    {
+        if (isUnrecoverableAcceptError(errno))
+            Util::forcedExit(EX_SOFTWARE);
+        return nullptr;
+    }
     try
     {
         LOG_DBG("Accepted prisoner socket #" << rc << ", creating socket object.");
-        if (rc < 0)
-            return std::shared_ptr<Socket>(nullptr);
 
         std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, Socket::Type::Unix);
         // Sanity check this incoming socket
@@ -1305,8 +1363,8 @@ std::shared_ptr<Socket> LocalServerSocket::accept()
     catch (const std::exception& ex)
     {
         LOG_ERR("Failed to create client socket #" << rc << ". Error: " << ex.what());
-        return std::shared_ptr<Socket>(nullptr);
     }
+    return nullptr;
 }
 
 /// Returns true on success only.

diff --git a/test/Makefile.am b/test/Makefile.am
@@ -94,6 +94,8 @@ all_la_unit_tests = \
 	unit-timeout_inactive.la \
 	unit-timeout_conn.la \
 	unit-timeout_none.la \
+	unit-serversock_accept1.la \
+	unit-streamsock_ctor1.la \
 	unit-base.la
 #	unit-admin.la
 #	unit-tilecache.la # Empty test.
@@ -232,6 +234,10 @@ unit_timeout_conn_la_SOURCES = UnitTimeoutConnections.cpp
 unit_timeout_conn_la_LIBADD = $(CPPUNIT_LIBS)
 unit_timeout_none_la_SOURCES = UnitTimeoutNone.cpp
 unit_timeout_none_la_LIBADD = $(CPPUNIT_LIBS)
+unit_serversock_accept1_la_SOURCES = UnitServerSocketAcceptFailure1.cpp
+unit_serversock_accept1_la_LIBADD = $(CPPUNIT_LIBS)
+unit_streamsock_ctor1_la_SOURCES = UnitStreamSocketCtorFailure1.cpp
+unit_streamsock_ctor1_la_LIBADD = $(CPPUNIT_LIBS)
 unit_prefork_la_SOURCES = UnitPrefork.cpp
 unit_prefork_la_LIBADD = $(CPPUNIT_LIBS)
 unit_storage_la_SOURCES = UnitStorage.cpp