Skip to content

fix: close the tls server config a listen loop built - #688

Merged
kacy merged 3 commits into
mainfrom
fix-server-tls-config-close
Aug 9, 2026
Merged

fix: close the tls server config a listen loop built#688
kacy merged 3 commits into
mainfrom
fix-server-tls-config-close

Conversation

@kacy

@kacy kacy commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Every std tls server built a config from its cert and key pem files and never closed it, so the certificate pem and the private key der stayed in the tls registry for the life of the process. A program that restarts its listeners in place paid that cost once per restart.

The ownership rule is the one the client side already settled on: whoever builds a config closes it. tls.listen records the handle against the listening socket, but that is a borrow rather than a transfer, so Listener.close() is right to give back only the borrow, and the three creators are the ones that owe a close — App.listen_tls, listen_h2_tls, and its streaming twin.

What makes a server different is the timing. An accept loop spawns a task per connection, and that task reads the certificate and the key out of the registry when its own handshake reaches them, which can be a whole handshake timeout after the accept that spawned it returned. native_server_handshake blocks on the client hello first and only calls native_server_certificate_chain and native_server_private_key after it arrives, so the gap between "the accept loop stopped" and "this connection still needs the key" is up to the 10s handshake timeout. The close therefore goes after shutdown.drain_default() rather than into a defer, because a defer would also fire on the give-up path, where nothing drains and a task can be mid-handshake. The bind-failure exit closes as well: nothing has borrowed the config at that point, and a server retrying a taken port would otherwise leak a certificate and a key per attempt.

Two things came out of the investigation that were worth fixing alongside. docs/tls.md claimed the listener owned its server config and that Listener.close() released it, which is not what the code does and is a plausible reason the three creators never closed; that section now carries the server rule and the ordering, with an example. And tls.open_server_configs() is new — it counts the server configs still holding a certificate, which makes this class of leak observable from outside the module and is what the new test asserts on.

The give-up path (back_off_after_accept_failure propagating after a run of accept failures) deliberately still leaves the config open, with a comment saying so. There is no drain on that path to wait behind, and turning in-flight handshakes into failures is worse than one open config on a path that is already ending the server.

what was tested

The new tests/cases/test_tls_server_config_release.pith runs a real App.listen_tls and proves both halves of the rule in one pass. It opens a tcp connection, waits for shutdown.inflight() to confirm the server has taken it, calls shutdown.request(), and only then sends the client hello — so the handshake happens strictly inside the drain. It asserts the handshake succeeds, that the config is still open while that connection is served, that the request gets a 200, and that open_server_configs() is back to zero once listen_tls returns. Nothing waits on a fixed sleep; the connect retries until the listener is bound and the ordering is read off shutdown.inflight().

The test was falsified three ways against deliberately broken versions of listen_tls, so it is known to fail when the fix is absent or misplaced:

  • no close at all (the bug as filed) — open server configs after: 1
  • close placed before the drain — handshake after the shutdown request: false, unexpected eof
  • the shipped version — passes

It matches its golden output under os threads, green with default workers, and green with a single worker.

Two claims behind the ordering were measured rather than argued. A probe confirmed pith evaluates the return expression before firing a defer, so defer after return shutdown.drain_default() would run post-drain; the explicit form is used anyway, because the give-up path needs to be excluded. A second probe showed shutdown.drain_default() does not wait for a task that has been spawned but has not yet reached shutdown.enter() — it returns 0 immediately. That gap does not endanger this change (such a task has not read the listener's config either, and fails cleanly on the mapping the accept loop already dropped), but it is a real pre-existing hole in the drain and is written up below.

make run-regressions-only is 329/329. make memcheck is clean, with the new case added to MEMCHECK_CASES — it is the only test that drives a tls listen loop through a full shutdown, which is exactly where a use after close would land. pith test std/net/tls.pith is 9/9, including a new "closing a server config empties every registry map it wrote to" that mirrors the existing client-side one.

noticed, not fixed

shutdown.drain_default() can return before a connection it should have waited for. The enter() that makes a connection visible to the drain runs inside the spawned task, so a connection accepted in the last moments before a shutdown can be dropped without the drain ever counting it. Moving enter() into the accept loop, before the spawn, would close it. Separately, the accept loops release the listener's config borrow before draining, so a task that has entered but has not yet read the mapping loses its handshake during a shutdown; the window is a few non-blocking instructions wide, and an attempt to observe it did not, so it is left alone here rather than changed on argument.

kacy added 3 commits August 9, 2026 16:14
every std tls server built a config from its cert and key pem files and never
closed it, so the certificate pem and the private key der stayed in the tls
registry for the life of the process. a program that restarts its listeners in
place paid that cost once per restart.

the ownership rule is the same one the client side settled on: whoever builds a
config closes it. tls.listen records the handle against the listening socket,
but that is a borrow, not a transfer, so Listener.close() is right to give back
only the borrow and the three creators are the ones that owe a close --
App.listen_tls, listen_h2_tls, and listen_h2_tls_streaming.

what makes a server different is when. an accept loop spawns a task per
connection, and that task reads the certificate and the key out of the registry
when its own handshake reaches them, which can be a whole handshake timeout
after the accept returned. so the close goes after the drain, not in a defer
that would also fire on the give-up path where nothing drains. the bind-failure
exit closes too: nothing has borrowed the config at that point.

adds tls.open_server_configs() -- how many server configs still hold a
certificate -- so a leak is observable from outside the module, and documents
the server half of the rule in docs/tls.md, which until now claimed the listener
owned the config.
the change it covers closes a config that spawned handshake tasks read from,
so a use after close is the failure it carries. valgrind is what would see one.
@kacy
kacy merged commit e2841af into main Aug 9, 2026
2 checks passed
@kacy
kacy deleted the fix-server-tls-config-close branch August 9, 2026 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant