diff --git a/Socket.cpp b/Socket.cpp index 2167ed6..2c18c26 100644 --- a/Socket.cpp +++ b/Socket.cpp @@ -128,7 +128,6 @@ namespace core coreutils::Log(coreutils::LOG_DEBUG_2) << "EPOLLWRNORM"; writeSocket(); } - 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 @@ -162,6 +161,23 @@ namespace core // 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(); + // Was cleared several statements above (right after the EPOLLIN/ + // EPOLLWRNORM handling) - a genuinely separate race from the one + // documented above: Socket::write() re-arms via this same + // resetSocket() (see its own doc comment there) whenever it can + // grab `lock`, from whatever thread called it (e.g. a broadcast + // hitting this session while ITS OWN eventReceived() call is still + // running, past receiveData()'s lock_guard but before reaching + // here). Clearing inHandler that early left exactly that window + // open for write() to re-arm this fd a second, premature time, + // letting a third thread pick up a fresh event - and potentially + // delete this object outright - while this call was still + // mid-flight. Keeping inHandler true until the instant before + // resetSocket() actually runs (write() checks it, see its own + // comment) closes that window down to the two adjacent statements + // below, as tight as achievable without restructuring how write() + // re-arms at all. + inHandler = false; if(keepAlive) resetSocket(); return keepAlive; @@ -303,7 +319,21 @@ namespace core outlock.lock(); fifo.emplace(data); outlock.unlock(); - if(lock.try_lock()) { + // Was unconditional - re-arming here (resetSocket() -> EPOLL_CTL_MOD) + // while this socket's OWN eventReceived() call is still mid-flight + // on another thread (inHandler true - see eventReceived()'s own + // comment on exactly this race) makes the fd immediately eligible + // for a THIRD thread to pick up a fresh event and potentially + // delete this object (Thread::run() does exactly that on a false + // return) while the first thread is still touching it - a genuine + // use-after-free, matching a real heap-corruption crash confirmed + // live via dmesg under concurrent load. eventReceived() already + // re-arms itself once it's actually done (checking the current + // fifo, which already includes whatever was just queued above); + // skip doing it a second, early time from here while that's still + // in progress - the data isn't lost, just picked up on that call's + // own re-arm instead of this one's. + if(!inHandler && lock.try_lock()) { resetSocket(); lock.unlock(); } diff --git a/Socket.h b/Socket.h index c93f478..1bbb217 100644 --- a/Socket.h +++ b/Socket.h @@ -4,6 +4,7 @@ #include "ZString.h" #include #include +#include #include namespace core { @@ -199,7 +200,11 @@ namespace core { int descriptor = -1; std::mutex outlock; bool readHangup = false; - volatile bool inHandler = false; + // Was `volatile bool` - volatile gives no actual thread-safety + // (no atomicity, no cross-thread visibility guarantee), and this + // flag is now load-bearing (see Socket::write()'s own check of it) + // rather than just feeding a diagnostic log line. + std::atomic inHandler{false}; //------------------------------------------------------------------------------------- // the writeSocket is called when epoll has received a write request for a socket.