Skip to content

Commit 98ae285

Browse files
author
Sven Göthel
committed
ServerSocket shall not throw (EOL) on accept returning nullptr; Simplify 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 - 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 - EMFILE // Temporary: No free file descriptors left (per process) - ENFILE // Temporary: No free file descriptors left (system) - ENOBUFS // Temporary: No free socket memory left (system) - ENOMEM // Temporary: No free socket memory left (system) Signed-off-by: Sven Göthel <[email protected]> Change-Id: I939df8e8462d2c29d19214a9b5fb70ad37dc7500
1 parent a21425b commit 98ae285

7 files changed

+493
-49
lines changed

‎net/NetUtil.hpp

+12
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,12 @@ class ProtocolHandlerInterface;
2626
struct addrinfo;
2727
struct sockaddr;
2828

29+
#if !MOBILEAPP && ENABLE_DEBUG
30+
#define TEST_SERVERSOCKET_FAULT_INJECTION 1
31+
#else
32+
#define TEST_SERVERSOCKET_FAULT_INJECTION 0
33+
#endif
34+
2935
namespace net
3036
{
3137

@@ -37,6 +43,12 @@ class DefaultValues
3743

3844
/// Maximum number of concurrent external TCP connections. Zero disables instrument.
3945
size_t maxExtConnections;
46+
47+
#if TEST_SERVERSOCKET_FAULT_INJECTION
48+
size_t testExternalStreamSocketCtorFailureInterval; // recoverable error injection
49+
size_t testExternalServerSocketAcceptSimpleErrorInterval; // recoverable error injection
50+
size_t testExternalServerSocketAcceptFatalErrorInterval; // fatal error injection
51+
#endif
4052
};
4153
extern DefaultValues Defaults;
4254

‎net/ServerSocket.hpp

+14-10
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,14 @@ class SocketFactory
2929
class ServerSocket : public Socket
3030
{
3131
public:
32+
/// Socket ::accept() recoverable retry errors.
33+
/// See [accept(2)](https://www.man7.org/linux/man-pages/man2/accept.2.html) errors leading to retry
34+
static bool isRetryAcceptError(const int cause);
35+
36+
/// Socket ::accept() recoverable errors, includes isSocketAcceptRetryError.
37+
/// See [accept(2)](https://www.man7.org/linux/man-pages/man2/accept.2.html) errors leading to retry
38+
static bool isRecoverableAcceptError(const int cause);
39+
3240
ServerSocket(Socket::Type type,
3341
std::chrono::steady_clock::time_point creationTime,
3442
SocketPoll& clientPoller, std::shared_ptr<SocketFactory> sockFactory) :
@@ -104,18 +112,11 @@ class ServerSocket : public Socket
104112
if (events & POLLIN)
105113
{
106114
std::shared_ptr<Socket> clientSocket = accept();
107-
if (!clientSocket)
108-
{
109-
const std::string msg = "Failed to accept. (errno: ";
110-
throw std::runtime_error(msg + std::strerror(errno) + ')');
111-
}
112-
const size_t extConnCount = StreamSocket::getExternalConnectionCount();
113-
if( 0 == net::Defaults.maxExtConnections || extConnCount <= net::Defaults.maxExtConnections )
115+
if (clientSocket)
114116
{
115-
LOG_TRC("Accepted client #" << clientSocket->getFD());
117+
LOGA_TRC(Socket, "Accepted client #" << clientSocket->getFD() << ", " << *clientSocket);
116118
_clientPoller.insertNewSocket(std::move(clientSocket));
117-
} else
118-
LOG_WRN("Limiter rejected extConn[" << extConnCount << "/" << net::Defaults.maxExtConnections << "]: " << *clientSocket);
119+
}
119120
}
120121
}
121122

@@ -132,6 +133,9 @@ class ServerSocket : public Socket
132133
#endif
133134
SocketPoll& _clientPoller;
134135
std::shared_ptr<SocketFactory> _sockFactory;
136+
#if TEST_SERVERSOCKET_FAULT_INJECTION
137+
static std::atomic<size_t> TestExternalServerSocketAcceptCount;
138+
#endif
135139
};
136140

137141
#if !MOBILEAPP

‎net/Socket.cpp

+155-39
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "TraceEvent.hpp"
1616
#include "Util.hpp"
1717

18+
#include <cerrno>
1819
#include <chrono>
1920
#include <cstring>
2021
#include <cctype>
@@ -26,6 +27,7 @@
2627
#include <cstdio>
2728
#include <string>
2829
#include <unistd.h>
30+
#include <sysexits.h>
2931
#include <sys/stat.h>
3032
#include <sys/types.h>
3133
#include <sys/un.h>
@@ -67,8 +69,19 @@ std::unique_ptr<Watchdog> SocketPoll::PollWatchdog;
6769

6870
std::atomic<size_t> StreamSocket::ExternalConnectionCount = 0;
6971

70-
net::DefaultValues net::Defaults = { .inactivityTimeout = std::chrono::seconds(3600),
71-
.maxExtConnections = 200000 /* arbitrary value to be resolved */ };
72+
net::DefaultValues net::Defaults = { .inactivityTimeout = std::chrono::seconds(3600)
73+
, .maxExtConnections = 0 /* unlimited default */
74+
#if !MOBILEAPP && ENABLE_DEBUG
75+
, .testExternalStreamSocketCtorFailureInterval = 0
76+
, .testExternalServerSocketAcceptSimpleErrorInterval = 0
77+
, .testExternalServerSocketAcceptFatalErrorInterval = 0
78+
#endif
79+
};
80+
81+
#if TEST_SERVERSOCKET_FAULT_INJECTION
82+
std::atomic<size_t> StreamSocket::TestExternalStreamSocketCount = 0;
83+
std::atomic<size_t> ServerSocket::TestExternalServerSocketAcceptCount = 0;
84+
#endif
7285

7386
#define SOCKET_ABSTRACT_UNIX_NAME "0coolwsd-"
7487

@@ -1145,63 +1158,156 @@ bool ServerSocket::bind([[maybe_unused]] Type type, [[maybe_unused]] int port)
11451158
#endif
11461159
}
11471160

1161+
bool ServerSocket::isRetryAcceptError(const int cause)
1162+
{
1163+
switch(cause)
1164+
{
1165+
case EINTR:
1166+
case EAGAIN: // == EWOULDBLOCK
1167+
case ENETDOWN: // man accept(2): to be treated like EAGAIN
1168+
case EPROTO: // man accept(2): to be treated like EAGAIN
1169+
case ENOPROTOOPT: // man accept(2): to be treated like EAGAIN
1170+
case EHOSTDOWN: // man accept(2): to be treated like EAGAIN
1171+
case ENONET: // man accept(2): to be treated like EAGAIN
1172+
case EHOSTUNREACH: // man accept(2): to be treated like EAGAIN
1173+
case EOPNOTSUPP: // man accept(2): to be treated like EAGAIN
1174+
case ENETUNREACH: // man accept(2): to be treated like EAGAIN
1175+
case ECONNABORTED: // OK
1176+
case ETIMEDOUT: // OK, should not happen due ppoll
1177+
return true;
1178+
default:
1179+
return false;
1180+
}
1181+
}
1182+
1183+
bool ServerSocket::isRecoverableAcceptError(const int cause)
1184+
{
1185+
switch(cause)
1186+
{
1187+
case EINTR:
1188+
case EAGAIN: // == EWOULDBLOCK
1189+
case ENETDOWN: // man accept(2): to be treated like EAGAIN
1190+
case EPROTO: // man accept(2): to be treated like EAGAIN
1191+
case ENOPROTOOPT: // man accept(2): to be treated like EAGAIN
1192+
case EHOSTDOWN: // man accept(2): to be treated like EAGAIN
1193+
case ENONET: // man accept(2): to be treated like EAGAIN
1194+
case EHOSTUNREACH: // man accept(2): to be treated like EAGAIN
1195+
case EOPNOTSUPP: // man accept(2): to be treated like EAGAIN
1196+
case ENETUNREACH: // man accept(2): to be treated like EAGAIN
1197+
case ECONNABORTED: // OK
1198+
case ETIMEDOUT: // OK, should not happen due ppoll
1199+
case EMFILE: // Temporary: No free file descriptors left (per process)
1200+
case ENFILE: // Temporary: No free file descriptors left (system)
1201+
case ENOBUFS: // Temporary: No free socket memory left (system)
1202+
case ENOMEM: // Temporary: No free socket memory left (system)
1203+
return true;
1204+
default:
1205+
return false;
1206+
}
1207+
}
1208+
11481209
std::shared_ptr<Socket> ServerSocket::accept()
11491210
{
11501211
// Accept a connection (if any) and set it to non-blocking.
11511212
// There still need the client's address to filter request from POST(call from REST) here.
1213+
int rc;
11521214
#if !MOBILEAPP
11531215
assert(_type != Socket::Type::Unix);
11541216

11551217
struct sockaddr_in6 clientInfo;
11561218
socklen_t addrlen = sizeof(clientInfo);
1157-
const int rc = ::accept4(getFD(), (struct sockaddr *)&clientInfo, &addrlen, SOCK_NONBLOCK | SOCK_CLOEXEC);
1219+
#if TEST_SERVERSOCKET_FAULT_INJECTION
1220+
const size_t acceptCount = ++TestExternalServerSocketAcceptCount;
1221+
bool injectedError = false;
1222+
if (net::Defaults.testExternalServerSocketAcceptSimpleErrorInterval > 0 &&
1223+
acceptCount % net::Defaults.testExternalServerSocketAcceptSimpleErrorInterval == 0)
1224+
{
1225+
rc = -1;
1226+
errno = EAGAIN; // recoverable error
1227+
injectedError = true;
1228+
LOG_DBG("Injecting recoverable accept failure: EAGAIN: " << acceptCount);
1229+
}
1230+
else if (net::Defaults.testExternalServerSocketAcceptFatalErrorInterval > 0 &&
1231+
acceptCount % net::Defaults.testExternalServerSocketAcceptFatalErrorInterval == 0)
1232+
{
1233+
rc = -1;
1234+
errno = EFAULT; // fatal error
1235+
injectedError = true;
1236+
LOG_DBG("Injecting fatal accept failure: EAGAIN: " << acceptCount);
1237+
}
1238+
else
1239+
#endif
1240+
rc = ::accept4(getFD(), (struct sockaddr *)&clientInfo, &addrlen, SOCK_NONBLOCK | SOCK_CLOEXEC);
11581241
#else
1159-
const int rc = fakeSocketAccept4(getFD());
1242+
rc = fakeSocketAccept4(getFD());
11601243
#endif
1161-
LOG_TRC("Accepted socket #" << rc << ", creating socket object.");
1162-
try
1244+
if (rc < 0)
11631245
{
1164-
// Create a socket object using the factory.
1165-
if (rc != -1)
1246+
const int cause = errno;
1247+
constexpr const char * messagePrefix = "Failed to accept. (errno: ";
1248+
if (!isRecoverableAcceptError(cause))
11661249
{
1167-
#if !MOBILEAPP
1168-
char addrstr[INET6_ADDRSTRLEN];
1169-
1170-
Socket::Type type;
1171-
const void *inAddr;
1172-
if (clientInfo.sin6_family == AF_INET)
1173-
{
1174-
struct sockaddr_in *ipv4 = (struct sockaddr_in *)&clientInfo;
1175-
inAddr = &(ipv4->sin_addr);
1176-
type = Socket::Type::IPv4;
1177-
}
1250+
// Unrecoverable error, forceExit instead of throw (closing `ServerSocket` and discard it from `AcceptPoll` during `SocketPoll::poll`)
1251+
LOG_FTL(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
1252+
#if TEST_SERVERSOCKET_FAULT_INJECTION
1253+
if (injectedError)
1254+
throw std::runtime_error(std::string(messagePrefix) + std::to_string(cause) + ", " + std::strerror(cause) + ')');
11781255
else
1179-
{
1180-
struct sockaddr_in6 *ipv6 = &clientInfo;
1181-
inAddr = &(ipv6->sin6_addr);
1182-
type = Socket::Type::IPv6;
1183-
}
1256+
#endif
1257+
Util::forcedExit(EX_SOFTWARE);
1258+
}
1259+
else // Recoverable error, ignore to retry
1260+
LOG_DBG(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
1261+
return nullptr;
1262+
}
1263+
LOG_TRC("Accepted socket #" << rc << ", creating socket object.");
11841264

1185-
std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, type);
1265+
#if !MOBILEAPP
1266+
char addrstr[INET6_ADDRSTRLEN];
11861267

1187-
::inet_ntop(clientInfo.sin6_family, inAddr, addrstr, sizeof(addrstr));
1188-
_socket->setClientAddress(addrstr, clientInfo.sin6_port);
1268+
Socket::Type type;
1269+
const void *inAddr;
1270+
if (clientInfo.sin6_family == AF_INET)
1271+
{
1272+
struct sockaddr_in *ipv4 = (struct sockaddr_in *)&clientInfo;
1273+
inAddr = &(ipv4->sin_addr);
1274+
type = Socket::Type::IPv4;
1275+
}
1276+
else
1277+
{
1278+
struct sockaddr_in6 *ipv6 = &clientInfo;
1279+
inAddr = &(ipv6->sin6_addr);
1280+
type = Socket::Type::IPv6;
1281+
}
1282+
::inet_ntop(clientInfo.sin6_family, inAddr, addrstr, sizeof(addrstr));
11891283

1190-
LOG_TRC("Accepted socket #" << _socket->getFD() << " has family "
1191-
<< clientInfo.sin6_family << ", " << *_socket);
1192-
#else
1193-
std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, Socket::Type::Unix);
1194-
#endif
1195-
return _socket;
1196-
}
1197-
return std::shared_ptr<Socket>(nullptr);
1284+
const size_t extConnCount = StreamSocket::getExternalConnectionCount();
1285+
if( 0 < net::Defaults.maxExtConnections && extConnCount >= net::Defaults.maxExtConnections )
1286+
{
1287+
LOG_WRN("Limiter rejected extConn[" << extConnCount << "/" << net::Defaults.maxExtConnections << "]: #"
1288+
<< rc << " has family "
1289+
<< clientInfo.sin6_family << ", address " << addrstr << ":" << clientInfo.sin6_port);
1290+
::close(rc);
1291+
return nullptr;
1292+
}
1293+
try
1294+
{
1295+
// Create a socket object using the factory.
1296+
std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, type);
1297+
_socket->setClientAddress(addrstr, clientInfo.sin6_port);
1298+
1299+
LOG_TRC("Accepted socket #" << _socket->getFD() << " has family "
1300+
<< clientInfo.sin6_family << ", " << *_socket);
1301+
return _socket;
11981302
}
11991303
catch (const std::exception& ex)
12001304
{
12011305
LOG_ERR("Failed to create client socket #" << rc << ". Error: " << ex.what());
12021306
}
1203-
12041307
return nullptr;
1308+
#else
1309+
return createSocketFromAccept(rc, Socket::Type::Unix);
1310+
#endif
12051311
}
12061312

12071313
#if !MOBILEAPP
@@ -1247,11 +1353,21 @@ bool Socket::isLocal() const
12471353
std::shared_ptr<Socket> LocalServerSocket::accept()
12481354
{
12491355
const int rc = ::accept4(getFD(), nullptr, nullptr, SOCK_NONBLOCK | SOCK_CLOEXEC);
1356+
if (rc < 0)
1357+
{
1358+
const int cause = errno;
1359+
if( cause != EINTR && cause != EAGAIN && cause != EWOULDBLOCK )
1360+
{
1361+
// Unrecoverable error, throw, which will close `ServerSocket` and discard it from `AcceptPoll` during `SocketPoll::poll`
1362+
constexpr const char * messagePrefix = "Failed to accept. (errno: ";
1363+
LOG_FTL(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
1364+
Util::forcedExit(EX_SOFTWARE);
1365+
}
1366+
return nullptr;
1367+
}
12501368
try
12511369
{
12521370
LOG_DBG("Accepted prisoner socket #" << rc << ", creating socket object.");
1253-
if (rc < 0)
1254-
return std::shared_ptr<Socket>(nullptr);
12551371

12561372
std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, Socket::Type::Unix);
12571373
// Sanity check this incoming socket
@@ -1301,8 +1417,8 @@ std::shared_ptr<Socket> LocalServerSocket::accept()
13011417
catch (const std::exception& ex)
13021418
{
13031419
LOG_ERR("Failed to create client socket #" << rc << ". Error: " << ex.what());
1304-
return std::shared_ptr<Socket>(nullptr);
13051420
}
1421+
return nullptr;
13061422
}
13071423

13081424
/// Returns true on success only.

‎net/Socket.hpp

+16
Original file line numberDiff line numberDiff line change
@@ -1056,6 +1056,19 @@ class StreamSocket : public Socket,
10561056
_readType(readType),
10571057
_inputProcessingEnabled(true)
10581058
{
1059+
#if TEST_SERVERSOCKET_FAULT_INJECTION
1060+
if (isExternalCountedConnection())
1061+
{
1062+
const size_t extStreamSocketCount = ++TestExternalStreamSocketCount;
1063+
if (net::Defaults.testExternalStreamSocketCtorFailureInterval > 0 &&
1064+
extStreamSocketCount% net::Defaults.testExternalStreamSocketCtorFailureInterval == 0)
1065+
{
1066+
LOG_DBG("Injecting recoverable StreamSocket ctor exception " << extStreamSocketCount
1067+
<< "/" << net::Defaults.testExternalStreamSocketCtorFailureInterval);
1068+
throw std::runtime_error("Test: StreamSocket exception: fd " + std::to_string(fd));
1069+
}
1070+
}
1071+
#endif
10591072
LOG_TRC("StreamSocket ctor");
10601073
if (isExternalCountedConnection())
10611074
++ExternalConnectionCount;
@@ -1767,6 +1780,9 @@ class StreamSocket : public Socket,
17671780

17681781
bool isExternalCountedConnection() const { return !_isClient && isIPType(); }
17691782
static std::atomic<size_t> ExternalConnectionCount; // accepted external TCP IPv4/IPv6 socket count
1783+
#if TEST_SERVERSOCKET_FAULT_INJECTION
1784+
static std::atomic<size_t> TestExternalStreamSocketCount;
1785+
#endif
17701786
};
17711787

17721788
enum class WSOpCode : unsigned char {

‎test/Makefile.am

+6
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,8 @@ all_la_unit_tests = \
9494
unit-timeout_inactive.la \
9595
unit-timeout_conn.la \
9696
unit-timeout_none.la \
97+
unit-serversock_accept1.la \
98+
unit-streamsock_ctor1.la \
9799
unit-base.la
98100
# unit-admin.la
99101
# unit-tilecache.la # Empty test.
@@ -232,6 +234,10 @@ unit_timeout_conn_la_SOURCES = UnitTimeoutConnections.cpp
232234
unit_timeout_conn_la_LIBADD = $(CPPUNIT_LIBS)
233235
unit_timeout_none_la_SOURCES = UnitTimeoutNone.cpp
234236
unit_timeout_none_la_LIBADD = $(CPPUNIT_LIBS)
237+
unit_serversock_accept1_la_SOURCES = UnitServerSocketAcceptFailure1.cpp
238+
unit_serversock_accept1_la_LIBADD = $(CPPUNIT_LIBS)
239+
unit_streamsock_ctor1_la_SOURCES = UnitStreamSocketCtorFailure1.cpp
240+
unit_streamsock_ctor1_la_LIBADD = $(CPPUNIT_LIBS)
235241
unit_prefork_la_SOURCES = UnitPrefork.cpp
236242
unit_prefork_la_LIBADD = $(CPPUNIT_LIBS)
237243
unit_storage_la_SOURCES = UnitStorage.cpp

0 commit comments

Comments
 (0)