Fix TLS sendq/partial-write handling, auth timer lifecycle, and backend close semantics - #99
Merged
Merged
Conversation
TLS short writes (SSL_MODE_ENABLE_PARTIAL_WRITE and the GnuTLS/libtls equivalents) could leave con_rexmit pointing mid-message while send_queued() advanced the MsgQ via msgq_delete(count_out). Finishing the remainder then called msgq_excise() with a mid-message pointer, which only matches the start of a MsgBuf: default builds abort(), and --disable-asserts builds leave the queue wrong and corrupt the TLS stream. Clients dropped with no IRC ERROR (after NOTICE AUTH, or under large NAMES / sendq bursts). Send path (all TLS backends + deliver_it): - Credit successful rexmit bytes through count_out so msgq_delete() advances the queue's own Msg.sent offset; con_rexmit is then always head->msg + head->sent. msgq_excise() is removed. - Drain short TLS writes until the message finishes or the backend reports a real block (WANT_WRITE / EAGAIN); return IO_BLOCKED only then. - Do not set FLAG_BLOCKED on TLS short IO_SUCCESS (that busy-loops ET_WRITE while POLLOUT stays ready). Still set it for plain short writev. Credit sendB on IO_BLOCKED when some bytes already went out. - Clear con_rexmit beside every MsgQClear (dead_link / close_connection / dealloc_connection). Tests: TLS auth-drop pipeline and NAMES-burst regressions (tls_stress marker, pytest-timeout dependency).
- destroy_auth_request() during timer_run() while the timeout timer is GEN_MARKED must not freelist the AuthRequest yet: timer_del() is a no-op while marked and timer_run() still owns the Timer. Defer with AR_FREE_PENDING and freelist on ET_DESTROY. Detach from the client first, and timer_del() before freelist reuse so a leftover queue link cannot form a self-loop that pegs timer_enqueue() at 100% CPU. - Split start_dns_ident_queries() from start_dns_ident() so start_auth() finishes auth exactly once. start_dns_ident() remains the deferred WebSocket resume path from s_bsd and finishes auth itself. Previously start_auth() called check_auth_finished() twice on one AuthRequest, which could use a freelisted request (SIGSEGV after client pointer nulling) when the first call completed or exited the client.
ircd_tls_negotiate() returned 1 (success) when s_tls() was already NULL, so FLAG_NEGOTIATING_TLS stayed set and start_auth() was invoked on every subsequent ET_WRITE. Clear the flag and return -1 in all three backends. OpenSSL: return an explicit 1 on handshake success; the previous 'return res' at the end of the function returned X509_digest()'s result, which reads as "still negotiating" when the digest fails.
- gnutls_record_recv() == 0 is a peer close -> IO_FAILURE. It was treated as IO_BLOCKED (gnutls_error_is_fatal(0) is false), leaving the session open until the client gave up on its SSL shutdown wait (30s for asyncio). - ircd_tls_close(): only send close_notify after a completed handshake, matching OpenSSL SSL_is_init_finished() and libtls tls_close(). ircd_tls_negotiate() marks success via gnutls_session_set_ptr(); gnutls_protocol_get_version() cannot be used for this since it is already set before any ClientHello arrives. A stalled handshake now yields TCP EOF instead of a TLS alert record. Tests: test_stalled_handshake_times_out asserts a plain EOF (no alert byte accepted); new test_stalled_handshake_after_clienthello_times_out drives the client side in a MemoryBIO so an encrypted TLS 1.3 alert after ServerHello is detected too. Verified on openssl, gnutls, libtls.
If gnutls_x509_crt_import() or gnutls_x509_crt_get_fingerprint() failed after a successful handshake, ircd_tls_negotiate() returned 1 without clearing FLAG_NEGOTIATING_TLS, so send_queued() never sent anything and the connection wedged until ping timeout. Log the error, leave the fingerprint empty and complete the handshake normally, as the OpenSSL and libtls backends do.
The outbound server-link path discarded the negotiate result and only looked at FLAG_NEGOTIATING_TLS. A handshake timeout (which returns -1 without clearing the flag) was read as "still negotiating" and the link sat until ping timeout; a missing session (flag cleared, -1) fell through to sending PASS/SERVER on a socket without TLS. Handle the result like tls_negotiate_client(): notify opers, close the TLS session and fail the connection.
- conftest: never abort the suite when the failure snapshot cannot be written (e.g. root-owned failures/ from a prior docker run); fall back under /tmp. - debug_support: snapshot whichever hub container is running (tls-hub, hub, limits) instead of hard-coding ircu-hub. - docker-compose: debug volume, ASAN options, seccomp:unconfined and core/nofile ulimits for ircd-tls-hub / ircd-tls-leaf, matching the other ircd services.
MrIron-no
force-pushed
the
fix/tls-partialsend
branch
from
August 24, 2026 16:06
29bdf73 to
b494af6
Compare
…l error Follow-up to the TLS PR, addressing review findings #1 and #2. #1 (correctness): the con_rexmit drain reported a raw byte count that send_queued() fed to msgq_delete(), which deletes in (partial-normal, prio, normal) order. When a whole normal message was deferred (sent==0) and a priority message (e.g. a check_pings PING) was enqueued before the next ET_WRITE, msgq_delete() deleted the never-sent PING first and left the normal message's tail to be re-sent as duplicate bytes -- desyncing the P10 link and dropping the PING. Restore identity-based removal: a corrected msgq_excise() (matching the head message by buffer containment, checking both queues) removes exactly the drained message, and its bytes are no longer credited to *count_out. The in-loop drain stays count-based (it sends in mapiov order within one synchronous call). #2 (disclosure): on a fatal mid-drain error the backend frees the TLS session and returned IO_FAILURE with count_out > 0; deliver_it() reported that as progress, so send_queued() looped back and flushed the rest of the sendq -- private messages included -- as plaintext on the raw socket. Zero *count_out on every IO_FAILURE return so deliver_it() reports no progress and the link dead-links immediately, matching pre-PR behaviour. Applied consistently to the OpenSSL, GnuTLS and libtls backends.
Follow-up to the TLS PR, addressing review findings #3 and #5. #3: completed_connection()'s TLS-failure branch freed the session but did not mark the client dead, so exit_client() flushed its ERROR line as plaintext into the half-open handshake stream. Set FLAG_DEADSOCKET so can_send() rejects the write, matching tls_negotiate_client(). #5: tls_handshake_succeeded() discarded completed_connection()'s return, so a link whose Connect block vanished during the handshake (rehash) was left half-initialized until the ping timeout. Exit the client on a 0 return, as the ET_CONNECT path already does.
…iring Follow-up to the TLS PR, addressing review finding #6. The defensive timer_del() in start_auth() was a no-op on a still-GEN_MARKED timer, after which memset() zeroed t_header links timer_run() still owned -- recreating the timer-enqueue self-loop (100% CPU) it was meant to prevent. AR_FREE_PENDING already guarantees a freelisted request's timer is fully destroyed before reuse, so assert that invariant rather than silently repairing a state that should never occur.
Follow-up to the TLS PR, addressing review findings #4 and #7. #4: the failure-snapshot guard caught only OSError, but the docker inspect/logs calls raise subprocess.TimeoutExpired (a SubprocessError) on a hung daemon -- turning every test failure into a pytest internal error. Catch subprocess.SubprocessError too. #7: the TLS stress tests open many concurrent connections from the single docker-network IP; add permissive IPCHECK_CLONE_LIMIT/PERIOD to the TLS hub config (matching the plaintext hub) so clone throttling does not fail them spuriously.
Covers review finding #1 at the C level. A normal message deferred to con_rexmit (sent == 0) with a priority message enqueued behind it must be removed by identity, leaving the priority message intact; the test also checks a mid-message con_rexmit pointer (multi-partial drain), excising a priority message, and the lone-message case. Wired into `make check` as msgq_excise_t. Verified to abort on the pre-fix count-based deletion, which removes the priority message in place of the drained one.
ircd_tls_negotiate() collapsed every failure to a bare -1, so operator notices and disconnect logs read only "TLS negotiation failed". It now fills a caller-provided reason buffer (TLS_REASON_LEN) with a specific, human-readable cause, propagated to the SNO_OLDSNO operator notice and the disconnect reason on both the inbound (tls_negotiate_client) and outbound (completed_connection) paths. A categorical ERROR line is still written to the peer where the channel is plaintext; TLS-layer rejections reach the peer as the backend's own TLS alert instead. Reasons per backend: - OpenSSL: X509_verify_cert_error_string() for a verification abort (SSL_get_verify_result() is set even when SSL_accept() fails), otherwise the OpenSSL error reason; plus certificate-required and handshake errors. - GnuTLS: gnutls_certificate_verification_status_print() for a bad verdict, gnutls_strerror() for the verify call or handshake error. - libtls: tls_error() for the handshake error. A stalled handshake still closes with a plain TCP EOF (no peer write, which would corrupt a mid-handshake peer's TLS stream). The server-link fingerprint-mismatch notice now reports the presented and configured fingerprints (m_server.c). Example operator notices: TLS negotiation failed from unknown server: self-signed certificate TLS negotiation failed from unknown server: certificate has expired TLS negotiation failed to leaf2.test.net: no shared cipher
show_ports() built its flag string in char flags[8], which was sized
for the original C/S + H + 4- + 6- letters (6 + NUL). The TLS ('E')
and Cloudflare ('F') additions push the worst case (hidden TLS
cloudflare listener with both address families unbound after a
rehash) to 8 characters plus NUL, one byte past the buffer. Bump it
to 16.
Add tests/stats_ports covering STATS p locally, hunted to a remote
server, after REHASH, and on a server with TLS/websocket/cloudflare
listeners.
Both crediting error paths in ircd_tls_sendv() zeroed *count_out before deciding whether the error was fatal, so a non-fatal error (e.g. a warning alert) mid-batch returned IO_BLOCKED with zero credit. send_queued() then deleted nothing from the sendq and the messages already handed to gnutls_record_send() were retransmitted on the next ET_WRITE, duplicating lines mid-stream. Zero the credit only on IO_FAILURE, matching the OpenSSL and libtls backends.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TLS clients could be dropped with no IRC
ERRORafterNOTICE AUTHor during large NAMES / sendq bursts. Root cause: TLS short writes (SSL_MODE_ENABLE_PARTIAL_WRITEand the GnuTLS/libtls equivalents) leftcon_rexmitpointing mid-message whilesend_queued()advanced the MsgQ viamsgq_delete(count_out); finishing the remainder then calledmsgq_excise()with a mid-message pointer, which only matches the start of a MsgBuf — default buildsabort(),--disable-assertsbuilds silently corrupt the queue and the TLS stream.Also fixes crashes/spins found under the same load in the auth/timer path, and aligns close/negotiate semantics across the three TLS backends.
Commits
con_rexmit) — credit partial bytes throughcount_outsomsgq_delete()advances the queue's ownMsg.sent;con_rexmitis then alwayshead->msg + head->sent. Drain short writes until a realWANT_WRITE/EAGAIN; don't setFLAG_BLOCKEDon TLS short success; clearcon_rexmitbeside everyMsgQClear.msgq_excise()removed. Regression tests:test_tls_auth_drop.py,test_tls_names_burst.py.check_auth_finished— defer freelisting while the timeout timer isGEN_MARKED(AR_FREE_PENDING); splitstart_dns_ident_queries()sostart_auth()finishes auth exactly once.!tls→ clear flag, return −1 (all backends); OpenSSL returns an explicit 1 on success instead ofX509_digest()'s result.record_recv()==0→IO_FAILURE;close_notifyonly after a completed handshake, tracked viagnutls_session_set_ptr()(gnutls_protocol_get_version()is set before any ClientHello, so it can't be used). Strict stall tests, including an encrypted-alert (TLS 1.3) variant.FLAG_NEGOTIATING_TLS, wedging the connection.ircd_tls_negotiate()failure incompleted_connection()— outbound links no longer sit until ping timeout on a handshake failure/timeout.Verification
tests/tls/(22 tests) pass on OpenSSL; also pass with everySSL_writecapped to 16 bytes via a local harness that forces thecon_rexmitpath on every message. The same harness onmainreproduces the bug: first pipelined TLS registration silently dropped, hub exits 134 (SIGABRT at themsgq_exciseassert).TLS_BACKEND=…images).Notes for reviewers
check_resolver_timeout()never armsres_timeout(t_expirestays 0), so DNS lookups are bounded only byAUTH_TIMEOUT. Not touched here; worth a separate issue. If it is ever fixed,restart_resolver()'s unguardedtimer_init(&res_timeout)needs a queued-check.Follow-up (review response)
A review of the 7 commits above raised seven findings; all are addressed in the commits that follow, plus a regression test and an operator-facing improvement.
Review fixes
c794e9cTLS send path — "Bogus userid" error if identd is turned off and nothing to slow it down #1 the drainedcon_rexmitmessage is now removed by identity (restored, correctedmsgq_excise) instead of by a byte countmsgq_delete()could misattribute to a priority message that jumped ahead (duplicate bytes / dropped PING); execvp result gets swallowed in s_auth #2 everyIO_FAILUREreturn zeroescount_outsosend_queued()dead-links immediately instead of flushing the rest of the sendq as plaintext.0d1aab7s_bsd — gitignore: Add auto generated files #3completed_connection()'s TLS-failure branch setsFLAG_DEADSOCKET(no plaintext ERROR into the half-open handshake); channel mode +M #5tls_handshake_succeeded()acts oncompleted_connection()'s return instead of leaving a half-set-up link until ping timeout.7134d2es_auth — channel mode +P #6 thestart_auth()freelist guard is replaced with an assert of the invariantAR_FREE_PENDINGguarantees (the guard could re-create the 100% CPU timer spin it was meant to prevent).79a0450tests — Hello this possible add the spam #4 the debug-snapshot guard also catchessubprocess.SubprocessError(a hung docker daemon no longer aborts the suite); idle reset for CPRIVMSG command #7 the TLS hub config gets permissiveIPCHECK_CLONE_LIMIT/PERIODso the concurrent stress connections from one docker IP are not throttled.Added
c17721aC unit test formsgq_excise(make check) — reproduces the "Bogus userid" error if identd is turned off and nothing to slow it down #1 prio-reorder at the queue level; verified to abort on the pre-fix count-based deletion and pass with the fix.873ab37Detailed TLS failure reasons —ircd_tls_negotiate()fills a reason buffer with a specific cause, surfaced to theSNO_OLDSNOoperator notice and disconnect log on both the inbound and outbound paths (peer still gets a categorical ERROR / the backend TLS alert; a stalled handshake still closes with a plain EOF). Examples:TLS negotiation failed from unknown server: self-signed certificateTLS negotiation failed from unknown server: certificate has expiredTLS fingerprint mismatch for server X: presented <fp>, configured <fp>Verification: full
tests/tls/suite (74 tests) passes on openssl, gnutls and libtls after these commits; operator/peer notices verified against live openssl and gnutls hubs.