port: drop a port's pending fd-event changes before it is freed by andypost · Pull Request #415 · freeunitorg/freeunit

andypost · 2026-09-17T09:26:19Z

Fixes #414.

What was wrong

An fd-event change is queued into the engine's batch by a pointer to the
event, and that pointer is dereferenced when the batch is committed -- at
the top of the next poll at the latest. Nothing kept the struct the
pointer names alive until then.

port->socket is embedded in nxt_port_t, so nxt_port_release() frees
it with port->mem_pool.

Reachability, reproduced

The issue traced a two-work-item interleaving. There is a tighter one, in a
single function: nxt_port_write_msgs() calls nxt_port_rearm() (which
reaches nxt_port_rearm_now() -> nxt_fd_event_enable_write() ->
nxt_epoll_change()) and then, with nothing in between, calls
nxt_port_use(task, port, use_delta) with a delta that can be negative
(src/nxt_port_socket.c, the cleanup: block). If that is the last
reference, nxt_port_release() runs right there and frees the pool the
pending change points into.

The new nxt_fd_event_change_test drives the platform's real event engine
and does exactly that: arm the write event, close the descriptors the way
nxt_port_close() does, drop the last reference. Built with
--tests --debug --openssl -fsanitize=address,undefined, with the fix
removed:

tests: [notice] fd event change test: releasing the port left 1 changes pointing into its freed memory pool
==250829==ERROR: AddressSanitizer: heap-use-after-free on address 0x6efd18cd3514
WRITE of size 1 at 0x6efd18cd3514 thread T0
    #0 nxt_epoll_commit_changes src/nxt_epoll_engine.c:674
    #1 nxt_epoll_poll src/nxt_epoll_engine.c:934
freed by thread T0 here:
    #2 nxt_mp_free src/nxt_mp.c:824
    #3 nxt_port_mp_cleanup src/nxt_port.c:111
    #5 nxt_mp_release src/nxt_mp.c:303
    #6 nxt_port_release src/nxt_port.c:289
    #7 nxt_port_use src/nxt_port.c:1427

That write is ev->changing = 0. The epoll_ctl() two lines later reads
ev->fd, a descriptor the port has already closed, so on a busy process it
can name somebody else's file.

The fix

A new engine operation, cancel_changes, takes an event's pending changes
out of the batch. nxt_port_release() calls it before releasing the pool.

The changes are dropped rather than committed: the descriptor is closed by
then -- nxt_port_mp_cleanup() asserts pair[0] and pair[1] are -1, so
every release path has run nxt_port_close() first -- so committing would
act on a descriptor number that may already name somebody else's file. A
caller that wants the kernel told deletes the event first, which is what
nxt_fd_event_close() is for.

For epoll and kqueue that leaves nothing behind: the kernel drops a closed
descriptor from its set. The poll, devpoll and pollset engines keep
their set in user space instead, and only an applied delete removes an
entry -- see the second bullet under What this does not fix.

Why not the two shapes the issue sketched:

Reference-count the port per pending change. The commit path holds an
nxt_fd_event_t * and there is no generic way back to an owner, so this
means a back-pointer and a release callback in a struct every connection
and listener embeds. Much larger, for the same result.
Defer the release, as a connection does. nxt_conn_close_handler()
asks nxt_fd_event_close() whether changes are pending and puts the rest
of its teardown behind a zero timer, which fires after the next poll. A
port has no close handler, no timer, and is freed from a reference-count
drop that can happen from anywhere; giving it one changes port lifetime
semantics far beyond this bug.

Implemented for all seven engines. select batches nothing, so its
implementation is empty. kqueue now sets ->changing in
nxt_kqueue_fd_set() so that the call-site guard means the same thing
there; nothing else in that engine reads the flag (nxt_kqueue_close()
scans by descriptor, and nxt_kqueue_cancel_changes() scans by ->udata
without testing it).

A kqueue flush deliberately does not clear the flag. 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 an entry
holds, so clearing the flag across a flushed batch would write through an
nxt_file_event_t as if it were an nxt_fd_event_t. The flag therefore
only ever says "maybe": it is set whenever a change is queued and cleared
only once the batch has been scanned, so it never skips a cancel that was
needed, and the cost of an over-report is a scan that finds nothing.

No other caller is added: connections, listeners and the signal pipe reach
the engine exactly as before.

Tests

nxt_fd_event_change_test, three legs, all asserting on the engine's own
change count so a leg that stops exercising the path fails rather than
passing quietly:

a queued change is dropped, and ->changing goes 1 -> 0;
cancelling one of three events keeps the other two, and the following
poll commits them -- this is the compaction, which can silently go wrong
while the count stays right. On epoll the leg then asks the kernel which
descriptors actually reached the set (EPOLL_CTL_MOD finds the two that
were kept, and reports ENOENT for the cancelled one), an oracle
independent of the engine's own bookkeeping;
the port path above.

The ->changing assertions are epoll-only. The flag is not a cross-engine
contract: a kqueue flush leaves it set for the reason given above, so
asserting changing == 0 after a commit would fail on every
NXT_HAVE_KQUEUE build. The change count is the oracle that holds on both.

Run green on --tests --openssl --debug with
-fsanitize=address,undefined (50 tests passed, no ASan report) and on
--tests --openssl release. Leg 3 was confirmed to fail without the fix,
both by its own assertion and by the ASan report quoted above.

What this does not fix

nxt_epoll_commit_changes() queues nxt_epoll_error_handler with the
same bare ev pointer on an epoll_ctl() failure; that work item has
the same lifetime gap and is untouched here.
poll, devpoll and pollset keep an fd -> event hash and set in user
space that nothing removes when a port is freed, because a port never
deletes its event. That dangling entry is pre-existing and is the common
case on those engines. This change narrows one sub-case and does not fix
it: where nxt_port_fd_disable_write() had queued an NXT_POLL_DELETE
(nxt_poll_disable_write() does that when ev->read is already
inactive), that delete used to be applied and would have removed the
entry; it is now dropped with the rest. Note this is still a net
improvement in memory safety rather than a regression: committing the
batch runs ev->changing = 0 on the freed event before it even reaches
the op, so on those engines the old behaviour was itself a
use-after-free. Fixing the residual properly means deleting the event
before the port is freed on those engines, which is a wider change than
epoll/kqueue: a pending fd-event change can outlive the port it points into (UAF) #414; they are not on the Linux or BSD path and want their own look.
The fix is anchored to the free, not to the descriptor close.
nxt_port_write_close() closes pair[1] and leaves socket.fd at the
number it had, and nxt_port_close() does the same for both, while
callers keep the port alive afterwards. So a change armed before such a
close can still be committed against a closed -- possibly reused --
descriptor number. That is not a use-after-free, and it is the narrower
half of the hazard epoll/kqueue: a pending fd-event change can outlive the port it points into (UAF) #414 describes. Moving the cancel into
nxt_port_close() would cover both with one call site, but it is not a
drop-in: nxt_router_thread_exit_handler() reassigns port->engine to
the router's engine immediately before calling nxt_port_close(), so a
cancel there would scan the wrong engine's batch on a live path. Left for
its own change.
Only the epoll path is compiled and run here. kqueue, devpoll,
pollset and eventport are reviewed, not built.

🤖 Generated with Claude Code

andypost · 2026-09-18T02:53:56Z

@codex review
@claude review

claude · 2026-09-18T02:54:12Z

Claude finished @andypost's task in 6m 6s —— View job

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

Gather context (diff, engine sources, port lifetime)
Review engine implementations of cancel_changes
Review nxt_port.c call site and lifetime reasoning
Review the new test
Post review

The diagnosis is right and the mechanism is the right shape: a pointer-keyed batch needs a way to un-queue an entry, the compaction is a correct stable in-place removal in all six batching engines, the select no-op is right, and the new interface slot is filled in every one of the eight nxt_event_interface_t initializers (positional, so a miss would have shifted). The test asserting on the engine's own nchanges rather than just "no ASan report" is the right call. Notes below, most-significant first.

I could not build or run: ./configure is outside my allowed tools here (--allowedTools would need Bash(./configure*) and Bash(make*)), so everything below is from reading. Findings 1, 2 and 3 are reasoned from the sources and I'd want them confirmed on the relevant platform.

1. The new test fails on kqueue platforms

src/nxt_kqueue_engine.c:508 sets ev->changing = 1, and — as its own comment says — the flag is cleared only in cancel_changes. Neither nxt_kqueue_get_kevent()'s flush (src/nxt_kqueue_engine.c:540) nor nxt_kqueue_poll() (src/nxt_kqueue_engine.c:748) clears it.

Leg 2 of the test asserts the opposite, at src/test/nxt_fd_event_change_test.c:347:

if (nxt_slow_path(ev[0]->changing != 0 || ev[2]->changing != 0)) {
    ... "a committed change left changing set (%d, %d)" ...
    goto done;
}

That leg is compiled under #elif (NXT_HAVE_KQUEUE), so on FreeBSD/macOS --tests builds it should report (1, 1) and return NXT_ERROR. Same for leg 1's pre-check pairing — leg 1 passes because cancel_changes does clear the flag, but leg 2's post-commit check has no kqueue equivalent. Either make that assertion epoll-only, or clear changing in the kqueue flush paths (which would also make the flag mean the same thing on every engine). The PR notes kqueue was "reviewed, not built", which is exactly where this lands. Fix this →

2. On poll/devpoll/pollset, dropping a pending DELETE is not the same as committing it

The header comment justifies dropping with "every facility this abstracts over removes a descriptor from its set when the descriptor is closed." That holds for epoll and kqueue kernel sets. It does not hold for poll, devpoll and pollset, whose set is user-space state that only nxt_poll_set_delete() and friends touch — engine->u.poll.set plus fd_hash.

The delete case is reachable for a port: nxt_port_fd_disable_write() (src/nxt_port_socket.c:632) reaches nxt_poll_disable_write(), which queues NXT_POLL_DELETE when ev->read == NXT_EVENT_INACTIVE (src/nxt_poll_engine.c:237) — and nxt_port_read_close() sets exactly that (src/nxt_port_socket.c:1382). Cancel that DELETE and the entry survives with a closed descriptor, so the next poll() returns POLLNVAL, and src/nxt_poll_engine.c:606 looks the fd up in the hash and dereferences the freed event (ev->task, ev->error_handler) to enqueue a handler on it.

The PR is right that the hash entry is already stale on the common path (a port never deletes its event), but this removes the one path that did clean it up, so on those engines it can turn a benign-in-practice read of ev->fd into a later dereference of the whole struct. Worth either applying the delete's set/hash removal in cancel_changes on those three engines, or stating the regression explicitly in the "what this does not fix" list rather than calling them unaffected.

3. The same change can outlive the descriptor without any free

The fix is anchored to nxt_port_release(), i.e. to the free. But the descriptor dies earlier and independently: nxt_port_write_close() (src/nxt_port_socket.c:198) closes pair[1] and deliberately leaves socket.fd at the number it had, and nxt_port_close() (src/nxt_port.c:155) does the same for both — and callers such as src/nxt_router.c:1592 and src/nxt_process.c:816 keep the port alive afterwards.

So: arm write → close → next poll commits epoll_ctl(..., ev->fd) on a closed, possibly reused descriptor. No use-after-free, but it is the second half of the hazard this PR describes ("can name somebody else's file"), and it is not covered. Cancelling where the descriptor dies rather than where the memory dies would cover both cases with one call site, and nxt_port_release() would then need nothing. If you'd rather keep the scope tight, this belongs in "what this does not fix" — it's the nearest neighbour of the bug being fixed.

4. The call-site guard is what forces the kqueue flag

src/nxt_port.c:258:

if (port->engine != NULL && port->socket.changing) {
    nxt_fd_event_cancel_changes(port->engine, &port->socket);
}

Six of the seven engines test ev->changing themselves on entry; kqueue's implementation deliberately ignores it and scans. So the port->socket.changing half of this guard buys one avoided call and, in exchange, requires nxt_kqueue_fd_set() to start writing a flag it never clears — a permanently-stale bit in a struct that generic code can read. Dropping the && port->socket.changing and the ev->changing = 1 in nxt_kqueue_fd_set() would leave behaviour identical and remove that wrinkle (and, incidentally, finding 1).

5. `port->engine` is not guaranteed to be the engine that holds the change

The comment says a port with an engine is released on that engine's thread, which nxt_port_use() does guarantee. But it also implicitly assumes port->engine is the engine the change was queued on. nxt_router_thread_exit_handler() reassigns it at src/nxt_router.c:5255 — port->engine = task->thread->engine — for a port that was read-enabled on the departed worker's engine, then closes and drops the last reference. Benign today, because that worker engine never polls again, but the cancel would scan the wrong batch. Worth a clause in the comment, since the rest of it is careful about exactly this kind of thing.

6. Smaller things

src/nxt_eventport_engine.c:333 is 84 columns; everything else in these files holds to 80.
The CHANGES entry has no blank line before or after it, unlike its neighbours in that file. It is also placed after the njs "Change" entry while docs/changes.xml places the same entry before it — the two files disagree on order.
Leg 2 proves the count and (under ASan) that nothing freed was touched, but in a plain build it does not prove the right two entries survived compaction — a swapped entry commits silently. An epoll_ctl(EPOLL_CTL_MOD) on ev[1]'s descriptor returning ENOENT, and 0 for ev[0]'s, would pin that down without a sanitizer.
nxt_epoll_cancel_changes() early-returns on !ev->changing while nxt_kqueue_cancel_changes() does not. Both are correct for their engine, but the asymmetry is the kind of thing worth one line at the kqueue call site rather than only in the fd_set comment.

Findings 1 and 2 are the ones I'd want resolved before merge; 3 is a scope call that at minimum wants a line in the PR's own "what this does not fix". The epoll path — which is what ships on Linux — reads correct to me.
· branch fix/414-epoll-change-lifetime

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a2ee5c14a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17b892d521

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

An fd-event change is queued into the engine's batch by a pointer to the event, and the pointer is dereferenced when the batch is committed -- at the top of the next poll at the latest. Nothing kept the struct that pointer names alive until then. port->socket lives inside the port, so nxt_port_release() frees it with port->mem_pool. nxt_port_rearm_now() arms the write event, and nxt_port_write_msgs() calls it immediately before the nxt_port_use() that can take the count to zero and release the port; nothing runs in between. The next nxt_epoll_poll() then writes ev->changing into freed memory and calls epoll_ctl() with a descriptor number the port had closed, which by then may name somebody else's file. A connection does not have this problem: nxt_conn_close_handler() asks nxt_fd_event_close() whether changes are pending and defers the rest of its teardown behind a zero timer, which fires after the next poll. A port has no such handler, never deletes its event from the engine, and is freed from a reference count drop that can happen anywhere. So add the missing engine operation instead: cancel_changes takes an event's pending changes out of the batch. The changes are dropped, not committed -- the descriptor is already closed (nxt_port_mp_cleanup() asserts pair[] is -1), and every facility here removes a descriptor from its set when the descriptor is closed. A caller that wants the kernel told deletes the event first, which is what nxt_fd_event_close() is for. nxt_port_release() calls it, guarded on ->changing so that a port that queued nothing pays nothing. kqueue now sets ->changing in nxt_kqueue_fd_set() so the guard means the same thing there; nothing else in that engine reads the flag. select batches nothing, so its implementation is empty. nxt_fd_event_change_test drives the platform's real engine, not a stub, and asserts on the engine's change count so a leg that stops exercising the path fails instead of passing quietly. Without the call in nxt_port_release() its third leg reports 1 change left and, under -fsanitize=address, a heap-use-after-free in nxt_epoll_commit_changes() on memory freed by nxt_port_release(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 19e0b0baf8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-09-18T15:18:06Z

+
+    nxt_fd_event_enable_write(engine, ev);
+
+    if (nxt_slow_path(nxt_fd_event_change_test_nchanges(engine) != 1)) {


Flush fixture changes before asserting batch sizes

When epoll is built without NXT_HAVE_EVENTFD, or kqueue without NXT_HAVE_EVFILT_USER, nxt_event_engine_create() installs its fallback signal pipe through nxt_fd_event_enable_read(), leaving one change in this same batch before the test event is enabled. This assertion therefore observes 2 rather than 1 and the test suite fails immediately on those supported configurations; poll the new engine once or record/subtract its initial change count before running the legs.

Useful? React with 👍 / 👎.

andypost marked this pull request as ready for review September 18, 2026 02:53

chatgpt-codex-connector Bot reviewed Sep 18, 2026

View reviewed changes

Comment thread src/test/nxt_fd_event_change_test.c

andypost force-pushed the fix/414-epoll-change-lifetime branch from 2a2ee5c to 17b892d Compare September 18, 2026 10:48

chatgpt-codex-connector Bot reviewed Sep 18, 2026

View reviewed changes

Comment thread src/test/nxt_fd_event_change_test.c Outdated

andypost and others added 2 commits September 18, 2026 17:01

changelog: a freed port no longer leaves a pending fd-event change

19e0b0b

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

andypost force-pushed the fix/414-epoll-change-lifetime branch from 17b892d to 19e0b0b Compare September 18, 2026 15:13

chatgpt-codex-connector Bot reviewed Sep 18, 2026

View reviewed changes

Navigation Menu

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

port: drop a port's pending fd-event changes before it is freed#415
andypost wants to merge 2 commits into
masterfrom
fix/414-epoll-change-lifetime

andypost commented Sep 17, 2026 •

edited

Loading

Uh oh!

andypost commented Sep 18, 2026

Uh oh!

claude Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant


		nxt_fd_event_enable_write(engine, ev);

		if (nxt_slow_path(nxt_fd_event_change_test_nchanges(engine) != 1)) {

Conversation

andypost commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong

Reachability, reproduced

The fix

Tests

What this does not fix

Uh oh!

andypost commented Sep 18, 2026

Uh oh!

claude Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

1. The new test fails on kqueue platforms

2. On poll/devpoll/pollset, dropping a pending DELETE is not the same as committing it

3. The same change can outlive the descriptor without any free

4. The call-site guard is what forces the kqueue flag

5. port->engine is not guaranteed to be the engine that holds the change

6. Smaller things

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

andypost commented Sep 17, 2026 •

edited

Loading

claude Bot commented Sep 18, 2026 •

edited

Loading

5. `port->engine` is not guaranteed to be the engine that holds the change