From 6e2d911004257b2b6384e3a12ad0c050275f4ed6 Mon Sep 17 00:00:00 2001 From: Richard Wheeler Date: Thu, 25 Jun 2026 15:44:24 -0400 Subject: [PATCH] net.http: run TLS server handshakes on worker threads (#27433 item 3) (#27552) The TLS handshake ran inline on the single accept thread, so handshakes were serialized and a client that completed the TCP connect but stalled mid-handshake wedged the accept loop and delayed stop() by up to the handshake budget. This was previously deferred because concurrent handshakes would race on the listener's shared mbedtls config/RNG/key, but #27437 enabled MBEDTLS_THREADING_C on all platforms, so that shared state is now mutex-protected and each worker already handshakes on its own per-connection ssl context. Split SSLListener.accept_with_timeouts into accept_raw_with_timeout (raw TCP + ssl setup, no handshake) and a new SSLConn.complete_handshake; the accept thread now only does the fast raw accept (polled at 100ms) and each worker performs its connection handshake. The handshaking fd is idle-tracked, so close_idle can interrupt a stalled handshake and stop() is observed within ~accept_poll_timeout regardless of handshakes in flight. The accept thread enqueues each raw conn through an interruptible select (poll s.state) rather than a bare `ch <- conn`: the send, and the post-loop shutdown (ch.close + close_idle + ws.wait), all run on the accept thread while stop() only flips s.state from another thread. A slow-handshake flood that fills every worker and the channel buffer would otherwise block the accept thread on the send so it never reaches close_idle, delaying shutdown until a worker handshake timed out. On shutdown the accept thread closes the not-yet-queued conn itself (it was never idle-tracked). Supporting changes: - SSLConn.shutdown() is now idempotent (clears opened before freeing), so a worker-defer vs close_idle double-shutdown is a no-op rather than a double-free. - do_handshake_loop holds the handshake retry loop with no self-shutdown; server_handshake keeps the synchronous self-shutdown wrapper for accept(). - TlsIdleConnTracker.mark_idle dedups handles so close_idle cannot close the same fd twice within its own loop. Tests: double-shutdown idempotency guard; a parallel concurrent-handshake round trip; and a slow-handshake-flood shutdown test (a one-worker, one-slot server flooded with stalled handshakes must still stop() promptly, not wait out the handshake timeout). Verified on Windows and Linux (gcc). Co-authored-by: WOZCODE --- vlib/net/http/server_tls_idle.v | 8 +- vlib/net/http/server_tls_notd_use_openssl.v | 66 ++++++-- vlib/net/http/server_tls_test.v | 146 ++++++++++++++++++ ...tls_sslconn_shutdown_does_not_panic_test.v | 8 + vlib/net/mbedtls/ssl_connection.c.v | 72 ++++++--- 5 files changed, 268 insertions(+), 32 deletions(-) diff --git a/vlib/net/http/server_tls_idle.v b/vlib/net/http/server_tls_idle.v index e846bc3dd..366b9f193 100644 --- a/vlib/net/http/server_tls_idle.v +++ b/vlib/net/http/server_tls_idle.v @@ -28,7 +28,13 @@ fn (mut t TlsIdleConnTracker) mark_idle(handle int) bool { if t.closing { return false } - t.handles << handle + // Dedup: the same handle can be marked twice (e.g. a worker marks it for the + // handshake window and again per keep-alive request, or the OS recycles the + // fd value). A duplicate would make close_idle shut down / close the same fd + // twice within its own loop, so only track each handle once. + if t.handles.index(handle) < 0 { + t.handles << handle + } return true } diff --git a/vlib/net/http/server_tls_notd_use_openssl.v b/vlib/net/http/server_tls_notd_use_openssl.v index 1d19e4842..ad0876123 100644 --- a/vlib/net/http/server_tls_notd_use_openssl.v +++ b/vlib/net/http/server_tls_notd_use_openssl.v @@ -12,9 +12,11 @@ const tls_accept_poll_timeout = 100 * time.millisecond // tls_handshake_timeout is the default value for Server.tls_handshake_timeout, // used as the fallback handshake budget when Server.accept_timeout is zero or -// net.infinite_timeout. The handshake runs on the accept thread, so without a -// finite bound a client that completes the TCP connect and then stalls -// mid-handshake would wedge the accept loop forever. +// net.infinite_timeout. The handshake now runs on a worker thread (the accept +// thread only does the raw TCP accept, polled at tls_accept_poll_timeout, so +// stop() is observed promptly regardless of handshakes); without a finite bound a +// client that completes the TCP connect and then stalls mid-handshake would tie +// up a worker indefinitely. const tls_handshake_timeout = 30 * time.second fn tls_accept_timeouts(accept_timeout time.Duration, handshake_fallback time.Duration) (time.Duration, time.Duration) { @@ -77,11 +79,14 @@ fn (mut s Server) listen_and_serve_tls() { } s.addr = addr + accept_poll_timeout, handshake_timeout := tls_accept_timeouts(s.accept_timeout, + s.tls_handshake_timeout) ch := chan &mbedtls.SSLConn{cap: s.pool_channel_slots} mut idle_conns := &TlsIdleConnTracker{} mut ws := []thread{cap: s.worker_num} for wid in 0 .. s.worker_num { - ws << new_tls_handler_worker(wid, ch, s.handler, s.max_keep_alive_requests, idle_conns) + ws << new_tls_handler_worker(wid, ch, s.handler, s.max_keep_alive_requests, + handshake_timeout, idle_conns) } if s.show_startup_message { @@ -94,10 +99,8 @@ fn (mut s Server) listen_and_serve_tls() { if s.on_running != unsafe { nil } { s.on_running(mut s) } - accept_poll_timeout, handshake_timeout := tls_accept_timeouts(s.accept_timeout, - s.tls_handshake_timeout) for s.state == .running { - mut conn := listener.accept_with_timeouts(accept_poll_timeout, handshake_timeout) or { + mut conn := listener.accept_raw_with_timeout(accept_poll_timeout) or { if s.state != .running { break } @@ -109,7 +112,32 @@ fn (mut s Server) listen_and_serve_tls() { } continue } - ch <- conn + // Hand the raw (not-yet-handshaked) conn to a worker. Don't use a bare + // blocking `ch <- conn`: the accept loop, this send, and the post-loop + // shutdown (ch.close + close_idle + ws.wait) all run on this one thread, + // while stop()/close() only flip s.state from another thread. Under a slow- + // handshake flood -- every worker blocked in complete_handshake and the + // channel buffer full of untracked conns -- a blocking send would wedge the + // accept thread so it never re-checks s.state nor reaches close_idle(), + // delaying shutdown until a worker handshake times out. Poll s.state via a + // select timeout so shutdown is still observed promptly. + mut queued := false + for s.state == .running && !queued { + select { + ch <- conn { + queued = true + } + accept_poll_timeout { + // channel full; loop re-checks s.state + } + } + } + if !queued { + // Shutting down before this conn could be handed off. It was never + // mark_idle'd, so close_idle() won't see it; close it here (exactly + // once -- no worker ever received it) to avoid leaking the fd. + conn.shutdown() or {} + } } ch.close() idle_conns.close_idle() @@ -121,18 +149,20 @@ struct TlsHandlerWorker { id int ch chan &mbedtls.SSLConn max_keep_alive_requests int + handshake_timeout time.Duration mut: idle_conns &TlsIdleConnTracker = unsafe { nil } pub mut: handler Handler } -fn new_tls_handler_worker(wid int, ch chan &mbedtls.SSLConn, handler Handler, max_keep_alive_requests int, idle_conns &TlsIdleConnTracker) thread { +fn new_tls_handler_worker(wid int, ch chan &mbedtls.SSLConn, handler Handler, max_keep_alive_requests int, handshake_timeout time.Duration, idle_conns &TlsIdleConnTracker) thread { mut w := &TlsHandlerWorker{ id: wid ch: ch handler: handler max_keep_alive_requests: max_keep_alive_requests + handshake_timeout: handshake_timeout idle_conns: idle_conns } return spawn w.process_requests() @@ -171,6 +201,24 @@ fn (mut w TlsHandlerWorker) handle_conn(mut conn mbedtls.SSLConn) { } conn.shutdown() or {} } + // Run the TLS handshake here on the worker thread (the accept thread only does + // the raw TCP accept), so handshakes proceed in parallel up to worker_num and a + // client that stalls mid-handshake can't wedge the accept loop or delay stop(). + // mark_idle first so close_idle can interrupt a stalled handshake by force- + // closing the fd, and so a conn handed off while the server is shutting down is + // closed (mark_idle returns false) rather than handshaked. On success, unmark + // before the serve path does its own idle marking, keeping each handle tracked + // once. On any early return the defer above performs the single shutdown. + if !w.idle_conns.mark_idle(conn.handle) { + return + } + conn.complete_handshake(w.handshake_timeout) or { + $if debug { + eprintln('TLS handshake failed: ${err}') + } + return + } + w.idle_conns.unmark_idle(conn.handle) // If the TLS handshake negotiated HTTP/2 via ALPN, switch to the HTTP/2 // driver; otherwise fall through to the existing HTTP/1.1 path unchanged. if conn.negotiated_alpn() == 'h2' { diff --git a/vlib/net/http/server_tls_test.v b/vlib/net/http/server_tls_test.v index 3e6fbd8db..7ea1ca221 100644 --- a/vlib/net/http/server_tls_test.v +++ b/vlib/net/http/server_tls_test.v @@ -269,6 +269,86 @@ fn test_server_tls_close_during_silent_handshake() { assert srv.status() == .closed } +// test_server_tls_close_under_handshake_flood guards the case where every worker +// is stuck in a slow TLS handshake AND the channel buffer is full of untracked +// raw conns, so the accept thread parks on `ch <- conn`. If the accept thread +// could not poll s.state during that send it would never reach close_idle(), and +// close() would hang until a worker handshake timed out. Each client sends a +// valid ClientHello then stalls, so the server's handshake blocks waiting for the +// client's next flight (a bare connect would instead fail the handshake fast and +// free the worker). With one worker and a one-slot buffer this fills: conn 0 -> +// the worker, conn 1 -> the buffer, conn 2 -> the blocked send. +fn test_server_tls_close_under_handshake_flood() { + $if use_openssl ? { + eprintln('skipping: TLS server not implemented for -d use_openssl yet') + return + } + port := pick_port() or { + assert false, 'pick_port: ${err}' + return + } + // A real 165-byte TLS 1.2 ClientHello captured from V's own mbedtls client. + // The exact bytes (random/session id) are irrelevant; the server only needs a + // structurally valid first flight to start the handshake and then block + // waiting for the client's ClientKeyExchange, which never arrives. + client_hello := [u8(0x16), 0x03, 0x03, 0x00, 0xa0, 0x01, 0x00, 0x00, 0x9c, 0x03, 0x03, 0x6a, + 0x3d, 0x34, 0x4c, 0x8f, 0x46, 0x64, 0xd8, 0xe9, 0x2d, 0x46, 0x16, 0xd3, 0xf2, 0x87, 0xe4, + 0xee, 0xc5, 0x57, 0x6c, 0x8e, 0xd7, 0x36, 0x19, 0x3b, 0x35, 0x7a, 0x01, 0x03, 0xde, 0x6e, + 0x64, 0x00, 0x00, 0x24, 0xc0, 0x2c, 0xc0, 0x2b, 0xc0, 0x30, 0xc0, 0x2f, 0xc0, 0x24, 0xc0, + 0x23, 0xc0, 0x28, 0xc0, 0x27, 0xc0, 0x0a, 0xc0, 0x09, 0xc0, 0x14, 0xc0, 0x13, 0x00, 0x9d, + 0x00, 0x9c, 0x00, 0x3d, 0x00, 0x3c, 0x00, 0x35, 0x00, 0x2f, 0x01, 0x00, 0x00, 0x4f, 0x00, + 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18, 0x00, 0x0b, 0x00, 0x02, + 0x01, 0x00, 0x00, 0x0d, 0x00, 0x1a, 0x00, 0x18, 0x08, 0x04, 0x08, 0x05, 0x08, 0x06, 0x04, + 0x01, 0x05, 0x01, 0x02, 0x01, 0x04, 0x03, 0x05, 0x03, 0x02, 0x03, 0x02, 0x02, 0x06, 0x01, + 0x06, 0x03, 0x00, 0x23, 0x00, 0x00, 0x00, 0x10, 0x00, 0x0e, 0x00, 0x0c, 0x02, 0x68, 0x32, + 0x08, 0x68, 0x74, 0x74, 0x70, 0x2f, 0x31, 0x2e, 0x31, 0x00, 0x17, 0x00, 0x00, 0xff, 0x01, + 0x00, 0x01, 0x00] + mut srv := &http.Server{ + addr: '127.0.0.1:${port}' + cert: server_tls_cert + cert_key: server_tls_key + in_memory_verification: true + // A finite accept_timeout doubles as the handshake budget (see + // tls_accept_timeouts), so use a large one: each stalled handshake must + // stay stuck long enough to keep the worker busy and form the wedge, + // rather than time out in milliseconds. The accept loop still polls at the + // 100ms tls_accept_poll_timeout cap regardless. + accept_timeout: 8 * time.second + worker_num: 1 + pool_channel_slots: 1 + handler: EchoHandler{} + show_startup_message: false + } + t := spawn srv.listen_and_serve() + srv.wait_till_running() or { + srv.close() + t.wait() + assert false, 'server failed to start: ${err}' + return + } + mut clients := []&net.TcpConn{} + for _ in 0 .. 4 { + mut c := net.dial_tcp('127.0.0.1:${port}') or { continue } + c.write(client_hello) or {} + clients << c + } + defer { + for mut c in clients { + c.close() or {} + } + } + // Let the accept thread occupy the worker and buffer and park on the send. + time.sleep(400 * time.millisecond) + sw := time.new_stopwatch() + srv.close() + t.wait() + // close_idle() force-closes the stuck handshake fds, so close() returns well + // before the 8s handshake timeout — but only if the accept thread escaped the + // blocked send to reach it. + assert sw.elapsed() < 2 * time.second + assert srv.status() == .closed +} + fn test_server_tls_close_interrupts_idle_keep_alive() { $if use_openssl ? { eprintln('skipping: TLS server not implemented for -d use_openssl yet') @@ -479,6 +559,72 @@ fn test_server_tls_close_interrupts_incomplete_h2_request() { assert srv.status() == .closed } +fn test_server_tls_parallel_handshakes() { + $if use_openssl ? { + eprintln('skipping: TLS server not implemented for -d use_openssl yet') + return + } + port := pick_port() or { + assert false, 'pick_port: ${err}' + return + } + mut srv := &http.Server{ + addr: '127.0.0.1:${port}' + cert: server_tls_cert + cert_key: server_tls_key + in_memory_verification: true + accept_timeout: time.second + handler: EchoHandler{} + show_startup_message: false + } + t := spawn srv.listen_and_serve() + srv.wait_till_running() or { + srv.close() + t.wait() + assert false, 'server failed to start: ${err}' + return + } + defer { + srv.close() + t.wait() + } + time.sleep(50 * time.millisecond) + + // Fire many clients at once. Handshakes now run on the worker pool, so they + // proceed concurrently against the listener's shared mbedtls config/RNG (safe + // because MBEDTLS_THREADING_C is enabled on all platforms). Assert every + // round-trip succeeds with the correct body — a thread-safety regression in + // the shared handshake state would surface as a failed/garbled response. + n := 16 + results := chan string{cap: n} + for i in 0 .. n { + spawn fn [results, port, i] () { + resp := http.fetch( + url: 'https://127.0.0.1:${port}/p${i}' + enable_http2: false + validate: false + ) or { + results <- 'error: ${err}' + return + } + if resp.status_code != 200 { + results <- 'bad status: ${resp.status_code}' + return + } + results <- resp.body + }() + } + // Results arrive in nondeterministic order, so match on the common prefix + // rather than a specific path index. + mut ok := 0 + for _ in 0 .. n { + body := <-results + assert body.starts_with('tls hello /p'), 'unexpected response: ${body}' + ok++ + } + assert ok == n, 'expected ${n} successful concurrent handshakes, got ${ok}' +} + fn test_server_tls_h2_negotiation() { $if use_openssl ? { eprintln('skipping: TLS server not implemented for -d use_openssl yet') diff --git a/vlib/net/mbedtls/mbedtls_sslconn_shutdown_does_not_panic_test.v b/vlib/net/mbedtls/mbedtls_sslconn_shutdown_does_not_panic_test.v index 66b8be471..302fb5a49 100644 --- a/vlib/net/mbedtls/mbedtls_sslconn_shutdown_does_not_panic_test.v +++ b/vlib/net/mbedtls/mbedtls_sslconn_shutdown_does_not_panic_test.v @@ -20,6 +20,14 @@ fn server() ! { } eprintln('[+] Accepted connection') cli.shutdown()! + // A second shutdown must be a harmless no-op (idempotent): SSLConn.shutdown + // marks the conn closed before freeing, so this returns the "not open" error + // here rather than double-freeing the mbedtls contexts (which would abort the + // process). This guards the worker-defer-vs-close_idle double-shutdown race. + mut second_shutdown_errored := false + cli.shutdown() or { second_shutdown_errored = true } + assert second_shutdown_errored, 'second shutdown should be a no-op returning an error, not a double-free' + eprintln('[+] Second shutdown was a clean no-op') } @[if network ?] diff --git a/vlib/net/mbedtls/ssl_connection.c.v b/vlib/net/mbedtls/ssl_connection.c.v index dccd924dc..20c138e98 100644 --- a/vlib/net/mbedtls/ssl_connection.c.v +++ b/vlib/net/mbedtls/ssl_connection.c.v @@ -380,29 +380,22 @@ fn (mut l SSLListener) accept_tcp_connection() !&SSLConn { return conn } -fn (mut conn SSLConn) server_handshake(timeout time.Duration) ! { - deadline := ssl_timeout_deadline(timeout) +// do_handshake_loop drives the non-blocking TLS server handshake to completion +// (or `deadline`). It never calls shutdown: on any error it just returns, leaving +// cleanup to the caller. server_handshake wraps it for the synchronous accept +// path (self-shutdown on error); the threaded server path (complete_handshake) +// lets its worker defer own the single shutdown instead. +fn (mut conn SSLConn) do_handshake_loop(deadline time.Time) ! { mut ret := C.mbedtls_ssl_handshake(&conn.ssl) for ret != 0 { match ret { C.MBEDTLS_ERR_SSL_WANT_READ { - conn.wait_for_read(ssl_remaining_timeout(deadline)) or { - conn.shutdown() or {} - return err - } + conn.wait_for_read(ssl_remaining_timeout(deadline))! } C.MBEDTLS_ERR_SSL_WANT_WRITE { - conn.wait_for_write(ssl_remaining_timeout(deadline)) or { - conn.shutdown() or {} - return err - } + conn.wait_for_write(ssl_remaining_timeout(deadline))! } else { - conn.shutdown() or { - $if trace_ssl ? { - eprintln('${@METHOD} shutdown ---> res: ${err}') - } - } return error_with_code('net.mbedtls SSLListener.accept, mbedtls_ssl_handshake failed 1; handshake ret: ${ret}', ret) } @@ -412,14 +405,29 @@ fn (mut conn SSLConn) server_handshake(timeout time.Duration) ! { } } +fn (mut conn SSLConn) server_handshake(timeout time.Duration) ! { + deadline := ssl_timeout_deadline(timeout) + conn.do_handshake_loop(deadline) or { + conn.shutdown() or { + $if trace_ssl ? { + eprintln('${@METHOD} shutdown ---> res: ${err}') + } + } + return err + } +} + // accept_with_timeout waits up to `timeout` for a new client before accepting it. pub fn (mut l SSLListener) accept_with_timeout(timeout time.Duration) !&SSLConn { return l.accept_with_timeouts(timeout, timeout) } -// accept_with_timeouts waits up to `accept_timeout` for a new client, then -// waits up to `handshake_timeout` for the TLS server handshake to complete. -pub fn (mut l SSLListener) accept_with_timeouts(accept_timeout time.Duration, handshake_timeout time.Duration) !&SSLConn { +// accept_raw_with_timeout waits up to `accept_timeout` for a new client, accepts +// the raw TCP connection and sets up its (non-blocking) SSL context, but does NOT +// perform the TLS handshake. The returned conn is non-blocking with a non-blocking +// bio; the caller must run conn.complete_handshake to finish negotiation. This lets +// the threaded server accept on one thread and handshake on a worker thread. +pub fn (mut l SSLListener) accept_raw_with_timeout(accept_timeout time.Duration) !&SSLConn { wait_for(l.server_fd.fd, .read, accept_timeout)! mut conn := l.accept_tcp_connection()! @@ -437,13 +445,29 @@ pub fn (mut l SSLListener) accept_with_timeouts(accept_timeout time.Duration, ha } C.v_mbedtls_ssl_set_bio_nonblocking(&conn.ssl, &conn.server_fd) - conn.server_handshake(handshake_timeout)! - net.set_blocking(conn.handle, true) or { + return conn +} + +// complete_handshake finishes the TLS server handshake on a conn returned by +// accept_raw_with_timeout, waiting up to `timeout`, then restores blocking mode +// and the blocking bio. It never calls shutdown: on any error it returns and the +// caller owns cleanup (so the conn is shut down exactly once, by the caller). +pub fn (mut conn SSLConn) complete_handshake(timeout time.Duration) ! { + deadline := ssl_timeout_deadline(timeout) + conn.do_handshake_loop(deadline)! + net.set_blocking(conn.handle, true)! + C.mbedtls_ssl_set_bio(&conn.ssl, &conn.server_fd, C.mbedtls_net_send, C.mbedtls_net_recv, + C.mbedtls_net_recv_timeout) +} + +// accept_with_timeouts waits up to `accept_timeout` for a new client, then +// waits up to `handshake_timeout` for the TLS server handshake to complete. +pub fn (mut l SSLListener) accept_with_timeouts(accept_timeout time.Duration, handshake_timeout time.Duration) !&SSLConn { + mut conn := l.accept_raw_with_timeout(accept_timeout)! + conn.complete_handshake(handshake_timeout) or { conn.shutdown() or {} return err } - C.mbedtls_ssl_set_bio(&conn.ssl, &conn.server_fd, C.mbedtls_net_send, C.mbedtls_net_recv, - C.mbedtls_net_recv_timeout) return conn } @@ -546,6 +570,10 @@ pub fn (mut s SSLConn) shutdown() ! { if !s.opened { return error('net.mbedtls SSLConn.shutdown, connection was not open') } + // Mark closed before freeing so a second shutdown (e.g. a worker defer racing + // close_idle) is a harmless no-op rather than a double-free of the mbedtls + // contexts below. + s.opened = false if unsafe { s.certs != nil } { C.mbedtls_x509_crt_free(&s.certs.cacert) C.mbedtls_x509_crt_free(&s.certs.client_cert) -- 2.39.5