event/SocketEvent: remove FD before closing socket
SocketEvent knows the FD is still open and is about to close it, so it's unnecessary to rely on the kernel (via AbandonFD) to clean up the epoll_wait list. ### Why this is relevant - `AbandonFD` assumes that upon closing the socket, the FD will be automatically removed from the epoll list. That fd is associated with a reference to the `SocketEvent`, so this is an important and dangerous assumption to get wrong. In the case that the FD isn't immediately removed from the list by the kernel, the event loop can crash due to the `SocketEvent` being destroyed and it being a use-after-free bug at that point. - If a socket FD happens to be duplicated, then closing the SocketEvent FD will not automatically remove it from epoll, and will trigger said bug/crash. It is only automatically removed when all FD references to the underlying socket/resource are closed? - A `fork()` is one example where a socket FD can be duplicated and result in this situation. - `CLOEXEC` might be considered mitigation for this but also introduces a race condition where the crash can occur between a `fork()` and `exec()` without additional synchronization to freeze the event loop. One could argue the mpd event loop isn't fork-safe, and thus should be allowed to use `AbandonFD` however it likes. A decision on whether this is intended should probably be declared; but either way this fix seems appropriate in cases where `Abandon` isn't actually necessary. It also might be possible to fix `AbandonFD` to mark the `SocketEvent` as removed without using `EPOLL_CTL_DEL`? [edit: made this dependent on HAVE_THREADED_EVENT_LOOP which is always true for MPD, but not for ncmpc, for example - mk]
This commit is contained in:
parent
5cbbe8ae2e
commit
0a81e462db
@ -47,8 +47,18 @@ SocketEvent::Close() noexcept
|
||||
/* closing the socket automatically unregisters it from epoll,
|
||||
so we can omit the epoll_ctl(EPOLL_CTL_DEL) call and save
|
||||
one system call */
|
||||
if (std::exchange(scheduled_flags, 0) != 0)
|
||||
if (std::exchange(scheduled_flags, 0) != 0) {
|
||||
#ifdef HAVE_THREADED_EVENT_LOOP
|
||||
/* can't use this optimization in multi-threaded
|
||||
programs, because all file descriptors get
|
||||
duplicated in forked processes, leaving them
|
||||
registered in epoll, which could cause the parent
|
||||
to crash */
|
||||
loop.RemoveFD(fd.Get(), *this);
|
||||
#else
|
||||
loop.AbandonFD(*this);
|
||||
#endif
|
||||
}
|
||||
fd.Close();
|
||||
}
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user