Skip to content

Commit d56f20f

Browse files
net.http: fix TLS server close_idle fd-reuse race and remove dead set_read_timeout (#27542)
* net.http: fix TLS server close_idle fd-reuse race and remove dead set_read_timeout close_idle (server_tls_idle.v): move t.mu.unlock() to after the net.shutdown loop instead of before it. The old code dropped the lock before calling net.shutdown, leaving a window where a worker could race through unmark_idle → conn.shutdown() → net.close(fd), the OS could reuse that fd for a freshly accepted socket, and the subsequent net.shutdown would then interrupt an unrelated connection. Accepted TLS connections are blocking sockets (accept_with_timeouts restores blocking mode before returning), so net.shutdown returns immediately — holding the lock across the loop does not introduce a liveness hazard. listen_and_serve_tls (server_tls_notd_use_openssl.v): remove the dead conn.set_read_timeout(s.read_timeout) call after accept. Accepted connections are bound to the listener's ssl config (l.conf) via mbedtls_ssl_setup; the per-conn conf that set_read_timeout writes to is never bound to conn.ssl and has no effect on the live session. The server read_timeout is already applied correctly when the SSLListener is created with read_timeout: s.read_timeout in its config. Fixes items 2 (dead call) and 4 (fd-reuse race) from #27433. Co-Authored-By: WOZCODE <contact@withwoz.com> * net.http: add Windows wakeup explanation to close_idle, use defer unlock The Codex review of PR #27542 flagged that the lock-widening made close_idle''s net.close(handle) and the worker''s deferred conn.shutdown() a guaranteed double-close on Windows. Removing net.close was the wrong fix: net.shutdown(SD_BOTH) alone does NOT unblock a concurrent C.select() on Windows (only closesocket() does), so workers blocked in wait_for_read would hang for up to 41 s (mbedtls_server_read_timeout_ms) before noticing shutdown. Keep net.close in the Windows path; the double-close is safe because: - listen_and_serve_tls calls ch.close() before close_idle() - the accept loop has already stopped at that point - active handlers have called unmark_idle before w.handler.handle() runs, so close_idle only touches truly idle handles - the worker''s second closesocket returns WSAENOTSOCK, swallowed by or {} Also switch to defer { t.mu.unlock() } for consistency with mark_idle and unmark_idle. Co-Authored-By: WOZCODE <contact@withwoz.com> * net.http: stop close_idle/worker double-closing the TLS fd on Windows close_idle force-closes idle handles on Windows with net.close (closesocket) to wake a worker blocked in select(), which net.shutdown(SD_BOTH) does not do there. But the worker's handle_conn defer then ran conn.shutdown() unconditionally, closing the same fd a second time. Between the two closes the SOCKET value can be reused by ANY socket the process opens -- handle reuse is process-wide on Windows, not limited to this server's accept loop -- so the worker's second close could land on an unrelated socket. The previous "double-close is safe" comment only reasoned about this server's accept loop and was wrong. Transfer ownership: close_idle records the handles it force-closed (under the lock, Windows only), and the worker consults was_force_closed and skips its own conn.shutdown so the fd is closed exactly once. The tracker lock stays held across the shutdown loop: on non-Windows the worker is still the sole closer, and the held lock keeps its unmark_idle (hence its net.close) ordered after our net.shutdown, so the fd cannot be closed and recycled before we shut it down. Found by Codex on #27542. Co-Authored-By: WOZCODE <contact@withwoz.com> * net.http: free TLS resources for force-closed idle connections The previous fix skipped conn.shutdown() entirely for an idle TLS connection that close_idle force-closed on Windows, to avoid closing the socket a second time. But SSLConn.shutdown() does two jobs: it frees the mbedtls TLS resources (SSL context, config, certs, RNG state, ALPN allocation) AND closes the socket. Skipping it leaked all of those for every force-closed idle connection at server shutdown. Split the two concerns instead of bypassing cleanup: when close_idle already owns the socket close (was_force_closed), relinquish socket ownership (conn.owns_socket = false) and still call conn.shutdown(). shutdown() then frees the TLS resources but skips net.close, so the fd is still closed exactly once (by close_idle) and nothing leaks. On non-Windows was_force_closed is always false and the worker remains the sole closer, unchanged. Found by Codex on #27542. Co-Authored-By: WOZCODE <contact@withwoz.com> --------- Co-authored-by: WOZCODE <contact@withwoz.com>
1 parent eef477c commit d56f20f

2 files changed

Lines changed: 59 additions & 4 deletions

File tree

vlib/net/http/server_tls_idle.v

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,12 @@ struct TlsIdleConnTracker {
1212
mut:
1313
handles []int
1414
closing bool
15+
// closed records handles that close_idle force-closed on Windows. The woken
16+
// worker consults was_force_closed and relinquishes socket ownership (so its
17+
// conn.shutdown frees the TLS resources without closing the fd again), keeping
18+
// the fd closed exactly once. Empty on non-Windows (close_idle only shuts the
19+
// fd down there, leaving the worker the sole closer).
20+
closed []int
1521
}
1622

1723
fn (mut t TlsIdleConnTracker) mark_idle(handle int) bool {
@@ -37,12 +43,53 @@ fn (mut t TlsIdleConnTracker) unmark_idle(handle int) {
3743
}
3844
}
3945

46+
// was_force_closed reports whether close_idle has already closed `handle`
47+
// (Windows force-close to wake a worker blocked in select). The woken worker
48+
// must then relinquish socket ownership before its own conn.shutdown, which
49+
// then frees the TLS resources without closing the fd again: a second close
50+
// would race process-wide SOCKET reuse — the value can be reused by ANY socket
51+
// the process opens, not just this server's accept loop — and could close an
52+
// unrelated socket. Always false on non-Windows, where close_idle only shuts
53+
// the fd down and the worker remains the sole closer.
54+
fn (mut t TlsIdleConnTracker) was_force_closed(handle int) bool {
55+
$if windows {
56+
t.mu.lock()
57+
defer {
58+
t.mu.unlock()
59+
}
60+
return t.closed.index(handle) >= 0
61+
} $else {
62+
return false
63+
}
64+
}
65+
4066
fn (mut t TlsIdleConnTracker) close_idle() {
4167
t.mu.lock()
68+
defer {
69+
t.mu.unlock()
70+
}
4271
t.closing = true
4372
handles := t.handles.clone()
4473
t.handles.clear()
45-
t.mu.unlock()
74+
$if windows {
75+
// On Windows, net.shutdown(SD_BOTH) does not unblock a worker blocked in
76+
// select() — only closesocket() (net.close, below) does — so we must
77+
// force-close idle handles to wake their workers. Record ownership of
78+
// those closes here, under the lock, so a worker that wakes (or exits for
79+
// another reason) sees it via was_force_closed and relinquishes socket
80+
// ownership instead of closing the fd again. Without that the fd would be
81+
// closed twice, and between
82+
// the two closes the SOCKET value can be reused by any socket the process
83+
// opens, so the second close could hit an unrelated socket.
84+
t.closed << handles
85+
}
86+
// Hold the lock across the loop. On non-Windows, was_force_closed is always
87+
// false, so the worker remains the sole closer (via conn.shutdown); holding
88+
// the lock blocks its unmark_idle until our net.shutdown below has run, so
89+
// the worker cannot close the fd — and let the OS recycle the value — before
90+
// we shut it down. (Releasing the lock here would reopen exactly that race.)
91+
// On Windows the woken worker skips its close regardless, so the held lock
92+
// only briefly delays it and is otherwise harmless.
4693
for handle in handles {
4794
net.shutdown(handle)
4895
$if windows {

vlib/net/http/server_tls_notd_use_openssl.v

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,6 @@ fn (mut s Server) listen_and_serve_tls() {
109109
}
110110
continue
111111
}
112-
if s.read_timeout > 0 {
113-
conn.set_read_timeout(s.read_timeout)
114-
}
115112
ch <- conn
116113
}
117114
ch.close()
@@ -161,6 +158,17 @@ fn (mut w TlsHandlerWorker) handle_conn(mut conn mbedtls.SSLConn) {
161158
if !is_h2 {
162159
w.idle_conns.unmark_idle(conn.handle)
163160
}
161+
// close_idle may have already closed conn.handle on Windows (a forced
162+
// closesocket to wake this worker from a blocked read). conn.shutdown()
163+
// both frees the mbedtls TLS resources (SSL ctx, config, certs, RNG,
164+
// ALPN alloc) AND closes the socket; we must always do the former but
165+
// must not close the socket a second time — that would race process-wide
166+
// SOCKET reuse and could close an unrelated socket. So relinquish socket
167+
// ownership and still call shutdown for the TLS cleanup. On non-Windows
168+
// was_force_closed is always false and the worker remains the sole closer.
169+
if w.idle_conns.was_force_closed(conn.handle) {
170+
conn.owns_socket = false
171+
}
164172
conn.shutdown() or {}
165173
}
166174
// If the TLS handshake negotiated HTTP/2 via ALPN, switch to the HTTP/2

0 commit comments

Comments
 (0)