# HG changeset patch
# User Sergey Kandaurov <pluknet@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);

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel