net.http: fix TLS server close_idle fd-reuse race and remove dead set_read_timeout - #27542
Conversation
…_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 vlang#27433. Co-Authored-By: WOZCODE <contact@withwoz.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73d6230650
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Codex review of PR vlang#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>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae40710fe8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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 vlang#27542. Co-Authored-By: WOZCODE <contact@withwoz.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31405a6150
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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 vlang#27542. Co-Authored-By: WOZCODE <contact@withwoz.com>
|
@codex review |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
3 participants
Summary
Follow-up to #27429 (TLS server shutdown fix), addressing items 2 and 4 from #27433.
server_tls_idle.v—close_idlefd-reuse race (item 4): the old code droppedt.mubefore callingnet.shutdown, leaving a window where a worker could race throughunmark_idle → conn.shutdown() → net.close(fd), the OS could reuse that fd for a new socket, and the subsequentnet.shutdownwould interrupt an unrelated connection. Fix: movet.mu.unlock()to after the shutdown loop. Accepted TLS connections are blocking sockets (accept_with_timeoutsrestores blocking mode before returning), sonet.shutdownreturns immediately — no stall from holding the lock.server_tls_notd_use_openssl.v— deadset_read_timeoutcall (item 2): the per-connectionconn.set_read_timeout(s.read_timeout)call after accept had no effect. Accepted connections are bound to the listener's ssl config (l.conf) viambedtls_ssl_setup; the per-connconfthatset_read_timeoutwrites to is never attached to the live ssl session. The serverread_timeoutis correctly applied when theSSLListeneris created withread_timeout: s.read_timeoutin its config. Remove the dead call.Items 1 and 3 from #27433 were already fixed upstream or remain open architectural work.
Test plan
./v build vlib/net/http/./v test vlib/net/http/passesclose_idleholdst.muacross the shutdown loopset_read_timeoutcall inlisten_and_serve_tls🧙 Built with WOZCODE