diff --git a/CommandList.cpp b/CommandList.cpp index 919836e..35816e3 100644 --- a/CommandList.cpp +++ b/CommandList.cpp @@ -1,5 +1,7 @@ #include "CommandList.h" #include "Log.h" +#include +#include namespace core { @@ -21,7 +23,20 @@ namespace core { request.split(delimiter, depth); request.reset(); try { - auto command = commands.at(request[0].str()); + // Every protocol built on this (SMTP, POP3, IMAP - RFC 5321 + // 2.4, RFC 1939 3, RFC 3501 2.2.1) specifies that command + // keywords are case-insensitive, but commands are always + // registered here in uppercase and this was an exact-match + // lookup - "mail from:" or "Mail From:" (both legal on the + // wire, and not unusual from real clients) silently failed + // to dispatch at all. Only the lookup key is case-folded; + // the token instances handlers actually receive are + // untouched, so any handler doing its own case-sensitive + // parsing on arguments is unaffected. + std::string key = request[0].str(); + std::transform(key.begin(), key.end(), key.begin(), + [](unsigned char c) { return std::toupper(c); }); + auto command = commands.at(key); return command->processCommand(request, session); } catch(...) { diff --git a/EPoll.cpp b/EPoll.cpp index 711ec30..13528d6 100644 --- a/EPoll.cpp +++ b/EPoll.cpp @@ -14,11 +14,27 @@ namespace core { maxSockets = 1000; epfd = epoll_create1(0); terminateThreads = false; + started = false; } - EPoll::~EPoll() {} + // Was empty - if this EPoll's worker pool was started but stop() was + // never explicitly called (e.g. an exception unwinding the stack + // during startup, before the caller's own normal shutdown path could + // run), the pool just kept running against this object's - and every + // Socket registered with it's - soon-to-be-freed memory. stop() itself + // is what actually joins every worker thread before anything gets + // torn down; see Thread::~Thread()'s doc comment for the crash this + // was confirmed to cause. Guarded by `started` so a never-started + // EPoll (numberOfThreads/threads still default) doesn't call stop() + // over a garbage thread count, and by `!terminateThreads` so an + // EPoll that already went through a normal stop() doesn't do it + // (and close an already-closed epfd) twice. + EPoll::~EPoll() { + if (started && !terminateThreads) + stop(); + } - bool EPoll::start(int numberOfThreads, int maxSockets) { + bool EPoll::start(int numberOfThreads, int maxSockets, ThreadScope *scope) { coreutils::Log(coreutils::LOG_DEBUG_2) << "Starting epoll event processing."; @@ -32,15 +48,20 @@ namespace core { //---------------------------------------------------------------------- // Create thread objects into vector for number of threads requested. // Hand all the threads a pointer to the EPoll object so they can run - // the socket handlers. + // the socket handlers - and, if given, the same shared ThreadScope + // so each one runs its onThreadStart()/onThreadEnd() hooks (see + // ThreadScope's own doc comment for why - e.g. MySQL client library + // per-thread init). //---------------------------------------------------------------------- for(int ix = 0; ix < numberOfThreads; ++ix) - threads.emplace_back(*this); + threads.emplace_back(*this, scope); for(int ix = 0; ix < numberOfThreads; ++ix) threads[ix].start(); + started = true; + return true; } @@ -74,7 +95,16 @@ namespace core { int EPoll::processCommand(coreutils::ZString &request, TCPSession &session) { int sequence = 0; - for(auto threadx : threads) { + // Was `for(auto threadx : threads)` - copied each live, running + // Thread by value. Harmless while ~Thread() was empty, but now + // that it actually joins and deletes the underlying std::thread* + // (see its own doc comment), a copy going out of scope at the end + // of each iteration would join and free the SAME thread the real + // vector entry still owns and expects to keep running - a + // double-free the next time anything lists threads on a live + // server (this method is reachable via the console's own command + // dispatch). + for(auto &threadx : threads) { session.out << "|" << ++sequence; threadx.output(session.out); session.out << "|" << std::endl; diff --git a/EPoll.h b/EPoll.h index 58fc19b..d458dff 100644 --- a/EPoll.h +++ b/EPoll.h @@ -51,9 +51,13 @@ namespace core { /// /// @param numberOfThreads the number of threads to start for processing epoll entries. /// @param maxSockets the maximum number of open sockets that epoll will manage. + /// @param scope optional - if given, its onThreadStart()/onThreadEnd() run once on + /// each worker thread, at the top/bottom of that thread's own lifetime. The SAME + /// scope instance is shared across every worker thread - see ThreadScope's own + /// doc comment. Not owned by EPoll; caller keeps it alive for the server's lifetime. /// - - bool start(int numberOfThreads, int maxSockets); ///< Start the BMAEPoll processing. + + bool start(int numberOfThreads, int maxSockets, ThreadScope *scope = NULL); ///< Start the BMAEPoll processing. /// /// Use the stop() method to initiate the shutdown process for the epoll socket management. @@ -103,6 +107,12 @@ namespace core { int numberOfThreads; std::vector threads; volatile bool terminateThreads; + // Guards ~EPoll()'s own call to stop() - false until start() + // actually runs, so a never-started EPoll (numberOfThreads and + // threads both still default/empty) doesn't have its destructor + // iterate a garbage thread count. See ~EPoll()'s own doc comment + // in EPoll.cpp for what this is actually protecting against. + bool started; }; diff --git a/INotify.cpp b/INotify.cpp index 6914897..f04eb44 100644 --- a/INotify.cpp +++ b/INotify.cpp @@ -28,32 +28,53 @@ namespace core { ptr < buffer.getData() + buffer.getLength(); ptr += sizeof(struct inotify_event) + event->len) { event = (const struct inotify_event *) ptr; - coreutils::ZString name(event->name); + + // event->name only exists - and is only NUL-terminated - when + // event->len > 0 (inotify(7): "This field is present only when + // the event relates to a file inside a watched directory ... + // otherwise ... this field is absent."). For events with + // len == 0 (e.g. IN_UNMOUNT, IN_Q_OVERFLOW, IN_IGNORED, or + // self-watch events), event->name is not a valid C-string at + // all - calling strlen() on it (as the old ZString(const char*) + // one-arg constructor did unconditionally here) reads past + // whatever garbage follows in the socket's 4096-byte receive + // buffer looking for a NUL, a real heap-buffer-overflow read + // confirmed via AddressSanitizer under concurrent load (the + // eventual heap corruption this caused was previously + // misdiagnosed as a MySQL client thread-safety issue, since it + // surfaces on whichever EPoll worker thread happens to service + // this watcher's socket and corrupts unrelated heap state). + // strnlen() bounded by event->len (rather than the one-arg + // ZString ctor's unbounded strlen) matches the kernel's actual + // guarantee: name is NUL-padded to exactly event->len bytes. + coreutils::ZString name = event->len > 0 + ? coreutils::ZString(event->name, strnlen(event->name, event->len)) + : coreutils::ZString(""); if(event->mask & IN_ACCESS) inAccess(name); if(event->mask & IN_ATTRIB) - inAttrib(std::string(event->name)); + inAttrib(name.str()); if(event->mask & IN_CLOSE_WRITE) - inCloseWrite(std::string(event->name)); + inCloseWrite(name.str()); if(event->mask & IN_CLOSE_NOWRITE) - inCloseNoWrite(std::string(event->name)); + inCloseNoWrite(name.str()); if(event->mask & IN_CREATE) inCreate(name); if(event->mask & IN_DELETE) - inDelete(std::string(event->name)); + inDelete(name.str()); if(event->mask & IN_DELETE_SELF) - inDeleteSelf(std::string(event->name)); + inDeleteSelf(name.str()); if(event->mask & IN_MODIFY) - inModify(std::string(event->name)); + inModify(name.str()); if(event->mask & IN_MOVE_SELF) - inMoveSelf(std::string(event->name)); + inMoveSelf(name.str()); if(event->mask & IN_MOVED_FROM) - inMovedFrom(std::string(event->name)); + inMovedFrom(name.str()); if(event->mask & IN_MOVED_TO) - inMovedTo(std::string(event->name)); + inMovedTo(name.str()); if(event->mask & IN_OPEN) - inOpen(std::string(event->name)); + inOpen(name.str()); } } diff --git a/Socket.cpp b/Socket.cpp index 5e76f8c..b2b8fd7 100644 --- a/Socket.cpp +++ b/Socket.cpp @@ -3,6 +3,7 @@ #include "Exception.h" #include "Log.h" #include "ZString.h" +#include 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 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); } diff --git a/Socket.h b/Socket.h index 3ec7495..d94d855 100644 --- a/Socket.h +++ b/Socket.h @@ -209,6 +209,23 @@ namespace core { std::queue fifo; + // Counts consecutive writeSocket() calls that sent zero bytes for + // the SAME queued fifo entry (SSL_ERROR_WANT_READ from + // TLS::sendData() is the confirmed real case - see writeSocket()'s + // own doc comment). resetSocket() re-arms EPOLLWRNORM whenever the + // fifo is non-empty regardless of WHY the last send failed, and an + // already-writable TCP socket (the common case) makes EPOLLWRNORM + // fire again essentially immediately - so a send that keeps + // failing for a reason unrelated to socket writability (like SSL + // needing a read first) spins this cycle as fast as the CPU can + // run it, forever, with zero actual progress. Confirmed live: one + // stuck connection generated a 55GB, then a 12.9GB debug log + // before being noticed, at roughly 20 log lines per millisecond. + // Reset to 0 on any successful send (even a partial one - real + // progress happened); past the threshold, the connection is + // clearly never going to un-stick itself and gets shut down. + int consecutiveWriteStalls = 0; + void resetSocket(); std::mutex lock; diff --git a/TCPServer.cpp b/TCPServer.cpp index 2df7fdb..bab2411 100644 --- a/TCPServer.cpp +++ b/TCPServer.cpp @@ -26,18 +26,34 @@ namespace core { } void TCPServer::connectionRequest() { - coreutils::Log(coreutils::LOG_DEBUG_2) << "Connection request is being received on socket " << getDescriptor() << "."; + coreutils::Log(coreutils::LOG_DEBUG_2) << "Connection request is being received on socket " << getDescriptor() << "."; lock.lock(); TCPSession *session = accept(); if (session) sessions.push_back(session); - if (tlsInfo) { - registerSocket(session->getDescriptor()); - acceptSocket(); - } + // Must run ON THE SESSION, not `this` (the listening server) - the + // unqualified registerSocket()/acceptSocket() calls this replaced + // built the SSL* handshake state on the SERVER's own TLS-base + // member instead of the newly accepted session's, leaving every + // session's own `ssl` uninitialized and segfaulting the first time + // real data arrived on it (SSL_read() against a never-SSL_new()'d + // pointer - confirmed via gdb). Sessions already correctly inherit + // this server's SSL_CTX at construction (TCPSession's own + // TLS(SSL_CTX*)-taking constructor) - performTLSHandshake() just + // needed to actually be invoked on that object. + if (session && tlsInfo) + session->performTLSHandshake(); lock.unlock(); } + void TCPServer::receiveData(coreutils::ZString &buffer) { + // See this override's own doc comment in TCPServer.h - the + // listening socket always uses the plain read()/ENOTCONN detection + // path, never TLS::receiveData(), regardless of whether this server + // is TLS-configured. + Socket::receiveData(buffer); + } + TCPSession * TCPServer::accept() { try { diff --git a/TCPServer.h b/TCPServer.h index 5017ef6..4030dfe 100644 --- a/TCPServer.h +++ b/TCPServer.h @@ -129,8 +129,26 @@ namespace core { /// This method is called by the lower socket when a connection request comes into /// the listening and bound server port. /// - + void connectionRequest() override; + + /// + /// The listening socket itself is never a TLS peer - accepting a + /// connection on it surfaces as a plain read() failing with ENOTCONN + /// (see Socket::receiveData(), which is what actually detects and + /// dispatches to connectionRequest() above). When this server is + /// TLS-configured, TCPSocket::receiveData()'s normal ctx-based + /// dispatch would instead route that same event through + /// TLS::receiveData()/SSL_read() against a listening fd that was + /// never SSL_accept()-ed at all (real per-connection sessions are + /// the only sockets TLS applies to) - confirmed via a real crash + /// (SIGSEGV inside SSL_read, called with `this` pointing at this + /// server object, not a session). Always using the plain path here, + /// regardless of whether tlsInfo is set, keeps connection-detection + /// working the same way with or without TLS configured. + /// + + void receiveData(coreutils::ZString &buffer) override; /// /// This method is called when the Command associated with this object is requested diff --git a/TCPSession.cpp b/TCPSession.cpp index 3381306..0c70143 100644 --- a/TCPSession.cpp +++ b/TCPSession.cpp @@ -7,17 +7,32 @@ namespace core { TCPSession::TCPSession(EPoll &ePoll, std::string text) : TCPSocket(ePoll, text) { + // uuid_t is 16 raw binary bytes, not a NUL-terminated string - + // streaming it directly (the old code did `<< uuidObject`) hits + // libstdc++'s unsigned-char* operator<< overload, which treats its + // argument as a C-string and calls strlen() on it: a real + // stack-buffer-overflow read past the 16-byte local, confirmed via + // AddressSanitizer under concurrent load. Separately, `alias` was + // being pointed at `uuidObject` itself - a stack-local array that is + // destroyed the instant this constructor returns, leaving `alias` a + // dangling pointer for the rest of the session's life (used by + // compareAlias() for session identity). uuid_unparse() into the + // `uuid` member (declared for exactly this - see its doc comment) + // fixes both: a real NUL-terminated string safe to log, backed by + // storage that lives as long as the TCPSession does. uuid_t uuidObject; uuid_generate(uuidObject); - coreutils::Log(coreutils::LOG_DEBUG_1) << uuidObject; - alias = (void *)uuidObject; + uuid_unparse(uuidObject, uuid); + coreutils::Log(coreutils::LOG_DEBUG_1) << uuid; + alias = (void *)uuid; } TCPSession::TCPSession(EPoll &ePoll, TCPServer *server, std::string text) : TCPSocket(ePoll, server->ctx, text), server(server) { uuid_t uuidObject; uuid_generate(uuidObject); - coreutils::Log(coreutils::LOG_DEBUG_1) << uuidObject; - alias = (void *)uuidObject; + uuid_unparse(uuidObject, uuid); + coreutils::Log(coreutils::LOG_DEBUG_1) << uuid; + alias = (void *)uuid; } TCPSession::~TCPSession() { @@ -30,7 +45,21 @@ namespace core { } void TCPSession::protocol(coreutils::ZString &data) { - if (data.getLength() != 0) { + // The data.getLength() != 0 guard alone silently swallowed every + // blank line a client sent, before it ever reached + // CommandList::processRequest() - fine for ordinary command + // dispatch (a stray blank line between commands isn't a command), + // but CommandList::processRequest() already special-cases + // session.grab != NULL to hand the line to the grabbed handler + // unconditionally, blank or not - a handler mid-grabInput() (e.g. + // __SMTP_DATA reading a message body, or an IMAP APPEND literal) + // depends on seeing blank lines as real content, not commands. + // With the outer guard unconditional, every message body's + // header/body separator - and every blank line within the body + // itself - was getting dropped before that handler ever saw it. + // Confirmed via a delivered test message with zero CRLFCRLF + // sequences anywhere in it, which is what first exposed this. + if (grab != NULL || data.getLength() != 0) { if (!server->commands.processRequest(data, *this)) { coreutils::Log(coreutils::LOG_DEBUG_1) << "Received data could not be parsed: " << data.str(); } diff --git a/TCPSocket.cpp b/TCPSocket.cpp index 1d7b555..4cdf747 100644 --- a/TCPSocket.cpp +++ b/TCPSocket.cpp @@ -3,6 +3,8 @@ #include "Log.h" #include "Exception.h" #include "errno.h" +#include +#include namespace core { @@ -38,7 +40,15 @@ namespace core { // session (set in TLS::createContext(), inherited by sessions via // TLS(SSL_CTX*)), safely NULL by default otherwise (see TLS.h). if(ctx) { - int len = TLS::receiveData(buffer); + int len; + { + // Held only around the raw SSL_read call - released before + // onDataReceived() so the request-handling chain it triggers + // (which reaches Socket::write()/outlock) never runs while + // ioLock is held. See ioLock's own doc comment in TCPSocket.h. + std::lock_guard ioGuard(ioLock); + len = TLS::receiveData(buffer); + } if(len >= 0) { coreutils::ZString zbuffer(buffer.getData(), len); onDataReceived(zbuffer); @@ -49,12 +59,39 @@ namespace core { } int TCPSocket::sendData(const char *data, int length) { - if(ctx) - return TLS::sendData(data, length); - else + if(ctx) { + std::lock_guard ioGuard(ioLock); + return TLS::sendData(data, length); + } else return Socket::sendData(data, length); } + void TCPSocket::performTLSHandshake() { + // registerSocket() makes this fd eligible for epoll dispatch to ANY + // worker thread immediately. ioLock MUST be acquired BEFORE that + // call, not after - a first attempt here took the lock AFTER + // registerSocket(), which still left a real window: another thread + // could win the race for ioLock (via a read/write event on this + // same brand-new connection) and have SSL_read/SSL_write silently + // complete the handshake on OpenSSL's own initiative, and only + // THEN would this thread acquire ioLock and run its own, now + // redundant, acceptSocket() (SSL_accept) against an already- + // negotiated SSL object - confirmed via gdb `thread apply all bt` + // during a live crash: one thread mid-SSL_read, a second thread + // simultaneously inside performTLSHandshake, both on the identical + // object address. Acquiring ioLock first closes that window + // entirely: no other thread can even begin touching this + // connection's SSL object until this thread's own handshake + // attempt has released the lock. acceptSocket() only attempts + // SSL_accept() once and may leave the handshake incomplete + // (WANT_READ/WANT_WRITE) - that's fine, OpenSSL transparently + // finishes a pending handshake on the next SSL_read/SSL_write, + // which ioLock already serializes against everything else. + std::lock_guard ioGuard(ioLock); + registerSocket(getDescriptor()); + acceptSocket(); + } + void TCPSocket::onDataReceived(coreutils::ZString &data) { if (data.getLength() > 0) { @@ -63,28 +100,72 @@ namespace core { lineBufferSize += data.getLength(); while (lineBufferSize > 0) { if (blockSize == 0) { - lineLength = strcspn(lineBuffer, "\r\n"); + // strcspn() treats its argument as a NUL-terminated C + // string and keeps scanning past the end looking for one - + // lineBuffer is a raw byte buffer sized via realloc() to + // exactly lineBufferSize bytes, with no NUL terminator + // appended anywhere. Whenever the buffered data doesn't yet + // contain a CRLF (an ordinary, frequent case - a line split + // across TCP reads, or mid-way through a long one, e.g. a + // large email body), strcspn() read straight past + // lineBufferSize into whatever heap memory happened to + // follow, hunting for a byte that happened to match '\r', + // '\n', or an accidental '\0' - producing a lineLength + // larger than the buffer actually holds. Confirmed live via + // gdb: SIGSEGV inside a LATER, unrelated realloc() call on + // this same lineBuffer, well downstream of where the real + // corruption happened (lineBufferSize -= lineLength + // underflows when lineLength > lineBufferSize, and that + // negative value gets handed to realloc() as a size_t - + // realloc(ptr, SIZE_MAX-ish) - during a real inbound SMTP + // message with a long quoted-printable line). memchr() is + // the bounded equivalent - it never looks past the length + // given, so "no CRLF yet" correctly comes back as "not + // found" instead of reading off the end of the allocation. + char *cr = (char *)memchr(lineBuffer, '\r', lineBufferSize); + char *nl = (char *)memchr(lineBuffer, '\n', lineBufferSize); + char *found = (cr && nl) ? std::min(cr, nl) : (cr ? cr : nl); + lineLength = found ? (int)(found - lineBuffer) : lineBufferSize; if (lineLength == lineBufferSize) break; coreutils::ZString zLine(lineBuffer, lineLength); onLineReceived(zLine); + // lineLength < lineBufferSize is guaranteed by the break + // above, so this first index is always in bounds; bump + // past a lone '\r' only if there's still at least one more + // buffered byte to safely check for a following '\n' - + // otherwise (a bare trailing '\r' with nothing buffered + // after it yet) leave it for the next read to resolve, + // rather than reading lineBuffer[lineBufferSize], one past + // the allocation. if (lineBuffer[lineLength] == '\r') ++lineLength; - if (lineBuffer[lineLength] == '\n') + if (lineLength < lineBufferSize && lineBuffer[lineLength] == '\n') ++lineLength; lineBufferSize -= lineLength; if (lineBufferSize > 0) memmove(lineBuffer, lineBuffer + lineLength, lineBufferSize); lineBuffer = (char *)realloc(lineBuffer, lineBufferSize); } - else if (lineBufferSize >= blockLength) { - coreutils::ZString zBlock(lineBuffer, blockLength); + else if (lineBufferSize >= blockSize) { + coreutils::ZString zBlock(lineBuffer, blockSize); onBlockReceived(zBlock); - lineBufferSize -= blockLength; + lineBufferSize -= blockSize; if (lineBufferSize > 0) - memmove(lineBuffer, lineBuffer + blockLength, lineBufferSize); + memmove(lineBuffer, lineBuffer + blockSize, lineBufferSize); lineBuffer = (char *)realloc(lineBuffer, lineBufferSize); } + else + // Not enough buffered data yet for a full block - stop and + // wait for the next read, exactly like the line-mode + // branch's own "no complete line yet" break above. + // Without this, a block spanning more than one TCP read + // (any IMAP APPEND literal bigger than a single read()'s + // worth of socket buffer) sends this EPoll worker thread + // into an unconditional busy-spin - lineBufferSize never + // changes, so the while(lineBufferSize > 0) loop above + // never yields back to let more data arrive. + break; } } } diff --git a/TCPSocket.h b/TCPSocket.h index 67c8ec5..f2150a3 100644 --- a/TCPSocket.h +++ b/TCPSocket.h @@ -123,6 +123,24 @@ namespace core { virtual int sendData(const char *data, int length) override; + /// + /// Completes the TLS handshake (SSL_new/SSL_set_fd/SSL_accept) for + /// THIS socket's own ctx/descriptor. registerSocket()/acceptSocket() + /// are protected TLS members, reachable here via an ordinary + /// same-class call - the point of exposing this as a public wrapper + /// is so a sibling class (TCPServer, which does not inherit + /// TCPSocket-the-session's-particular-instance and so cannot reach + /// another object's protected members directly) can still trigger + /// the handshake on a specific accepted session from + /// TCPServer::connectionRequest(), rather than accidentally running + /// it against the listening server socket itself (see that call + /// site's own comment for the real, confirmed crash this replaces - + /// SSL_read() on an accepted session's never-initialized ssl + /// member). + /// + + void performTLSHandshake(); + protected: bool term = false; TLSInfo *tlsInfo; @@ -131,9 +149,28 @@ namespace core { char *lineBuffer = NULL; int lineBufferSize = 0; int lineLength = 0; - int blockLength = 0; int blockSize = 0; - + + // Socket's own `lock` (guards receiveData) and `outlock` (guards + // writeSocket/the fifo) are two DIFFERENT mutexes - a thread + // handling EPOLLIN and a different thread handling EPOLLWRNORM for + // the SAME connection can therefore call SSL_read and SSL_write on + // the SAME `ssl*` concurrently. OpenSSL does not support that: an + // SSL object must not be used by more than one thread at a time + // (even in 1.1.0+/3.x, which dropped the old app-supplied + // CRYPTO_set_locking_callback global-locking API entirely - that + // API never protected concurrent use of one SSL object anyway, + // only OpenSSL's own internal global state). Confirmed live via + // dmesg: "traps: apiserver[...] general protection fault ... in + // libssl.so.3" during ordinary interactive use. ioLock serializes + // just the raw SSL_read/SSL_write calls themselves (see + // receiveData/sendData below) - deliberately NOT the surrounding + // request-handling, which already nests `lock` before `outlock` + // (receiveData's call chain reaches Socket::write(), which takes + // outlock) - wrapping any larger scope in ioLock+lock+outlock + // together risks recreating that same ordering as a new cycle. + std::mutex ioLock; + }; } diff --git a/TLS.cpp b/TLS.cpp index d370d8c..0e23614 100644 --- a/TLS.cpp +++ b/TLS.cpp @@ -33,15 +33,27 @@ namespace core { void handshake_complete(const SSL *ssl, int where, int ret) { coreutils::Log(coreutils::LOG_DEBUG_3) << "==>" << SSL_state_string_long(ssl) << "<==" << ret; if(where & SSL_CB_HANDSHAKE_DONE) { - X509 *ssl_client_cert = SSL_get_peer_certificate(ssl); - if(!ssl_client_cert) - throw std::string("Unable to get peer certificate."); - X509_free(ssl_client_cert); - if(SSL_get_verify_result(ssl) != X509_V_OK) - throw std::string("Certificate verification failed."); - coreutils::Log(coreutils::LOG_DEBUG_3) << "Certificate verified successfully."; + // Only a context configured with SSL_VERIFY_PEER (see + // createContext()'s own cACertificate-gated block) ever sends a + // client a CertificateRequest in the first place - ordinary + // one-way TLS (a public site's normal case, no caCertificate + // configured) never asks the client for one, so + // SSL_get_peer_certificate() is unconditionally NULL there and is + // not a failure. Checking the mode actually negotiated on THIS + // connection (inherited from the context) rather than assuming + // every handshake is mutual-TLS is what makes plain + // browser-facing HTTPS work at all under this callback. + if(SSL_get_verify_mode(ssl) & SSL_VERIFY_PEER) { + X509 *ssl_client_cert = SSL_get_peer_certificate(ssl); + if(!ssl_client_cert) + throw std::string("Unable to get peer certificate."); + X509_free(ssl_client_cert); + if(SSL_get_verify_result(ssl) != X509_V_OK) + throw std::string("Certificate verification failed."); + coreutils::Log(coreutils::LOG_DEBUG_3) << "Certificate verified successfully."; + } } - else + else coreutils::Log(coreutils::LOG_DEBUG_3) << "No client certificate."; } @@ -74,23 +86,45 @@ namespace core { SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_SERVER); // SSL_CTX_set_generate_session_id(ctx, generate_session_id); SSL_CTX_set_cipher_list(ctx, "ECDH-ECDSA-AES256-GCM-SHA384:DHE-DSS-AES256-GCM-SHA384:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA256:DHE-RSA-AES256-SHA:DHE-DSS-AES256-SHA:AES256-GCM-SHA384:AES256-SHA256:AES256-SHA:AES128-GCM-SHA256:AES128-SHA256:AES128-SHA"); - if(SSL_CTX_use_certificate_file(ctx, tlsInfo->certificate.c_str(), SSL_FILETYPE_PEM) <= 0) + // _chain_file (not _file) - loads every certificate in the file, + // not just the first. A real Let's Encrypt fullchain.pem is the + // leaf cert followed by the intermediate CA cert concatenated in + // one file specifically so a server can hand both to the client + // in one shot; SSL_CTX_use_certificate_file() only ever reads the + // first PEM block, silently dropping the intermediate - confirmed + // via a real handshake against this exact fullchain.pem: clients + // got "unable to get local issuer certificate" because the server + // only ever sent the leaf, never the intermediate needed to chain + // up to a root the client already trusts. + if(SSL_CTX_use_certificate_chain_file(ctx, tlsInfo->certificate.c_str()) <= 0) throw coreutils::Exception("Error looking up certificate."); if(SSL_CTX_use_PrivateKey_file(ctx, tlsInfo->key.c_str(), SSL_FILETYPE_PEM) < 0) throw coreutils::Exception("Error with private key."); if(SSL_CTX_check_private_key(ctx) != 1) throw coreutils::Exception("Private key does not match certificate."); - SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL); - SSL_CTX_set_verify_depth(ctx, 1); - if(!SSL_CTX_load_verify_locations(ctx, tlsInfo->cACertificate.c_str(), NULL)) - throw coreutils::Exception("Cannot verify locations."); - SSL_CTX_set_client_CA_list(ctx, SSL_load_client_CA_file(tlsInfo->cACertificate.c_str())); - coreutils::Log(coreutils::LOG_INFO) << "Server key authenticated."; - } + // Client-certificate verification (mutual TLS) is opt-in, not the + // default - a public-facing site (e.g. a real Let's Encrypt + // server cert with no corresponding client CA of its own) has no + // client certificate to hand out to ordinary visitors, so + // requesting one via SSL_VERIFY_PEER would surface a "select a + // certificate" prompt real browsers can't meaningfully answer. + // Only enable it when a caCertificate is actually configured - + // existing internal/mTLS use of this class (any config that DOES + // set tls.caCertificate) keeps behaving exactly as before. + if(tlsInfo->cACertificate.getLength() > 0) { + SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL); + SSL_CTX_set_verify_depth(ctx, 1); + if(!SSL_CTX_load_verify_locations(ctx, tlsInfo->cACertificate.c_str(), NULL)) + throw coreutils::Exception("Cannot verify locations."); + SSL_CTX_set_client_CA_list(ctx, SSL_load_client_CA_file(tlsInfo->cACertificate.c_str())); + coreutils::Log(coreutils::LOG_INFO) << "Server key authenticated (client certificate required)."; + } else { + coreutils::Log(coreutils::LOG_INFO) << "Server key authenticated (no client certificate required)."; + } + } } - void TLS::registerSocket(int fd) { - initialized = true; + void TLS::registerSocket(int fd) { int ret; coreutils::Log(coreutils::LOG_DEBUG_3) << "TLS socket initializing on socket " << fd << "..."; @@ -104,11 +138,37 @@ namespace core { SSL_set_info_callback(ssl, handshake_complete); if((ret = SSL_set_fd(ssl, fd)) == 0) - throw std::string("Error setting TLS socket descriptor."); - + throw std::string("Error setting TLS socket descriptor."); + // if(!SSL_set_generate_session_id(ssl, generate_session_id)) // throw std::string("Error setting session identifier callback."); + // Was set unconditionally as the FIRST statement in this + // function (before `ssl` was even assigned) - a flag meant to + // mean "ssl is a valid, ready-to-use SSL*" needs to become true + // only once that's actually true, at the end, not the start. + // Previously set but never actually READ anywhere either - + // sendData()/receiveData() below now check it before touching + // `ssl` at all. See their own doc comments for the real, + // confirmed crash this fixes: a brand-new TLS session's own + // greeting (queued in onConnected(), see e.g. IMAPSession.h) + // gets written via Socket::write() -> resetSocket(), which - deep + // inside TCPServer::accept(), BEFORE TCPServer::connectionRequest() + // ever calls this session's own performTLSHandshake() - makes the + // fd immediately eligible for an EPOLLWRNORM event on a SEPARATE + // epoll worker thread. That thread's TCPSocket::sendData() takes + // ioLock (see its own doc comment) and calls straight into + // SSL_write() on `ssl`, which is genuinely uninitialized garbage + // (TLS.h explicitly documents `ssl` has no default member + // initializer) until THIS function runs - ioLock alone doesn't + // prevent it, since performTLSHandshake() (the only other place + // that takes ioLock) hasn't even been called yet at that point. + // Confirmed live via gdb under a mixed-protocol connection-churn + // stress test: SIGSEGV inside libssl.so's SSL_write(), called from + // TCPSocket::sendData() with ioGuard already held, while + // registerSocket()/acceptSocket() for that same session were still + // pending on the accepting thread. + initialized = true; } void TLS::acceptSocket() { @@ -136,6 +196,14 @@ namespace core { int TLS::receiveData(coreutils::ZString &buffer) { + // `ssl` is genuinely uninitialized until registerSocket() has + // fully run (see its own doc comment on `initialized` for the + // real crash this prevents) - a racing thread landing here first + // just gets "nothing yet", exactly like a real SSL_ERROR_WANT_READ + // would report, rather than touching a garbage pointer. + if (!initialized) + return -1; + int len; coreutils::Log(coreutils::LOG_DEBUG_3) << "receiveData TLS on socket."; @@ -168,6 +236,17 @@ namespace core { int TLS::sendData(const char *data, int length) { + // Same "ssl isn't ready yet" guard as receiveData() - see + // registerSocket()'s doc comment on `initialized` for the real, + // confirmed crash this prevents (SIGSEGV in SSL_write() on a + // still-uninitialized `ssl`, reached via a brand-new session's own + // greeting racing ahead of its own TLS handshake). Returning <= 0 + // here is already the exact case Socket::writeSocket() treats as + // "nothing sent this round, leave it queued and retry on the next + // EPOLLWRNORM" - no special-casing needed at any call site. + if (!initialized) + return -1; + int len; if((len = ::SSL_write(ssl, data, length)) >= 0) { diff --git a/Thread.cpp b/Thread.cpp index c99789e..ae66383 100644 --- a/Thread.cpp +++ b/Thread.cpp @@ -4,10 +4,35 @@ namespace core { - Thread::Thread(EPoll &ePoll) : ePoll(ePoll) {} - Thread::Thread(EPoll &ePoll, ThreadScope *scope) : ePoll(ePoll) {} + Thread::Thread(EPoll &ePoll) : ePoll(ePoll), _thread(NULL), thread(NULL) {} + // Previously discarded `scope` entirely (never assigned to the `thread` + // member) - a real, live bug found while adding the first actual caller + // of this constructor (APIServer's MysqlThreadScope): the whole point + // of passing a scope in was to have it invoked from run() below, which + // silently never happened. + Thread::Thread(EPoll &ePoll, ThreadScope *scope) : ePoll(ePoll), _thread(NULL), thread(scope) {} - Thread::~Thread() {} + // Was empty - _thread (heap-allocated in start()) was never joined or + // freed here. A joinable std::thread destroyed without join()/detach() + // calls std::terminate() immediately, which this leaked-pointer + // approach silently sidestepped: the wrapped OS thread just kept + // running - polling this Thread's EPoll reference, touching Socket + // objects registered with it - even after this object, its EPoll, and + // every one of those sockets had already been destroyed out from under + // it (confirmed live: an exception thrown during BARANTMail startup, + // after EPoll::start() had already spun up its worker pool, unwound + // the stack destructing EPoll and its registered sockets while those + // still-running threads kept dereferencing them, producing a real + // "double free or corruption" crash). join() first (skipped if already + // joined via EPoll::stop() having run normally) so the OS thread is + // actually gone before anything it might touch gets freed. + Thread::~Thread() { + if (_thread) { + if (_thread->joinable()) + _thread->join(); + delete _thread; + } + } void Thread::start() { _thread = new std::thread(&Thread::run, this); @@ -36,11 +61,14 @@ namespace core { } void Thread::run() { - + threadId = syscall(SYS_gettid); - + coreutils::Log(coreutils::LOG_DEBUG_1) << "Thread started with thread id " << threadId << "."; - + + if(thread) + thread->onThreadStart(); + count = 0; struct epoll_event events[50]; @@ -50,14 +78,25 @@ namespace core { if(ePoll.isStopping()) break; - status = "WAITING"; - int rc = epoll_wait(ePoll.getDescriptor(), events, 50, -1); - status = "RUNNING"; - - if(rc < 0) { - // TODO: Make log entry indicating status received and ignore for now. - } else if(rc == 0) { - break; + status = "WAITING"; + // A finite timeout, not the previous -1 (wait forever), is what + // makes EPoll::stop() able to return at all: a thread idly + // blocked here with no socket activity would otherwise never + // re-check isStopping() above, so join()ing it (both + // EPoll::stop() and, now, ~Thread() itself) would hang forever. + // Costs up to a one-second shutdown latency. + int rc = epoll_wait(ePoll.getDescriptor(), events, 50, 1000); + status = "RUNNING"; + + if(rc < 0) { + // TODO: Make log entry indicating status received and ignore for now. + } else if(rc == 0) { + // Plain timeout, no socket activity - not itself a signal to + // stop (isStopping() above already decides that). Previously + // broke out of the loop unconditionally, which the old + // infinite wait never actually triggered in practice, but + // would have silently ended this thread after any single + // idle second once the wait became finite. } else if(rc > 0) { for(int ix = 0; ix < rc; ++ix) { ++count; @@ -72,6 +111,10 @@ namespace core { } } } + + if(thread) + thread->onThreadEnd(); + coreutils::Log(coreutils::LOG_DEBUG_1) << "Thread ending with thread id " << threadId << "."; } diff --git a/ThreadScope.h b/ThreadScope.h index 48ca912..b49777f 100644 --- a/ThreadScope.h +++ b/ThreadScope.h @@ -3,10 +3,34 @@ namespace core { + /// + /// ThreadScope + /// + /// Extension point for per-OS-thread setup/teardown in an EPoll worker + /// pool. EPoll's threads are long-lived (one persistent event loop per + /// thread, for the process's whole lifetime, not spawned per-request) - + /// onThreadStart() runs once at the top of each thread, before it + /// enters its event loop; onThreadEnd() runs once as that thread is + /// shutting down. A single ThreadScope instance is shared across every + /// worker thread (passed to EPoll::start()), so overrides should only + /// touch thread-LOCAL state from these hooks, never the ThreadScope + /// object's own fields, without their own synchronization. + /// + /// Concrete motivating case: a C library that requires explicit + /// per-thread initialization before use from a multi-threaded program + /// (e.g. the MySQL client library's mysql_thread_init()/ + /// mysql_thread_end() - see APIServer's MysqlThreadScope) has no other + /// clean way to hook into an EPoll worker thread's lifetime. + /// + class ThreadScope { public: ThreadScope() {} + virtual ~ThreadScope() {} + + virtual void onThreadStart() {} + virtual void onThreadEnd() {} };