Skip to content

Commit 474b8ea

Browse files
committed
fix(ssl): eliminate coredump under HTTPS + multi-thread, harden server
Repros (kMultiThread + InitSSL + concurrent ab): - HttpServer::ssl_sockets_map_ was an unordered_map mutated concurrently by Accept/Read/Leave workers, causing rehash-time UAF on the bucket array. macOS libmalloc trapped (SIGTRAP) inside the freelist; lldb's unordered_map pretty-printer reliably segfaulted while inspecting the corrupted map, confirming the data structure itself was torn. Root causes addressed: - ssl_sockets_map_ has no synchronization. Add a std::mutex; critical sections only touch the map (shared_ptr is copied out and used outside the lock so SSL_read/write never run under the map lock). - SSLSocket ctor checked the member mode_ before assigning it, so pmutex_ was never created and the kSafely lock was silently disabled even when explicitly requested. Move the assignment before the check. - HandleAccept now passes SSLSocket::Mode::kSafely so the per-SSL* mutex actually serializes future concurrent SSL_read/write on the same session, in addition to the map lock. Additional robustness (orthogonal but surfaced by the same stress test): - SIGPIPE killed the entire server when a peer hung up mid-write. Ignore it once in TcpServer::Init via std::call_once, but only if the user has not installed their own handler (probe with sigaction + SIG_DFL), so the library does not stomp user signal policy. - SSL_accept blocked workers (and the single event loop in kIOMultiplexing mode) indefinitely on silent/stalled clients. Apply a bounded read timeout (default 10s, configurable via SetSSLHandshakeTimeout) before SSL_accept and restore the user's read_timeout_ for the actual request afterwards. Verified on macOS arm64: - Before: ab -n 2000 -c 100 -> SIGTRAP at ~15 requests. - After: 3000 concurrent curls -> 2996/2999 200 OK, server alive. 10 stalled TCP-only connections no longer starve workers; a legitimate curl is served within ~1.5s (handshake timeout reclaims a worker), instead of hanging forever.
1 parent 6e0efc3 commit 474b8ea

4 files changed

Lines changed: 96 additions & 13 deletions

File tree

src/cppnet/http/server/http_server.cpp

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -259,26 +259,49 @@ void HttpServer::HandleAccept(TcpServer &server, Socket &event_soc) {
259259
logger_->Debug("accept from " + recv_addr.ToString() +
260260
" soc: " + std::to_string(event_soc.fd()));
261261

262-
if (read_timeout_.first != 0 || read_timeout_.second != 0) {
263-
event_soc.SetReadTimeout(read_timeout_.first, read_timeout_.second);
264-
}
265262
if (write_timeout_.first != 0 || write_timeout_.second != 0) {
266263
event_soc.SetWriteTimeout(write_timeout_.first, write_timeout_.second);
267264
}
268265

269266
#ifdef CPPNET_OPENSSL
270267
if (ssl_context_) {
271-
auto ssl_socket = ssl_context_->AcceptSSL(event_soc);
268+
// Apply a bounded read timeout BEFORE SSL_accept so a silent client
269+
// cannot block this worker (or the single event loop) indefinitely.
270+
if (ssl_handshake_timeout_sec_ > 0) {
271+
event_soc.SetReadTimeout(ssl_handshake_timeout_sec_, 0);
272+
}
273+
// kSafely so concurrent SSL_read/write on the same SSL* is serialized
274+
// (needed in any multi-threaded layout above us).
275+
auto ssl_socket =
276+
ssl_context_->AcceptSSL(event_soc, SSLSocket::Mode::kSafely);
272277
if (ssl_socket == nullptr) {
273278
logger_->Error("[ssl_context.AcceptSSL]:" + ssl_context_->err_msg() +
274279
" soc:" + std::to_string(event_soc.fd()));
275280
server.RemoveSoc(event_soc);
276281
event_soc.Close();
277282
return;
278283
}
279-
ssl_sockets_map_[event_soc.fd()] = ssl_socket;
284+
{
285+
std::lock_guard<std::mutex> g(ssl_sockets_map_mtx_);
286+
ssl_sockets_map_[event_soc.fd()] = ssl_socket;
287+
}
288+
// Restore the user-configured read timeout for the actual HTTP request
289+
// reading (0/0 means "no timeout", matching the original default).
290+
if (ssl_handshake_timeout_sec_ > 0) {
291+
event_soc.SetReadTimeout(read_timeout_.first, read_timeout_.second);
292+
}
280293
}
281294
#endif
295+
296+
// For non-SSL path the read timeout has not been set above; do it now so
297+
// both code paths end up with the same socket configuration.
298+
if (
299+
#ifdef CPPNET_OPENSSL
300+
!ssl_context_ &&
301+
#endif
302+
(read_timeout_.first != 0 || read_timeout_.second != 0)) {
303+
event_soc.SetReadTimeout(read_timeout_.first, read_timeout_.second);
304+
}
282305
}
283306

284307
void HttpServer::HandleLeave(TcpServer &server, Socket &event_soc) {
@@ -287,10 +310,19 @@ void HttpServer::HandleLeave(TcpServer &server, Socket &event_soc) {
287310
logger_->Debug("leave from " + recv_addr.ToString() +
288311
" soc: " + std::to_string(event_soc.fd()));
289312
#ifdef CPPNET_OPENSSL
290-
if (ssl_context_ &&
291-
ssl_sockets_map_.find(event_soc.fd()) != ssl_sockets_map_.end()) {
292-
ssl_sockets_map_[event_soc.fd()]->CloseSSL();
293-
ssl_sockets_map_.erase(event_soc.fd());
313+
if (ssl_context_) {
314+
std::shared_ptr<SSLSocket> ssl_soc;
315+
{
316+
std::lock_guard<std::mutex> g(ssl_sockets_map_mtx_);
317+
auto it = ssl_sockets_map_.find(event_soc.fd());
318+
if (it != ssl_sockets_map_.end()) {
319+
ssl_soc = it->second;
320+
ssl_sockets_map_.erase(it);
321+
}
322+
}
323+
if (ssl_soc) {
324+
ssl_soc->CloseSSL();
325+
}
294326
}
295327
#endif
296328
}
@@ -302,14 +334,15 @@ void HttpServer::HandleRead(TcpServer &server, Socket &event_soc) {
302334

303335
#ifdef CPPNET_OPENSSL
304336
if (ssl_context_) {
305-
if (ssl_sockets_map_.find(event_soc.fd()) != ssl_sockets_map_.end()) {
306-
soc = ssl_sockets_map_[event_soc.fd()];
307-
} else {
337+
std::lock_guard<std::mutex> g(ssl_sockets_map_mtx_);
338+
auto it = ssl_sockets_map_.find(event_soc.fd());
339+
if (it == ssl_sockets_map_.end()) {
308340
logger_->Error("[logicerr]:ssl socket not found");
309341
server.RemoveSoc(event_soc);
310342
event_soc.Close();
311343
return;
312344
}
345+
soc = it->second;
313346
} else {
314347
soc = std::make_shared<Socket>(event_soc);
315348
}
@@ -463,6 +496,10 @@ void HttpServer::SetWriteTimeout(unsigned timeout_sec, unsigned timeout_usec) {
463496
write_timeout_ = std::make_pair(timeout_sec, timeout_usec);
464497
}
465498

499+
void HttpServer::SetSSLHandshakeTimeout(unsigned timeout_sec) {
500+
ssl_handshake_timeout_sec_ = timeout_sec;
501+
}
502+
466503
#ifdef CPPNET_OPENSSL
467504
int HttpServer::InitSSL(const Address &addr,
468505
std::shared_ptr<SSLContext> ssl_context) {

src/cppnet/http/server/http_server.hpp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
#ifdef CPPNET_OPENSSL
1414
#include "../../ssl/ssl_context.hpp"
15+
#include <mutex>
1516
#include <unordered_map>
1617
#endif
1718

@@ -228,6 +229,14 @@ class HttpServer : public HttpGroup {
228229
* @param: 0 stand for block until end
229230
*/
230231
void SetWriteTimeout(unsigned timeout_sec, unsigned timeout_usec);
232+
/**
233+
* @brief: set bounded SSL handshake read timeout (seconds).
234+
* A slow or silent client must not be allowed to block the worker
235+
* thread (or the single event loop) forever during SSL_accept.
236+
* Pass 0 to disable (NOT recommended for production).
237+
* Default: 10 seconds. Only effective in SSL mode.
238+
*/
239+
void SetSSLHandshakeTimeout(unsigned timeout_sec);
231240

232241
#ifdef CPPNET_OPENSSL
233242
public:
@@ -242,6 +251,11 @@ class HttpServer : public HttpGroup {
242251

243252
private:
244253
std::shared_ptr<SSLContext> ssl_context_ = nullptr;
254+
// Map of fd -> SSLSocket. Accessed concurrently by worker threads in
255+
// kMultiThread mode (Accept/Read/Leave each touch the map); the mutex
256+
// serializes those accesses. Critical sections only touch the map itself:
257+
// the shared_ptr is copied out and used outside the lock.
258+
std::mutex ssl_sockets_map_mtx_;
245259
std::unordered_map<int, std::shared_ptr<SSLSocket>> ssl_sockets_map_;
246260
#endif
247261

@@ -259,6 +273,8 @@ class HttpServer : public HttpGroup {
259273
bool is_continue_ = false;
260274
std::pair<int, int> read_timeout_{0, 0};
261275
std::pair<int, int> write_timeout_{0, 0};
276+
// Sane default so a stalled SSL handshake cannot pin a worker forever.
277+
unsigned ssl_handshake_timeout_sec_ = 10;
262278
};
263279

264280
} // namespace cppnet

src/cppnet/server/tcp_server.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,37 @@
22
#include "../utils/const.hpp"
33
#include "io_multiplexing/io_multiplexing_factory.hpp"
44

5+
#include <mutex>
56
#include <string.h>
67
#include <unistd.h>
78

9+
#ifndef _WIN32
10+
#include <signal.h>
11+
#endif
12+
813
namespace cppnet {
914

15+
namespace {
16+
// Ignore SIGPIPE once, process-wide. Without this, writing to a peer that has
17+
// closed the connection raises SIGPIPE and kills the entire server. Be polite:
18+
// if the user has already installed their own handler we leave it alone.
19+
void EnsureSigPipeIgnored() {
20+
#ifndef _WIN32
21+
static std::once_flag once;
22+
std::call_once(once, []() {
23+
struct sigaction old_action;
24+
if (sigaction(SIGPIPE, nullptr, &old_action) == 0 &&
25+
old_action.sa_handler == SIG_DFL) {
26+
struct sigaction action{};
27+
action.sa_handler = SIG_IGN;
28+
sigemptyset(&action.sa_mask);
29+
sigaction(SIGPIPE, &action, nullptr);
30+
}
31+
});
32+
#endif
33+
}
34+
} // namespace
35+
1036
TcpServer::TcpServer(const std::string &ip, uint16_t port) : addr_(ip, port) {}
1137

1238
TcpServer::TcpServer(Address &addr) : addr_(addr) {}
@@ -258,6 +284,7 @@ int TcpServer::WakeUp() {
258284
}
259285

260286
int TcpServer::Init() {
287+
EnsureSigPipeIgnored();
261288
loop_flag_ = true;
262289
listenfd_ = CreateSocket();
263290
if (listenfd_.status() != Socket::kInit) {

src/cppnet/ssl/ssl_socket.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,13 @@ SSLSocket::SSLSocket(SSL *ssl, const Socket &soc, Mode mode) {
2323
SSL_set_fd(ssl_, fd_);
2424
status_ = kInit;
2525

26+
// NOTE: assign mode_ BEFORE checking it; previously the check used the
27+
// default-initialized mode_ (kQuickly) and pmutex_ was never created,
28+
// silently disabling the SSL serialization lock.
29+
mode_ = mode;
2630
if (mode_ == Mode::kSafely) {
2731
pmutex_ = std::make_unique<std::mutex>();
2832
}
29-
mode_ = mode;
3033
}
3134

3235
int SSLSocket::Close() {

0 commit comments

Comments
 (0)