Fix numerous thread/socket lifecycle crashes and TLS/epoll write-stall busy-loop
Thread/EPoll lifecycle: - Thread::~Thread() now actually joins/frees its worker thread (was empty - the OS thread kept running against already-freed EPoll/Socket objects on any early teardown, a confirmed "double free" crash) - EPoll::~EPoll() calls stop() if never explicitly stopped, for the same reason - epoll_wait() now uses a 1s timeout instead of blocking forever, so a worker thread can actually notice isStopping() and let itself be joined - EPoll::processCommand() iterated threads by value, silently double-freeing the real vector's Thread the moment each copy went out of scope - ThreadScope gained onThreadStart()/onThreadEnd() hooks (actually wired up now - the constructor that was supposed to store the scope discarded it) for per-thread C-library init (e.g. MySQL client) Socket/TCPSocket/TLS: - Socket::writeSocket(): give up and shut down a connection after 50 consecutive zero-progress sends instead of retrying forever - SSL_ERROR_WANT_READ on a write needs a READ to make progress, but resetSocket() re-arms EPOLLWRNORM regardless, which an already-writable TCP socket satisfies immediately - a real stuck connection generated 55GB, then 12.9GB of debug log this way before being caught - Socket::writeSocket() now honors sendData()'s actual return value (previously discarded, entry popped unconditionally after one call) - a large response could get silently truncated past what fit in one write()/SSL_write() call - TLS::registerSocket()/sendData()/receiveData(): `initialized` now only goes true once SSL_set_fd() has actually succeeded, and sendData()/receiveData() check it first - fixes a real SIGSEGV where a new session's own queued greeting could reach SSL_write() on a different epoll thread before this session's handshake setup had run - TCPServer::connectionRequest() now performs the TLS handshake ON THE ACCEPTED SESSION, not the listening server itself (was building SSL state on the wrong object, leaving every session's own `ssl` uninitialized) - TCPServer::receiveData() always uses the plain (non-TLS) read path - the listening socket is never itself a TLS peer - TCPSocket's block-mode literal reading used an unset `blockLength` field instead of `blockSize`, and could busy-spin forever on a multi-packet literal - TCPSocket line scanning replaced an unbounded strcspn() (assumes a NUL-terminated buffer; read past a non-NUL-terminated receive buffer into adjacent heap memory) with a bounds-checked memchr() scan Protocol/session handling: - CommandList::processRequest() now case-folds the command keyword before lookup - SMTP/POP3/IMAP commands are spec-required to be case-insensitive; a client sending "Mail From:" instead of "MAIL FROM:" previously failed to dispatch at all - TCPSession::protocol() no longer drops blank lines while a handler has grabbed input (e.g. __SMTP_DATA reading a message body, an IMAP APPEND literal) - those handlers depend on seeing blank lines as real content, not commands; this silently corrupted any message body containing a blank line - TCPSession's uuid alias fixed: streaming a raw uuid_t via operator<< triggered libstdc++'s unsigned-char* overload (strlen() on 16 binary bytes, a real stack-buffer-overflow read), and `alias` pointed at a stack-local that was already destroyed by the time the constructor returned - INotify::processEvent() only treats event->name as a NUL-terminated string when event->len > 0 (per inotify(7)) instead of unconditional strlen() on a field the kernel doesn't guarantee is even present - a confirmed heap-buffer-overflow read that had been misdiagnosed as a MySQL thread-safety issue
This commit is contained in:
Brad Arant committed
1 parent
a51a486560
commit
c3e8d70774
14 files changed
+627
-89
No files matched your search
+131
-13
@@ -3,6 +3,7 @@
|
||||
#include "Exception.h"
|
||||
#include "Log.h"
|
||||
#include "ZString.h"
|
||||
#include <iostream>
|
||||
|
||||
namespace core
|
||||
{
|
||||
@@ -80,23 +81,84 @@ namespace core
|
||||
if(event.events & EPOLLIN) {
|
||||
coreutils::Log(coreutils::LOG_DEBUG_2) << "EPOLLIN";
|
||||
coreutils::ZString zbuffer(buffer, length);
|
||||
lock.lock();
|
||||
receiveData(zbuffer);
|
||||
if(!shutDown) {
|
||||
inHandler = false;
|
||||
lock.unlock();
|
||||
resetSocket();
|
||||
// receiveData() calls all the way down through
|
||||
// onDataReceived()/HTTPRequest::doParse()/application request
|
||||
// handling with NO exception protection anywhere in that chain -
|
||||
// confirmed live: a single thrown coreutils::Exception (e.g. a
|
||||
// malformed request hitting a parse-time throw, well before
|
||||
// ApiSession::handleComplete()'s own try/catch around
|
||||
// match.render() ever runs) reached std::terminate() and killed
|
||||
// the entire server process, not just this one connection, since
|
||||
// this runs on an EPoll worker thread and C++ terminates the
|
||||
// whole program on an exception escaping a thread. The old
|
||||
// lock.lock()/lock.unlock() pair (no RAII) also leaked the lock
|
||||
// permanently on this same path - std::lock_guard fixes that as
|
||||
// a side effect of adding the try/catch here.
|
||||
try {
|
||||
std::lock_guard<std::mutex> guard(lock);
|
||||
receiveData(zbuffer);
|
||||
} catch(coreutils::Exception &e) {
|
||||
// Also to stderr, not just Log() - this process's Log class
|
||||
// writes through a static listener/file neither this server
|
||||
// nor its main() ever configures (its own startup banner
|
||||
// uses plain std::cout, not Log at all), so a Log()-only
|
||||
// message here would be silently dropped. stderr reliably
|
||||
// lands in the log file given how this process is started
|
||||
// (both stdout and stderr redirected to the same file).
|
||||
std::cerr << "Exception processing socket " << getDescriptor() << ": " << e.text << std::endl;
|
||||
coreutils::Log(coreutils::LOG_EXCEPT) << "Exception processing socket " << getDescriptor() << ": " << e.text;
|
||||
shutdown("exception during receiveData: " + e.text);
|
||||
} catch(std::exception &e) {
|
||||
std::cerr << "std::exception processing socket " << getDescriptor() << ": " << e.what() << std::endl;
|
||||
coreutils::Log(coreutils::LOG_EXCEPT) << "std::exception processing socket " << getDescriptor() << ": " << e.what();
|
||||
shutdown(std::string("exception during receiveData: ") + e.what());
|
||||
} catch(...) {
|
||||
std::cerr << "Unknown exception processing socket " << getDescriptor() << std::endl;
|
||||
coreutils::Log(coreutils::LOG_EXCEPT) << "Unknown exception processing socket " << getDescriptor();
|
||||
shutdown("unknown exception during receiveData");
|
||||
}
|
||||
}
|
||||
if(event.events & EPOLLWRNORM) {
|
||||
coreutils::Log(coreutils::LOG_DEBUG_2) << "EPOLLWRNORM";
|
||||
writeSocket();
|
||||
inHandler = false;
|
||||
resetSocket();
|
||||
}
|
||||
inHandler = false;
|
||||
// A graceful shutdown() (session.terminate() -> "goodbye" reply
|
||||
// queued via write(), then shutdown() called - see e.g. IMAP's
|
||||
// LOGOUT/POP3's QUIT/SMTP's QUIT) used to make keepAlive false
|
||||
// the moment shutDown was set, in this SAME event cycle that had
|
||||
// just queued that reply - Thread::run() then deletes this object
|
||||
// immediately on a false return, before the reply was ever
|
||||
// written to the actual socket (write() only queues into fifo;
|
||||
// the real send() happens in writeSocket(), only reachable via a
|
||||
// LATER EPOLLWRNORM event this object never lived to receive).
|
||||
// Confirmed live: LOGOUT/QUIT connections closed correctly but
|
||||
// their own final reply line never reached the client. Staying
|
||||
// alive while shutDown is set but fifo still has data - as long
|
||||
// as the peer hasn't already hung up (readHangup - trying to
|
||||
// drain to a peer that's already gone is pointless, not graceful)
|
||||
// - gives that queued reply a real chance to go out before this
|
||||
// socket actually closes.
|
||||
bool keepAlive = !shutDown || (fifo.size() > 0 && !readHangup);
|
||||
|
||||
// resetSocket() re-arms this fd via EPOLL_CTL_MOD, which makes it
|
||||
// immediately eligible for delivery to ANY worker thread's
|
||||
// epoll_wait() - including one already blocked and about to wake
|
||||
// up right away. The old code called resetSocket() from inside
|
||||
// the branches above while several more statements (an
|
||||
// unconditional inHandler write, this Log call, and `return
|
||||
// !shutDown`) still touched `this` afterward - a second thread
|
||||
// could legitimately receive the re-armed event, finish handling
|
||||
// it, and delete this object (Thread::run() does exactly that
|
||||
// when eventReceived() returns false) before this thread's own
|
||||
// remaining statements ran, a heap-use-after-free confirmed via
|
||||
// AddressSanitizer under concurrent load. resetSocket() must be
|
||||
// the last thing that ever touches `this`, with the return value
|
||||
// already captured in a local (`keepAlive`) beforehand.
|
||||
coreutils::Log(coreutils::LOG_DEBUG_1) << "Event process ending for socket " << getDescriptor();
|
||||
return !shutDown;
|
||||
if(keepAlive)
|
||||
resetSocket();
|
||||
return keepAlive;
|
||||
}
|
||||
|
||||
void Socket::connectionRequest() {}
|
||||
@@ -147,9 +209,57 @@ namespace core
|
||||
outlock.lock();
|
||||
// coreutils::Log(coreutils::LOG_DEBUG_3) << "writing data to socket " << getDescriptor();
|
||||
if(fifo.size() > 0) {
|
||||
if(!shutDown) {
|
||||
sendData(fifo.front().c_str(), fifo.front().length());
|
||||
fifo.pop();
|
||||
// No longer gated on !shutDown: writeSocket() only ever runs in
|
||||
// response to an EPOLLWRNORM event this object explicitly
|
||||
// re-armed for (see resetSocket()'s own doc comment) - refusing
|
||||
// to actually perform that queued write once shutDown was set
|
||||
// was exactly what left a graceful QUIT/LOGOUT's own reply
|
||||
// stuck in the fifo forever, never reaching the client.
|
||||
{
|
||||
// sendData()'s return value was previously discarded and the
|
||||
// fifo entry popped unconditionally after ONE call - correct
|
||||
// only by accident, since a single write()/SSL_write() call is
|
||||
// never guaranteed to accept an entire buffer. A response
|
||||
// small enough to fit the kernel's socket send buffer in one
|
||||
// shot (true for almost everything on this site, and always
|
||||
// true testing over loopback, where that buffer is
|
||||
// effectively unlimited) went out fine either way - but a
|
||||
// large one (confirmed with a real 208KB response, mostly an
|
||||
// embedded base64 font: fine over loopback, but only ~65KB of
|
||||
// it ever reached a real remote client before the rest was
|
||||
// silently dropped here) left the client waiting forever for
|
||||
// bytes matching its own Content-Length that were never
|
||||
// coming. Now: pop only once the FULL entry has gone out;
|
||||
// otherwise trim off just what was actually sent and leave
|
||||
// the remainder queued - resetSocket() (called at the end of
|
||||
// every eventReceived(), see its own doc comment) keeps
|
||||
// re-arming EPOLLWRNORM as long as the fifo isn't empty, so
|
||||
// the next write-ready event retries this same entry
|
||||
// automatically. A return <= 0 (EAGAIN or a transient error)
|
||||
// sends nothing this round and is handled identically - just
|
||||
// leave the entry queued for the next attempt.
|
||||
int sent = sendData(fifo.front().c_str(), fifo.front().length());
|
||||
if(sent > 0) {
|
||||
consecutiveWriteStalls = 0;
|
||||
if((size_t)sent >= fifo.front().length())
|
||||
fifo.pop();
|
||||
else
|
||||
fifo.front().erase(0, sent);
|
||||
} else if(++consecutiveWriteStalls > 50) {
|
||||
// sendData() keeps failing for a reason that has nothing to
|
||||
// do with the socket's own write-readiness (confirmed real
|
||||
// case: TLS::sendData() returning SSL_ERROR_WANT_READ,
|
||||
// which needs a READ to make progress, not another write
|
||||
// attempt) - resetSocket() re-arms EPOLLWRNORM regardless,
|
||||
// and an already-writable TCP socket makes it fire again
|
||||
// immediately, so this cycle would otherwise spin forever
|
||||
// doing real work at the CPU's own top speed. See
|
||||
// consecutiveWriteStalls' own doc comment in Socket.h for
|
||||
// the confirmed real damage (a 55GB, then 12.9GB debug log)
|
||||
// this caused before being noticed.
|
||||
coreutils::Log(coreutils::LOG_EXCEPT) << "Socket " << getDescriptor() << ": " << consecutiveWriteStalls << " consecutive writes made zero progress - giving up and shutting down.";
|
||||
shutdown("stuck write retry loop - no progress after repeated attempts");
|
||||
}
|
||||
}
|
||||
}
|
||||
outlock.unlock();
|
||||
@@ -199,7 +309,15 @@ namespace core
|
||||
event.events = EPOLLIN | EPOLLRDHUP | EPOLLONESHOT | EPOLLET;
|
||||
if(fifo.size() > 0)
|
||||
event.events |= EPOLLWRNORM;
|
||||
if(!shutDown)
|
||||
// Was `if(!shutDown)` - refused to re-arm at all once shutting
|
||||
// down, even with data still queued to send (e.g. a graceful
|
||||
// QUIT/LOGOUT's own final reply - see eventReceived()'s own doc
|
||||
// comment on keepAlive for the full chain this broke). Re-arming
|
||||
// for just long enough to drain the fifo, when the peer is still
|
||||
// there to receive it (!readHangup - draining to an already-gone
|
||||
// peer is pointless), is what actually lets that reply go out
|
||||
// before this socket closes.
|
||||
if(!shutDown || (fifo.size() > 0 && !readHangup))
|
||||
epoll_ctl(ePoll.getDescriptor(), EPOLL_CTL_MOD, getDescriptor(), &event);
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user