port: drop a port's pending fd-event changes before it is freed

CHANGES

@@ -23,6 +23,15 @@ Changes with FreeUnit 1.36.2                                     30 Aug 2026
        is not valid UTF-8 with U+FFFD before it serializes.
 
     *) Change: upgrade contrib njs to 1.0.1.
+
+    *) Bugfix: a port that was closed and freed could leave a pending epoll
+       or kqueue change behind it. The change is held by a pointer into the
+       port, and is applied when the batch is committed -- at the top of the
+       next poll at the latest, which is after the port's memory pool is
+       released. The commit then read freed memory, and could also name a
+       descriptor number the kernel had already handed to somebody else. A
+       port now drops its pending changes before it is freed (#414).
+
     *) Bugfix: a process could stop hearing from a peer for good. When the
        wake-up that tells a peer to read the shared queue failed because
        the kernel could not allocate for it, the port treated the peer as

auto/sources

@@ -168,6 +168,7 @@ NXT_TEST_SRCS=" \
     src/test/nxt_http_chunk_parse_test.c \
     src/test/nxt_conf_parse_test.c \
     src/test/nxt_port_fail_test.c \
+    src/test/nxt_fd_event_change_test.c \
     src/test/nxt_port_use_unless_zero_test.c \
     src/test/nxt_port_mmap_range_test.c \
     src/test/nxt_port_ready_test.c \

docs/changes.xml

@@ -54,6 +54,18 @@ never filled up will not deliver (#392).
 </para>
 </change>
 
+<change type="bugfix">
+<para>
+a port that was closed and freed could leave a pending epoll or kqueue change
+behind it.  The change is held by a pointer into the port, and is applied when
+the batch is committed -- at the top of the next poll at the latest, which is
+after the port's memory pool is released.  The commit then read freed memory,
+and could also name a descriptor number the kernel had already handed to
+somebody else.  A port now drops its pending changes before it is freed
+(#414).
+</para>
+</change>
+
 <change type="change">
 <para>
 upgrade contrib njs to 1.0.1.

src/nxt_devpoll_engine.c

@@ -34,6 +34,8 @@ static void nxt_devpoll_enable(nxt_event_engine_t *engine, nxt_fd_event_t *ev);
 static void nxt_devpoll_disable(nxt_event_engine_t *engine, nxt_fd_event_t *ev);
 static nxt_bool_t nxt_devpoll_close(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
+static void nxt_devpoll_cancel_changes(nxt_event_engine_t *engine,
+    nxt_fd_event_t *ev);
 static void nxt_devpoll_enable_read(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
 static void nxt_devpoll_enable_write(nxt_event_engine_t *engine,
@@ -70,6 +72,7 @@ const nxt_event_interface_t  nxt_devpoll_engine = {
     nxt_devpoll_disable,
     nxt_devpoll_disable,
     nxt_devpoll_close,
+    nxt_devpoll_cancel_changes,
     nxt_devpoll_enable_read,
     nxt_devpoll_enable_write,
     nxt_devpoll_disable_read,
@@ -390,6 +393,46 @@ nxt_devpoll_change(nxt_event_engine_t *engine, nxt_fd_event_t *ev,
 }
 
 
+/*
+ * Take this event's pending changes out of the batch, so that a struct that
+ * is about to be freed is not dereferenced by nxt_devpoll_commit_changes().
+ *
+ * The change is dropped rather than committed: the descriptor is closed, or
+ * is about to be, so the change would name a descriptor number that may
+ * already belong to somebody else.
+ */
+
+static void
+nxt_devpoll_cancel_changes(nxt_event_engine_t *engine, nxt_fd_event_t *ev)
+{
+    nxt_devpoll_change_t  *change, *dst, *end;
+
+    if (!ev->changing) {
+        return;
+    }
+
+    dst = engine->u.devpoll.changes;
+    end = dst + engine->u.devpoll.nchanges;
+
+    for (change = dst; change < end; change++) {
+
+        if (change->event == ev) {
+            continue;
+        }
+
+        if (dst != change) {
+            *dst = *change;
+        }
+
+        dst++;
+    }
+
+    engine->u.devpoll.nchanges = (int) (dst - engine->u.devpoll.changes);
+
+    ev->changing = 0;
+}
+
+
 static nxt_int_t
 nxt_devpoll_commit_changes(nxt_event_engine_t *engine)
 {

src/nxt_epoll_engine.c

@@ -47,6 +47,8 @@ static void nxt_epoll_disable(nxt_event_engine_t *engine, nxt_fd_event_t *ev);
 static void nxt_epoll_delete(nxt_event_engine_t *engine, nxt_fd_event_t *ev);
 static nxt_bool_t nxt_epoll_close(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
+static void nxt_epoll_cancel_changes(nxt_event_engine_t *engine,
+    nxt_fd_event_t *ev);
 static void nxt_epoll_enable_read(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
 static void nxt_epoll_enable_write(nxt_event_engine_t *engine,
@@ -126,6 +128,7 @@ const nxt_event_interface_t  nxt_epoll_edge_engine = {
     nxt_epoll_disable,
     nxt_epoll_delete,
     nxt_epoll_close,
+    nxt_epoll_cancel_changes,
     nxt_epoll_enable_read,
     nxt_epoll_enable_write,
     nxt_epoll_disable_read,
@@ -172,6 +175,7 @@ const nxt_event_interface_t  nxt_epoll_level_engine = {
     nxt_epoll_disable,
     nxt_epoll_delete,
     nxt_epoll_close,
+    nxt_epoll_cancel_changes,
     nxt_epoll_enable_read,
     nxt_epoll_enable_write,
     nxt_epoll_disable_read,
@@ -412,6 +416,47 @@ nxt_epoll_close(nxt_event_engine_t *engine, nxt_fd_event_t *ev)
 }
 
 
+/*
+ * Take this event's pending changes out of the batch, so that a struct that
+ * is about to be freed is not dereferenced by nxt_epoll_commit_changes().
+ *
+ * The change is dropped rather than committed: the descriptor is closed, or
+ * is about to be, and close() removes it from the epoll set on its own.
+ * Committing instead would epoll_ctl() a descriptor number that may already
+ * name somebody else's file.
+ */
+
+static void
+nxt_epoll_cancel_changes(nxt_event_engine_t *engine, nxt_fd_event_t *ev)
+{
+    nxt_epoll_change_t  *change, *dst, *end;
+
+    if (!ev->changing) {
+        return;
+    }
+
+    dst = engine->u.epoll.changes;
+    end = dst + engine->u.epoll.nchanges;
+
+    for (change = dst; change < end; change++) {
+
+        if (change->event.data.ptr == ev) {
+            continue;
+        }
+
+        if (dst != change) {
+            *dst = *change;
+        }
+
+        dst++;
+    }
+
+    engine->u.epoll.nchanges = (nxt_uint_t) (dst - engine->u.epoll.changes);
+
+    ev->changing = 0;
+}
+
+
 static void
 nxt_epoll_enable_read(nxt_event_engine_t *engine, nxt_fd_event_t *ev)
 {

src/nxt_event_engine.h

@@ -67,6 +67,37 @@ typedef struct {
     nxt_bool_t                    (*close)(nxt_event_engine_t *engine,
                                       nxt_fd_event_t *ev);
 
+    /*
+     * Drop every change still pending for this event from the engine's
+     * change batch.
+     *
+     * The batching engines hold the change by pointer and dereference it
+     * when the batch is committed, at the top of the next poll at the
+     * latest.  Nothing else keeps the struct the pointer names alive until
+     * then, so anything that frees a struct holding an nxt_fd_event_t has to
+     * take its pending changes out of the batch first.
+     *
+     * The change is dropped, not committed: the descriptor is being closed
+     * or is closed already, so committing would act on a descriptor number
+     * that may already name somebody else's file.  A caller that wants the
+     * kernel told must delete the event before it closes, which is what
+     * nxt_fd_event_close() is for.
+     *
+     * For epoll and kqueue that leaves nothing behind: the kernel drops a
+     * descriptor from its set when the descriptor is closed.  The poll,
+     * devpoll and pollset engines instead keep the set in user space, in
+     * ->fd_hash and the engine's own array, and only an applied delete
+     * removes an entry from it.  Dropping a pending delete there leaves a
+     * stale entry pointing at the freed event -- the same entry those
+     * engines are already left with whenever an event is freed without a
+     * delete, which is the common case for a port.  Not committing is still
+     * the safer half: committing dereferences the freed event first.
+     *
+     * Runs on the engine's own thread, like every other operation here.
+     */
+    void                          (*cancel_changes)(nxt_event_engine_t *engine,
+                                      nxt_fd_event_t *ev);
+
     /*
      * Add a file descriptor to an event set and enable the most effective
      * read event notification method provided by underlying event facility.
@@ -363,6 +394,10 @@ void nxt_fd_event_hash_destroy(nxt_lvlhsh_t *lvlhsh);
     (engine)->event.close(engine, ev)
 
 
+#define nxt_fd_event_cancel_changes(engine, ev)                               \
+    (engine)->event.cancel_changes(engine, ev)
+
+
 #define nxt_fd_event_enable_read(engine, ev)                                  \
     (engine)->event.enable_read(engine, ev)
 

src/nxt_eventport_engine.c

@@ -23,6 +23,8 @@ static void nxt_eventport_disable(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
 static nxt_bool_t nxt_eventport_close(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
+static void nxt_eventport_cancel_changes(nxt_event_engine_t *engine,
+    nxt_fd_event_t *ev);
 static void nxt_eventport_enable_read(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
 static void nxt_eventport_enable_write(nxt_event_engine_t *engine,
@@ -63,6 +65,7 @@ const nxt_event_interface_t  nxt_eventport_engine = {
     nxt_eventport_disable,
     nxt_eventport_disable,
     nxt_eventport_close,
+    nxt_eventport_cancel_changes,
     nxt_eventport_enable_read,
     nxt_eventport_enable_write,
     nxt_eventport_disable_read,
@@ -293,6 +296,47 @@ nxt_eventport_disable_event(nxt_event_engine_t *engine, nxt_fd_event_t *ev)
 }
 
 
+/*
+ * Take this event's pending changes out of the batch, so that a struct that
+ * is about to be freed is not dereferenced by nxt_eventport_commit_changes().
+ *
+ * The change is dropped rather than committed: the descriptor is closed, or
+ * is about to be, so the change would name a descriptor number that may
+ * already belong to somebody else.
+ */
+
+static void
+nxt_eventport_cancel_changes(nxt_event_engine_t *engine, nxt_fd_event_t *ev)
+{
+    nxt_eventport_change_t  *change, *dst, *end;
+
+    if (!ev->changing) {
+        return;
+    }
+
+    dst = engine->u.eventport.changes;
+    end = dst + engine->u.eventport.nchanges;
+
+    for (change = dst; change < end; change++) {
+
+        if (change->event == ev) {
+            continue;
+        }
+
+        if (dst != change) {
+            *dst = *change;
+        }
+
+        dst++;
+    }
+
+    engine->u.eventport.nchanges = (nxt_uint_t)
+                                       (dst - engine->u.eventport.changes);
+
+    ev->changing = 0;
+}
+
+
 static nxt_int_t
 nxt_eventport_commit_changes(nxt_event_engine_t *engine)
 {

src/nxt_kqueue_engine.c

@@ -59,6 +59,8 @@ static void nxt_kqueue_disable(nxt_event_engine_t *engine, nxt_fd_event_t *ev);
 static void nxt_kqueue_delete(nxt_event_engine_t *engine, nxt_fd_event_t *ev);
 static nxt_bool_t nxt_kqueue_close(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
+static void nxt_kqueue_cancel_changes(nxt_event_engine_t *engine,
+    nxt_fd_event_t *ev);
 static void nxt_kqueue_enable_read(nxt_event_engine_t *engine,
     nxt_fd_event_t *ev);
 static void nxt_kqueue_enable_write(nxt_event_engine_t *engine,
@@ -142,6 +144,7 @@ const nxt_event_interface_t  nxt_kqueue_engine = {
     nxt_kqueue_disable,
     nxt_kqueue_delete,
     nxt_kqueue_close,
+    nxt_kqueue_cancel_changes,
     nxt_kqueue_enable_read,
     nxt_kqueue_enable_write,
     nxt_kqueue_disable_read,
@@ -313,6 +316,54 @@ nxt_kqueue_close(nxt_event_engine_t *engine, nxt_fd_event_t *ev)
 }
 
 
+/*
+ * Take this event's pending changes out of the batch, so that a struct that
+ * is about to be freed is not dereferenced when the batch is flushed -- by
+ * nxt_kqueue_get_kevent() when it fills, or by nxt_kqueue_poll().
+ *
+ * The change is dropped rather than flushed: the descriptor is closed, or is
+ * about to be, and kqueue(2) removes every kevent that references a
+ * descriptor when the descriptor is closed.  Flushing instead would name a
+ * descriptor number that may already belong to somebody else.
+ *
+ * Matching is on ->udata, which nxt_kqueue_fd_set() sets to the event; that
+ * names the struct being freed exactly, where ->ident names only a
+ * descriptor number.  nxt_kqueue_file_set() puts a file event in the same
+ * field, and a pointer comparison tells the two apart on its own.
+ *
+ * Unlike the other engines this does not return early when ->changing is
+ * clear.  Here the flag is only ever a "maybe" -- a flush leaves it set,
+ * for the reason nxt_kqueue_fd_set() gives -- so the scan is what decides,
+ * and it costs a walk of at most ->mchanges entries.
+ */
+
+static void
+nxt_kqueue_cancel_changes(nxt_event_engine_t *engine, nxt_fd_event_t *ev)
+{
+    struct kevent  *kev, *dst, *end;
+
+    dst = engine->u.kqueue.changes;
+    end = dst + engine->u.kqueue.nchanges;
+
+    for (kev = dst; kev < end; kev++) {
+
+        if (nxt_kevent_get_udata(kev->udata) == (void *) ev) {
+            continue;
+        }
+
+        if (dst != kev) {
+            *dst = *kev;
+        }
+
+        dst++;
+    }
+
+    engine->u.kqueue.nchanges = (int) (dst - engine->u.kqueue.changes);
+
+    ev->changing = 0;
+}
+
+
 /*
  * The kqueue event engine uses only three states: inactive, blocked, and
  * active.  An active oneshot event is marked as it is in the default
@@ -451,6 +502,25 @@ nxt_kqueue_fd_set(nxt_event_engine_t *engine, nxt_fd_event_t *ev,
 
     kev = nxt_kqueue_get_kevent(engine);
 
+    /*
+     * ->changing says a change for this event may be buffered.  It is what
+     * the generic callers test before they ask for a cancel, so kqueue has
+     * to set it even though no kqueue path reads it -- nxt_kqueue_close()
+     * scans by descriptor, and nxt_kqueue_cancel_changes() scans by ->udata
+     * without testing the flag.
+     *
+     * A flush does not clear it, because the batch mixes fd events with the
+     * file events nxt_kqueue_file_set() puts in the same ->udata field, and
+     * nothing in a kevent says which of the two it holds: clearing the flag
+     * over a flushed batch would write through an nxt_file_event_t as if it
+     * were an nxt_fd_event_t.  So the flag only ever says "maybe", and the
+     * cost of that is a scan that finds nothing.  It is never stale in the
+     * unsafe direction: it is set whenever a change is queued, and cleared
+     * only once the batch has been scanned.
+     */
+
+    ev->changing = 1;
+
     kev->ident = ev->fd;
     kev->filter = filter;
     kev->flags = flags;