From f0decfb081c5089939ca80a140212485bcdb1694 Mon Sep 17 00:00:00 2001 From: Richard Wheeler Date: Sun, 28 Jun 2026 08:22:51 -0400 Subject: [PATCH] net.http,net.mbedtls: harden TLS parallel-handshake + idempotent-shutdown test guards (#27559) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * net.http,net.mbedtls: harden TLS parallel-handshake + idempotent-shutdown test guards Follow-up to #27552 (run TLS server handshakes on worker threads). Test-only; no production change. server_tls_test.v: test_server_tls_parallel_handshakes used a non-blocking EchoHandler, so it passed even if handshakes ran fully serially (the accept loop accepts one conn at a time and worker_num can be 1 on low-core CI) - it only proved the shared mbedtls config/RNG survived serial handoff. Now force worker_num:4 and use a width-2 barrier: BlockingHandler signals on entry then parks on release, and the test refuses to release until >=2 handlers are parked at once. That can only happen if >=2 workers each completed a handshake and ran concurrently; with a single worker the 2nd arrival never comes and the overlap wait times out, failing the test (verified: worker_num:1 fails the overlap assert, worker_num:4 passes). Parking frees the worker, so overlap is observable even on a single core. mbedtls_sslconn_shutdown_does_not_panic_test.v: the double-shutdown check only asserted the 2nd shutdown returned *an* error; a reintroduced double-free could still error on a forgiving allocator. Assert the specific "connection was not open" sentinel so the test proves the idempotent !opened early-return path was taken, not a second pass through the mbedtls frees. Co-Authored-By: WOZCODE * net.http: gate parallel-handshake test on handshake-layer concurrency The parallel-handshakes test counted overlapping request handlers, which only proves two handlers ran at once — a serial-accept-then-queue design produces that too (the accept thread completes handshakes one after another and hands both finished connections to workers that park together). It therefore could not catch the regression it was meant to guard (Codex review on #27559, discussion_r3478571393). Move the discriminator to the handshake layer: wedge `stall` workers with raw TCP connections that connect but never send a ClientHello (each blocks inside complete_handshake), then fire `live` real HTTPS clients. With per-worker parallel handshakes the free workers service the live clients in well under a second; a serial-accept regression wedges the accept thread on the first stalled connection and cannot service the live clients until a stalled handshake hits its 10s budget. The check is time-bounded (live phase < 6s) so the regression fails the test instead of merely running ~20s slower. Verified: good path (worker_num 4) passes repeatedly at sub-second live phase; forcing the pool below stall+live reproduces the regression (live phase 19.6s, fails the 6s bound). Co-Authored-By: WOZCODE * net.http: force fresh TLS connection per live client in handshake test The two live clients in the parallel-handshake test used the shared default transport's keep-alive pool. If one client finished and returned its connection to the pool before the other checked one out, the second would reuse it and the test could pass after only a single live TLS handshake — no longer proving two concurrent completing handshakes against the shared mbedtls config/RNG (Codex review on #27559, discussion_r3484264670). Set disable_connection_reuse on the live fetches so each opens its own TLS connection (and sends Connection: close), guaranteeing two genuine concurrent handshakes. Still green; live phase stays sub-second, well under the 6s bound. Co-Authored-By: WOZCODE * net.http: bound parallel-handshake wait with a select deadline The previous version measured elapsed time and asserted it was < 6s only after draining the results channel. On the serialized-handshake regression path that drain blocked on the client's own TLS handshake: http.fetch (Request.ssl_do) dials and completes the handshake before req.read_timeout applies and retries on socket errors, so the live clients sat in the SSL backend handshake timeout (and retries) for ~19s before the assert ran. The test still failed, but slowly (Codex review on #27559, discussion_r3484599003). Bound the wait itself: collect the live results inside a single 6s budget via a select deadline, so a regression fails promptly at ~6s instead of after the client timeout. Also set max_retries: 1 on the live fetches so a regressed handshake surfaces as one timed-out request rather than retry-amplifying the teardown. Verified: good path passes (sub-second live phase); the regression sim now trips the assert at the 6s budget ("got 0") instead of ~19s, cutting the failing-run time roughly in half. Co-Authored-By: WOZCODE --------- Co-authored-by: WOZCODE --- vlib/net/http/server_tls_test.v | 104 ++++++++++++++---- ...tls_sslconn_shutdown_does_not_panic_test.v | 18 ++- 2 files changed, 97 insertions(+), 25 deletions(-) diff --git a/vlib/net/http/server_tls_test.v b/vlib/net/http/server_tls_test.v index 7ea1ca221..c489ced72 100644 --- a/vlib/net/http/server_tls_test.v +++ b/vlib/net/http/server_tls_test.v @@ -568,12 +568,33 @@ fn test_server_tls_parallel_handshakes() { assert false, 'pick_port: ${err}' return } + // Prove handshakes run in parallel on the worker threads, not serially on the + // accept thread. The discriminator is placed at the HANDSHAKE layer, not at the + // request handler: a barrier inside the handler only proves two *handlers* + // overlapped, which a serial-accept-then-queue design also produces (the accept + // thread can complete two handshakes one after another and hand both completed + // connections to workers that then park together). Counting overlapping handlers + // would therefore miss exactly the regression we care about. + // + // Instead, occupy `stall` workers with raw TCP connections that complete the TCP + // connect but never send a ClientHello, so each one wedges a worker *inside* + // complete_handshake (blocked reading the handshake). Then fire `live` real HTTPS + // clients. If handshakes run per-worker in parallel, the remaining free workers + // service the live clients concurrently and they succeed. If the implementation + // regresses to handshaking serially on the accept thread, the very first stalled + // connection wedges that thread mid-handshake and no later connection — stalled or + // live — is ever accepted/serviced, so every live fetch fails. worker_num must be + // >= stall + live so a free worker exists for each live client. + worker_num := 4 + stall := 2 + live := 2 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 + accept_timeout: 10 * time.second + worker_num: worker_num handler: EchoHandler{} show_startup_message: false } @@ -588,21 +609,50 @@ fn test_server_tls_parallel_handshakes() { 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 { + // Wedge `stall` workers mid-handshake with silent TCP connections. + mut stalled := []&net.TcpConn{} + for _ in 0 .. stall { + c := net.dial_tcp('127.0.0.1:${port}') or { + assert false, 'stall dial failed: ${err}' + return + } + stalled << c + } + // Give the accept loop time to hand each stalled connection to a worker (and, in + // a regressed serial-accept design, to become wedged on the first one) before the + // live clients connect. + time.sleep(500 * time.millisecond) + // Fire the live clients concurrently; with parallel per-worker handshakes the + // free workers service them in well under a second. This also exercises the + // shared mbedtls config/RNG under genuinely concurrent completing handshakes. + // + // The discriminator is time-bounded on purpose: a serial-accept regression does + // eventually service the live clients — but only after the stalled handshakes hit + // their budget (handshake_timeout == accept_timeout == 10s here, x2 stalled), so + // without a bound the test would merely run slower, not fail. The bound is on the + // WAIT itself (a select deadline below), not the client: http.fetch cannot bound + // this for us because Request.ssl_do dials and completes the TLS handshake before + // req.read_timeout applies (and retries on socket errors), so a read_timeout would + // not cap a regressed handshake. The select makes the regression fail promptly at + // ~6s; a real parallel handshake completes in well under a second. + live_budget := 6 * time.second + results := chan string{cap: live} + sw := time.new_stopwatch() + for i in 0 .. live { spawn fn [results, port, i] () { resp := http.fetch( - url: 'https://127.0.0.1:${port}/p${i}' + url: 'https://127.0.0.1:${port}/live${i}' enable_http2: false validate: false + // One attempt, no retries: a regressed handshake should surface as a + // single timed-out request, not retry-amplify the teardown. + max_retries: 1 + // Force a fresh TLS connection per live client. Without this, if one + // client finishes and returns its keep-alive connection to the shared + // pool before the other checks one out, the second reuses it and the + // test would pass after only a single live handshake — not two + // concurrent completing handshakes against the shared mbedtls state. + disable_connection_reuse: true ) or { results <- 'error: ${err}' return @@ -614,15 +664,31 @@ fn test_server_tls_parallel_handshakes() { results <- resp.body }() } - // Results arrive in nondeterministic order, so match on the common prefix - // rather than a specific path index. + // Collect both live results within a single `live_budget`, bounding the wait so a + // serialized-handshake regression fails at ~6s instead of hanging on the client's + // own handshake timeout. mut ok := 0 - for _ in 0 .. n { - body := <-results - assert body.starts_with('tls hello /p'), 'unexpected response: ${body}' - ok++ + for ok < live { + remaining := live_budget - sw.elapsed() + if remaining <= 0 { + break + } + select { + body := <-results { + assert body.starts_with('tls hello /live'), 'live handshake failed while ${stall} workers were mid-handshake (handshakes appear serialized on the accept thread): ${body}' + ok++ + } + remaining { + break + } + } + } + assert ok == live, 'expected ${live} live handshakes to complete within ${live_budget} while ${stall} workers were mid-handshake; got ${ok} — handshakes appear serialized on the accept thread, not parallel' + // Release the stalled sockets; srv.close() in the defer also force-closes any the + // worker is still blocked on via the idle tracker. + for mut c in stalled { + c.close() or {} } - assert ok == n, 'expected ${n} successful concurrent handshakes, got ${ok}' } fn test_server_tls_h2_negotiation() { 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 302fb5a49..726ad67f5 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 @@ -21,12 +21,18 @@ 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' + // sets `opened = false` BEFORE freeing, so a second call hits the early + // `if !s.opened` return instead of a second pass through the mbedtls frees. + // The guard's real purpose is preventing the double-free that the + // worker-defer-vs-close_idle race would otherwise cause (a double mbedtls_*_free + // aborts the process — or, on a forgiving allocator, silently corrupts the heap + // without erroring). Asserting merely that *an* error was returned would not + // distinguish the idempotent no-op from a second free that happened to report + // something; assert the specific "connection was not open" sentinel so the test + // proves the early-return path was taken. + mut second_err := '' + cli.shutdown() or { second_err = err.msg() } + assert second_err.contains('connection was not open'), 'second shutdown must hit the idempotent !opened early-return (sentinel "connection was not open"), proving no second free; got: ${second_err}' eprintln('[+] Second shutdown was a clean no-op') } -- 2.39.5