[PATCH] Events: protection from stale changes to level-triggered kevents

[Sergey Kandaurov]

# HG changeset patch
# User Sergey Kandaurov <pluknet at nginx.com>
# Date 1706055243 -14400
#      Wed Jan 24 04:14:03 2024 +0400
# Node ID d47ed07b06e93f4c6137ccd4ddfce0de23afb6a2
# Parent  ee40e2b1d0833b46128a357fbc84c6e23be9be07
Events: protection from stale changes to level-triggered kevents.

When kqueue events are reported in level-triggered mode, without EV_CLEAR set,
it was previously possible to try to remove a kevent attached to a closed file
descriptor.  Calling close() on a file descriptor removes any kevents that
reference the descriptor, it is an error to remove such kevents afterwards.

In FreeBSD, this results in a kevent reported with EV_ERROR set in flags and
EBADF in data, which corresponds to operating on an invalid file descriptor.
MacOS behaves similarly; the difference is that it uses a distinct error path
for no knote found and EV_ADD unspecified, and returns EV_ERROR with ENOENT.
Either way, this triggers "kevent() error" alerts.

In practice, this may happen as part of handing read event after the main loop
in ngx_event_pipe(), which then initiates closing the connection, as caught by
proxy_chunked_extra.t.

Another use-case common on SSL connections is handling read event after SSL
handshaking is finished, which results in a kevent removal change.  It may
happen then to fully read and process the request in the same cycle iteration,
closing the connection with the pending kevent removal.  A variation of this
use-case is to re-add the event after SSL handshaking to read SSL payload,
e.g. as part of the application protocol greeting, then receive EPIPE from a
subsequent SSL_write() and remove the event again on connection close.
Normally this would result in three change list elements appended: EV_DELETE,
EV_ADD, EV_DELETE.  The check in ngx_kqueue_del_event() annihilates instead
a previously appended EV_ADD change, leaving the first remove change, which
reduces to the previous use-case.  Caught by mail_ssl_session_reuse.t.

The fix is to check in ngx_kqueue_process_events() if we operate over a just
closed file descriptor in this iteration, as it may happen after the change
list was updated, and prune such kevent changes before entering kevent().

Another use-case that makes the fix incomplete is processing further events
in the same iteration that may reuse a just closed file descriptor and clear
ev->closed while initializing a reused event, rendering the check useless.
This may happen e.g. as part of accepting a new connection.  The fix is to
check in ngx_kqueue_add_event() if there are invalidated events in the change
list matching the one being added and prune the corresponding kevent changes.

diff --git a/src/event/modules/ngx_kqueue_module.c b/src/event/modules/ngx_kqueue_module.c
--- a/src/event/modules/ngx_kqueue_module.c
+++ b/src/event/modules/ngx_kqueue_module.c
@@ -283,8 +283,11 @@ static ngx_int_t
 ngx_kqueue_add_event(ngx_event_t *ev, ngx_int_t event, ngx_uint_t flags)
 {
     ngx_int_t          rc;
+#if !(NGX_HAVE_CLEAR_EVENT)
+    ngx_uint_t         i;
+    ngx_event_t       *e;
+#endif
 #if 0
-    ngx_event_t       *e;
     ngx_connection_t  *c;
 #endif
 
@@ -329,6 +332,36 @@ ngx_kqueue_add_event(ngx_event_t *ev, ng
 
 #endif
 
+#if !(NGX_HAVE_CLEAR_EVENT)
+
+    for (i = 0; i < nchanges; i++) {
+        if (ev->index == NGX_INVALID_INDEX
+            && ((uintptr_t) change_list[i].udata & (uintptr_t) ~1)
+                == (uintptr_t) ev)
+        {
+            ngx_log_debug3(NGX_LOG_DEBUG_EVENT, ev->log, 0,
+                           "kevent stale: %d: ft:%d fl:%04Xd",
+                           (int) change_list[i].ident, change_list[i].filter,
+                           change_list[i].flags);
+
+            /*
+             * the stale event from a file descriptor
+             * that was just closed and reused in this iteration
+             */
+
+            if (i < --nchanges) {
+                e = (ngx_event_t *)
+                    ((uintptr_t) change_list[nchanges].udata & (uintptr_t) ~1);
+                change_list[i] = change_list[nchanges];
+                e->index = i;
+
+                i--;
+            }
+        }
+    }
+
+#endif
+
     rc = ngx_kqueue_set_event(ev, event, EV_ADD|EV_ENABLE|flags);
 
     return rc;
@@ -503,6 +536,9 @@ ngx_kqueue_process_events(ngx_cycle_t *c
     ngx_uint_t        level;
     ngx_err_t         err;
     ngx_event_t      *ev;
+#if !(NGX_HAVE_CLEAR_EVENT)
+    ngx_event_t      *e;
+#endif
     ngx_queue_t      *queue;
     struct timespec   ts, *tp;
 
@@ -530,6 +566,36 @@ ngx_kqueue_process_events(ngx_cycle_t *c
         tp = &ts;
     }
 
+#if !(NGX_HAVE_CLEAR_EVENT)
+
+    for (i = 0; i < n; i++) {
+        ev = (ngx_event_t *)
+                           ((uintptr_t) change_list[i].udata & (uintptr_t) ~1);
+
+        if (ev->closed) {
+            ngx_log_debug3(NGX_LOG_DEBUG_EVENT, cycle->log, 0,
+                           "kevent closed: %d: ft:%d fl:%04Xd",
+                           (int) change_list[i].ident, change_list[i].filter,
+                           change_list[i].flags);
+
+            /*
+             * the stale event change for a file descriptor
+             * that was just closed in this iteration
+             */
+
+            if (i < --n) {
+                e = (ngx_event_t *)
+                           ((uintptr_t) change_list[n].udata & (uintptr_t) ~1);
+                change_list[i] = change_list[n];
+                e->index = i;
+
+                i--;
+            }
+        }
+    }
+
+#endif
+
     ngx_log_debug2(NGX_LOG_DEBUG_EVENT, cycle->log, 0,
                    "kevent timer: %M, changes: %d", timer, n);