Fix premature socket re-arm race between write() and eventReceived()
Socket::write() unconditionally called resetSocket() (via EPOLL_CTL_MOD) from whatever thread called it, whenever it could grab `lock` - including while this same socket's OWN eventReceived() call was still mid-flight on another thread (past receiveData()'s lock_guard, but before its own trailing resetSocket()/return). Re-arming makes the fd immediately eligible for a third thread to pick up a fresh event and, if it decides to shut the connection down, delete the object outright (Thread::run() does exactly that on a false return) - while the original thread is still touching it. A real heap-corruption crash inside libc's malloc internals was confirmed live via dmesg under concurrent load, consistent with exactly this: a socket freed while another thread still held a reference to it. inHandler existed as a flag already but only fed a diagnostic log line - never actually gated anything. Made it a real std::atomic<bool> (was `volatile`, which gives no actual atomicity or cross-thread visibility) and used it two ways: - write() now skips its own resetSocket() call entirely while inHandler is true - that call's own eventReceived() will re-arm once it's actually done, seeing the fifo entry write() just queued. - eventReceived() itself was clearing inHandler well before its own resetSocket() call (several statements up, right after EPOLLIN/ EPOLLWRNORM handling) - moved the clear to immediately precede that call, closing the exposed window down to the minimum achievable without restructuring how write() re-arms sockets altogether. This is a distinct bug from the two subscriber/session-list races fixed earlier tonight (SubscriptionManager.cpp, TCPServer.cpp) - found while stress-testing those fixes turned up a third, different crash signature (heap corruption, not the null-EPoll-reference pattern the earlier two produced).
This commit is contained in:
1 parent
0bb926a264
commit
8009564a7a
2 files changed
+38
-3
No files matched your search
+32
-2
@@ -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();
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user