From 80c4721e578ae20f6c5a6b7959fde909582eeb85 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= <daniel@ekloef.se>
Date: Fri, 1 Nov 2019 19:51:33 +0100
Subject: [PATCH] fdm: don't free FD data that may be referenced by epoll
 return data

It is perfectly possible, and legal, for a FDM handler to delete
another handler. The problem however is when the epoll returned array
of FD events contain the removed FD handler *after* the handler that
removed it.

That is, if the epoll returned array is:

  [FD=13, FD=37]

and the handler for FD=13 removes FD=37, then given the current
implementation, the FD user data (our handler callback etc) will point
to a free:d address.

Add support for this situation by deferring FD handler removal *when
called from another handler*.

This is done by "locking" the FDM struct before looping the handlers
for FDs with events (and unlocking afterwards).

In fdm_del(), we check if the FDM has been locked, in which case the
FD is marked as deleted, and put on a deferred list. But
not *actually* deleted.

Meanwhile, in the FDM poll function, we skip calling handlers for
marked-for-deletion FDs.

Then, when all FDs have been processed, we loop the deferred list and
finally deletes the FDs for real.
---
 fdm.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
 fdm.h |  1 +
 2 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/fdm.c b/fdm.c
index 9523ab5f..e26d394c 100644
--- a/fdm.c
+++ b/fdm.c
@@ -1,6 +1,7 @@
 #include "fdm.h"
 
 #include <stdlib.h>
+#include <stdbool.h>
 #include <unistd.h>
 #include <errno.h>
 #include <assert.h>
@@ -15,11 +16,14 @@ struct fd {
     int fd;
     void *data;
     fdm_handler_t handler;
+    bool deleted;
 };
 
 struct fdm {
     int epoll_fd;
-    tll(struct fd) fds;
+    bool is_polling;
+    tll(struct fd *) fds;
+    tll(struct fd *) deferred_delete;
 };
 
 struct fdm *
@@ -34,7 +38,9 @@ fdm_init(void)
     struct fdm *fdm = malloc(sizeof(*fdm));
     *fdm = (struct fdm){
         .epoll_fd = epoll_fd,
+        .is_polling = false,
         .fds = tll_init(),
+        .deferred_delete = tll_init(),
     };
     return fdm;
 }
@@ -60,23 +66,31 @@ fdm_add(struct fdm *fdm, int fd, int events, fdm_handler_t handler, void *data)
 {
 #if defined(_DEBUG)
     tll_foreach(fdm->fds, it) {
-        if (it->item.fd == fd) {
+        if (it->item->fd == fd) {
             LOG_ERR("FD=%d already registered", fd);
             return false;
         }
     }
 #endif
 
-    tll_push_back(
-        fdm->fds, ((struct fd){.fd = fd, .data = data, .handler = handler}));
+    struct fd *fd_data = malloc(sizeof(*fd_data));
+    *fd_data = (struct fd) {
+        .fd = fd,
+        .data = data,
+        .handler = handler,
+        .deleted = false,
+    };
+
+    tll_push_back(fdm->fds, fd_data);
 
     struct epoll_event ev = {
         .events = events,
-        .data = {.ptr = &tll_back(fdm->fds)},
+        .data = {.ptr = fd_data},
     };
 
     if (epoll_ctl(fdm->epoll_fd, EPOLL_CTL_ADD, fd, &ev) < 0) {
         LOG_ERRNO("failed to register FD with epoll");
+        free(fd_data);
         tll_pop_back(fdm->fds);
         return false;
     }
@@ -84,14 +98,26 @@ fdm_add(struct fdm *fdm, int fd, int events, fdm_handler_t handler, void *data)
     return true;
 }
 
-bool
-fdm_del(struct fdm *fdm, int fd)
+static bool
+fdm_del_internal(struct fdm *fdm, int fd, bool close_fd)
 {
+    if (fd == -1)
+        return true;
+
     tll_foreach(fdm->fds, it) {
-        if (it->item.fd == fd) {
+        if (it->item->fd == fd) {
             if (epoll_ctl(fdm->epoll_fd, EPOLL_CTL_DEL, fd, NULL) < 0)
                 LOG_ERRNO("failed to unregister FD=%d from epoll", fd);
 
+            if (close_fd)
+                close(it->item->fd);
+
+            it->item->deleted = true;
+            if (fdm->is_polling)
+                tll_push_back(fdm->deferred_delete, it->item);
+            else
+                free(it->item);
+
             tll_remove(fdm->fds, it);
             return true;
         }
@@ -101,6 +127,18 @@ fdm_del(struct fdm *fdm, int fd)
     return false;
 }
 
+bool
+fdm_del(struct fdm *fdm, int fd)
+{
+    return fdm_del_internal(fdm, fd, true);
+}
+
+bool
+fdm_del_no_close(struct fdm *fdm, int fd)
+{
+    return fdm_del_internal(fdm, fd, false);
+}
+
 bool
 fdm_poll(struct fdm *fdm)
 {
@@ -114,10 +152,22 @@ fdm_poll(struct fdm *fdm)
         return false;
     }
 
+    fdm->is_polling = true;
     for (int i = 0; i < ret; i++) {
         struct fd *fd = events[i].data.ptr;
-        if (!fd->handler(fdm, fd->fd, events[i].events, fd->data))
+        if (fd->deleted)
+            continue;
+
+        if (!fd->handler(fdm, fd->fd, events[i].events, fd->data)) {
+            fdm->is_polling = false;
             return false;
+        }
+    }
+    fdm->is_polling = false;
+
+    tll_foreach(fdm->deferred_delete, it) {
+        free(it->item);
+        tll_remove(fdm->deferred_delete, it);
     }
 
     return true;
diff --git a/fdm.h b/fdm.h
index cd0fee2a..fb12bffb 100644
--- a/fdm.h
+++ b/fdm.h
@@ -11,5 +11,6 @@ void fdm_destroy(struct fdm *fdm);
 
 bool fdm_add(struct fdm *fdm, int fd, int events, fdm_handler_t handler, void *data);
 bool fdm_del(struct fdm *fdm, int fd);
+bool fdm_del_no_close(struct fdm *fdm, int fd);
 
 bool fdm_poll(struct fdm *fdm);