# HG changeset patch # User Sergey Kandaurov <pluk...@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