Skip to content

Commit 7fcb554

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 (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 May consider handling temporary EMFILE, ENFILE, ENOBUFS and ENOMEM errors? 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
1 parent 16000e2 commit 7fcb554

6 files changed

+451
-45
lines changed

‎common/Unit.hpp

+9
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,15 @@ class UnitWSD : public UnitBase
503503
return false;
504504
}
505505

506+
// ---------------- ServerSocket hooks ----------------
507+
/// Simulate `::accept` errors for external `ServerSocket::accept`. Implement unrecoverable errors by throwing an exception.
508+
virtual bool simulateExternalAcceptError()
509+
{
510+
return false;
511+
}
512+
/// Simulate exceptions during `StreamSocket` constructor for external `ServerSocket::accept`.
513+
virtual void simulateExternalSocketCtorException(std::shared_ptr<Socket>& /*socket*/) { }
514+
506515
// ---------------- TileCache hooks ----------------
507516
/// Called before the lookupTile call returns. Should always be called to fire events.
508517
virtual void lookupTile(int part, int mode, int width, int height, int tilePosX, int tilePosY,

‎net/ServerSocket.hpp

+7-10
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ 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+
3236
ServerSocket(Socket::Type type,
3337
std::chrono::steady_clock::time_point creationTime,
3438
SocketPoll& clientPoller, std::shared_ptr<SocketFactory> sockFactory) :
@@ -104,18 +108,11 @@ class ServerSocket : public Socket
104108
if (events & POLLIN)
105109
{
106110
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 )
111+
if (clientSocket)
114112
{
115-
LOG_TRC("Accepted client #" << clientSocket->getFD());
113+
LOGA_TRC(Socket, "Accepted client #" << clientSocket->getFD() << ", " << *clientSocket);
116114
_clientPoller.insertNewSocket(std::move(clientSocket));
117-
} else
118-
LOG_WRN("Limiter rejected extConn[" << extConnCount << "/" << net::Defaults.maxExtConnections << "]: " << *clientSocket);
115+
}
119116
}
120117
}
121118

‎net/Socket.cpp

+109-35
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>
@@ -1149,63 +1151,119 @@ bool ServerSocket::bind([[maybe_unused]] Type type, [[maybe_unused]] int port)
11491151
#endif
11501152
}
11511153

1154+
bool ServerSocket::isRetryAcceptError(const int cause)
1155+
{
1156+
switch(cause)
1157+
{
1158+
case EINTR:
1159+
case EAGAIN: // == EWOULDBLOCK
1160+
case ENETDOWN: // man accept(2): to be treated like EAGAIN
1161+
case EPROTO: // man accept(2): to be treated like EAGAIN
1162+
case ENOPROTOOPT: // man accept(2): to be treated like EAGAIN
1163+
case EHOSTDOWN: // man accept(2): to be treated like EAGAIN
1164+
#ifdef ENONET
1165+
case ENONET: // man accept(2): to be treated like EAGAIN (undef on BSD, APPLE, ..)
1166+
#endif
1167+
case EHOSTUNREACH: // man accept(2): to be treated like EAGAIN
1168+
case EOPNOTSUPP: // man accept(2): to be treated like EAGAIN
1169+
case ENETUNREACH: // man accept(2): to be treated like EAGAIN
1170+
case ECONNABORTED: // OK
1171+
case ETIMEDOUT: // OK, should not happen due ppoll
1172+
return true;
1173+
default:
1174+
return false;
1175+
}
1176+
}
1177+
11521178
std::shared_ptr<Socket> ServerSocket::accept()
11531179
{
11541180
// Accept a connection (if any) and set it to non-blocking.
11551181
// There still need the client's address to filter request from POST(call from REST) here.
11561182
#if !MOBILEAPP
11571183
assert(_type != Socket::Type::Unix);
11581184

1185+
#if ENABLE_DEBUG
1186+
UnitWSD* const unitWsd = UnitWSD::isUnitTesting() ? &UnitWSD::get() : nullptr;
1187+
if (unitWsd && unitWsd->simulateExternalAcceptError())
1188+
return nullptr; // Recoverable error, ignore to retry
1189+
#endif
1190+
11591191
struct sockaddr_in6 clientInfo;
11601192
socklen_t addrlen = sizeof(clientInfo);
11611193
const int rc = ::accept4(getFD(), (struct sockaddr *)&clientInfo, &addrlen, SOCK_NONBLOCK | SOCK_CLOEXEC);
11621194
#else
11631195
const int rc = fakeSocketAccept4(getFD());
11641196
#endif
1165-
LOG_TRC("Accepted socket #" << rc << ", creating socket object.");
1166-
try
1197+
if (rc < 0)
11671198
{
1168-
// Create a socket object using the factory.
1169-
if (rc != -1)
1199+
const int cause = errno;
1200+
constexpr const char * messagePrefix = "Failed to accept. (errno: ";
1201+
if (isRetryAcceptError(cause))
11701202
{
1171-
#if !MOBILEAPP
1172-
char addrstr[INET6_ADDRSTRLEN];
1173-
1174-
Socket::Type type;
1175-
const void *inAddr;
1176-
if (clientInfo.sin6_family == AF_INET)
1177-
{
1178-
struct sockaddr_in *ipv4 = (struct sockaddr_in *)&clientInfo;
1179-
inAddr = &(ipv4->sin_addr);
1180-
type = Socket::Type::IPv4;
1181-
}
1182-
else
1183-
{
1184-
struct sockaddr_in6 *ipv6 = &clientInfo;
1185-
inAddr = &(ipv6->sin6_addr);
1186-
type = Socket::Type::IPv6;
1187-
}
1203+
// Recoverable error, ignore to retry
1204+
LOG_DBG(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
1205+
}
1206+
else
1207+
{
1208+
// Unrecoverable error, forceExit instead of throw (closing `ServerSocket` and discard it from `AcceptPoll` during `SocketPoll::poll`)
1209+
// May allow recovery from temporary EMFILE, ENFILE, ENOBUFS and ENOMEM errors?
1210+
LOG_FTL(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
1211+
Util::forcedExit(EX_SOFTWARE);
1212+
}
1213+
return nullptr;
1214+
}
1215+
LOG_TRC("Accepted socket #" << rc << ", creating socket object.");
11881216

1189-
std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, type);
1217+
#if !MOBILEAPP
1218+
char addrstr[INET6_ADDRSTRLEN];
11901219

1191-
::inet_ntop(clientInfo.sin6_family, inAddr, addrstr, sizeof(addrstr));
1192-
_socket->setClientAddress(addrstr, clientInfo.sin6_port);
1220+
Socket::Type type;
1221+
const void *inAddr;
1222+
if (clientInfo.sin6_family == AF_INET)
1223+
{
1224+
struct sockaddr_in *ipv4 = (struct sockaddr_in *)&clientInfo;
1225+
inAddr = &(ipv4->sin_addr);
1226+
type = Socket::Type::IPv4;
1227+
}
1228+
else
1229+
{
1230+
struct sockaddr_in6 *ipv6 = &clientInfo;
1231+
inAddr = &(ipv6->sin6_addr);
1232+
type = Socket::Type::IPv6;
1233+
}
1234+
::inet_ntop(clientInfo.sin6_family, inAddr, addrstr, sizeof(addrstr));
11931235

1194-
LOG_TRC("Accepted socket #" << _socket->getFD() << " has family "
1195-
<< clientInfo.sin6_family << ", " << *_socket);
1196-
#else
1197-
std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, Socket::Type::Unix);
1236+
const size_t extConnCount = StreamSocket::getExternalConnectionCount();
1237+
if (0 < net::Defaults.maxExtConnections && extConnCount >= net::Defaults.maxExtConnections)
1238+
{
1239+
LOG_WRN("Limiter rejected extConn[" << extConnCount << "/" << net::Defaults.maxExtConnections << "]: #"
1240+
<< rc << " has family "
1241+
<< clientInfo.sin6_family << ", address " << addrstr << ":" << clientInfo.sin6_port);
1242+
::close(rc);
1243+
return nullptr;
1244+
}
1245+
try
1246+
{
1247+
// Create a socket object using the factory.
1248+
std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, type);
1249+
#if ENABLE_DEBUG
1250+
if (unitWsd)
1251+
unitWsd->simulateExternalSocketCtorException(_socket);
11981252
#endif
1199-
return _socket;
1200-
}
1201-
return std::shared_ptr<Socket>(nullptr);
1253+
_socket->setClientAddress(addrstr, clientInfo.sin6_port);
1254+
1255+
LOG_TRC("Accepted socket #" << _socket->getFD() << " has family "
1256+
<< clientInfo.sin6_family << ", " << *_socket);
1257+
return _socket;
12021258
}
12031259
catch (const std::exception& ex)
12041260
{
12051261
LOG_ERR("Failed to create client socket #" << rc << ". Error: " << ex.what());
12061262
}
1207-
12081263
return nullptr;
1264+
#else
1265+
return createSocketFromAccept(rc, Socket::Type::Unix);
1266+
#endif
12091267
}
12101268

12111269
#if !MOBILEAPP
@@ -1251,11 +1309,27 @@ bool Socket::isLocal() const
12511309
std::shared_ptr<Socket> LocalServerSocket::accept()
12521310
{
12531311
const int rc = ::accept4(getFD(), nullptr, nullptr, SOCK_NONBLOCK | SOCK_CLOEXEC);
1312+
if (rc < 0)
1313+
{
1314+
constexpr const char * messagePrefix = "Failed to accept. (errno: ";
1315+
const int cause = errno;
1316+
if (isRetryAcceptError(cause))
1317+
{
1318+
// Recoverable error, ignore to retry
1319+
LOG_DBG(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
1320+
}
1321+
else
1322+
{
1323+
// Unrecoverable error, forced exit. Throw would close `ServerSocket` and discard it from `AcceptPoll` during `SocketPoll::poll`.
1324+
// May allow recovery from temporary EMFILE, ENFILE, ENOBUFS and ENOMEM errors?
1325+
LOG_FTL(messagePrefix << std::to_string(cause) << ", " << std::strerror(cause) << ')');
1326+
Util::forcedExit(EX_SOFTWARE);
1327+
}
1328+
return nullptr;
1329+
}
12541330
try
12551331
{
12561332
LOG_DBG("Accepted prisoner socket #" << rc << ", creating socket object.");
1257-
if (rc < 0)
1258-
return std::shared_ptr<Socket>(nullptr);
12591333

12601334
std::shared_ptr<Socket> _socket = createSocketFromAccept(rc, Socket::Type::Unix);
12611335
// Sanity check this incoming socket
@@ -1305,8 +1379,8 @@ std::shared_ptr<Socket> LocalServerSocket::accept()
13051379
catch (const std::exception& ex)
13061380
{
13071381
LOG_ERR("Failed to create client socket #" << rc << ". Error: " << ex.what());
1308-
return std::shared_ptr<Socket>(nullptr);
13091382
}
1383+
return nullptr;
13101384
}
13111385

13121386
/// Returns true on success only.

‎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)