Improve SSL routing and transport - #242
Merged
Merged
Conversation
❌ Code Formatting Check FailedSome files in this PR are not properly formatted according to the project's clang-format rules. To fix this issue: make formatThen commit and push the changes. |
Collaborator
|
What's the actual TLS issue? Please log a ticket to with all details, context, problem, proposal and solution etc. We need see a big picture. ~thanks. |
Collaborator
Author
will do it later |
Collaborator
|
@bettercallsaulj thanks for the work, but once again, please log a ticket to explain the origin of this work. Or the PR can't be merged, unless the community is on the same page. And please change the title to reflect the work properly. |
bettercallsaulj
force-pushed
the
fix_tls_issue
branch
from
July 6, 2026 17:08
88493b0 to
886f99e
Compare
Collaborator
Author
Done |
bettercallsaulj
force-pushed
the
fix_tls_issue
branch
2 times, most recently
from
July 7, 2026 04:04
9ecbf17 to
d08091d
Compare
dIvYaNshhh
reviewed
Jul 7, 2026
scheduleTransition() and four sibling sites in the SSL/transport layer posted lambdas capturing a raw `this` to the dispatcher. If the object was destroyed before the dispatcher drained the post queue, the lambda dereferenced freed memory. Symptom: SIGSEGV / EXC_BAD_ACCESS at ~0x20 during SSL handshake teardown, observed intermittently against real HTTPS MCP servers. The race exists on every run; the crash fires on the fraction where teardown wins. Add a std::shared_ptr<bool> alive_ liveness token to SslStateMachine, SslTransportSocket, and TransportSocketStateMachine. Each posted lambda now captures a std::weak_ptr<bool> and bails on expired(). Changes: - include/mcp/transport/ssl_state_machine.h: alive_ token + weak_ptr capture in scheduleTransition() - include/mcp/transport/ssl_transport_socket.h: alive_ token - src/transport/ssl_transport_socket.cc: weak_ptr capture at handshake kickoff, scheduleShutdownCheck(), and flushBufferedWrites() - include/mcp/transport/transport_socket_state_machine.h: alive_ token - src/transport/transport_socket_state_machine.cc: weak_ptr capture in scheduleTransition() post - tests/transport/test_ssl_state_machine.cc: 4 regression tests covering destroyed-before-drain, multiple posts after destroy, normal path still works, and mixed alive/dead machines through one dispatcher - tests/transport/test_transport_socket_state_machine.cc: 3 regression tests for TransportSocketStateMachineLifetime using a standalone LibeventDispatcher so drain timing is controllable
ERR_func_error_string() was deprecated in OpenSSL 3.0 and now always returns NULL — OpenSSL no longer tracks function names in the error queue. drainOpenSSLErrorQueue() streamed that NULL value directly into a std::stringstream, which is undefined behavior; libc++ calls strlen on the NULL pointer and segfaults. ERR_lib_error_string() can also return NULL for synthetic / app errors that aren't in the library's table, so both accessors are now null-guarded and substitute "unknown" when NULL is returned. The deprecated call is wrapped in a #pragma diagnostic suppression so the build stays clean. The helper was moved out of an anonymous namespace into mcp::transport::detail so unit tests can reach it directly. Callers inside ssl_transport_socket.cc are updated to use the qualified name. Changes: - include/mcp/transport/ssl_transport_socket.h: declare detail::drainOpenSSLErrorQueue() - src/transport/ssl_transport_socket.cc: move drainOpenSSLErrorQueue into detail::; null-guard both ERR_*_error_string() return values; qualify the three internal callers - tests/transport/test_ssl_error_queue.cc: 5 regression tests covering empty queue, single formatted error, multiple concatenated errors, func= field with NULL accessor, and synthetic unknown-library entries - tests/CMakeLists.txt: wire test_ssl_error_queue executable, link recipe, and add_test() registration
moveToBio() read encrypted bytes from the socket and, when the fixed-size network BIO filled before all of them were written, discarded the unwritten remainder. Those bytes were already consumed from the kernel socket buffer, so dropping them corrupted the TLS byte stream: large responses (e.g. an ~82KB tools/list) were truncated and the connection was torn down (RemoteClose). With the in-flight request never completing, callers that block on the result (ReActAgent tool loading) hang indefinitely. Carry the unwritten remainder in bio_carryover_ and re-feed it (in order) before any further socket read. On SSL_ERROR_WANT_READ, refill the BIO and keep decrypting so a response larger than the BIO capacity drains fully within one read instead of stalling.
The server used a single shared `current_connection_` pointer to decide which socket each response is written to, but it was only set when a connection was accepted (onNewConnection) -- never per request. Under concurrent requests all connections are accepted in a burst, so the pointer ends up pinned to the last-accepted connection and every response is written to that one socket; the other clients receive nothing and hang until their timeout. Fix: bind the originating connection per request. Add a `McpProtocolCallbacks::setCurrentConnection()` hook (default no-op) that the HTTP/SSE filter calls with `write_callbacks_->connection()` immediately before dispatching each request/notification; McpServer's callbacks set `current_connection_` from it. Safe because request handling runs synchronously on the single dispatcher thread. (The analogous bug on the connection-close path was already fixed via a per-connection lifecycle adapter; the request path was missed.) Verified on the live gateway: 60 concurrent in-pod requests went from 52/60 hanging to 60/60 OK; 100 requests through the public ingress went from ~25% hangs to 0.
Surface SSL transport failures at warning level, reduce noisy filter registry startup logs, and add debug-level MCP request/response tracing behind dedicated logging controls.
Summary: Keep encrypted bytes consumed from OpenSSL's network BIO when the underlying socket accepts only a partial write, and flush that carryover before reading more BIO output. Add a regression test for partial socket writes.
Summary: Add per-turn encrypted input and plaintext output budgets for SSL reads so one connection cannot drain an unbounded backlog in a single dispatcher turn. Re-arm readability when a budget is exhausted and add a regression test for the moveToBio iteration budget.
Summary: Move LoggerRegistry::shouldLog bloom-filter reads under the registry mutex so they cannot race with logger registration or bloom-filter rebuilds. Add a concurrent shouldLog/update regression test for the logger registry.
Summary: Add a client lifetime token for initializeProtocol's detached worker and dispatcher continuations so shutdown prevents stale callbacks from touching client state. Parse initialize responses through a checked helper so missing result payloads report a controlled error, and add regression coverage for shutdown token expiry and missing initialize results.
Summary: Cast header-name bytes through unsigned char before std::tolower in HTTP async client log redaction to avoid signed-char undefined behavior. Add a real-I/O regression that enables flow logging and sends a high-bit header name through the redaction path.
Summary: Replace immediate SSL shutdown self-reposts with a dispatcher timer and bounded retry count so peers that delay or omit close_notify cannot busy-spin the event loop. Cancel the shutdown retry timer during teardown and add a unit regression that verifies shutdown checks schedule a timer instead of posting recursively.
Summary: Clear OpenSSL's thread-local error queue before handshake, read, write, and shutdown operations so stale errors cannot affect later SSL_get_error interpretation. Expose the clear helper beside the existing diagnostic drain helper and add regression coverage that stale queued errors are removed before SSL operations.
Summary: Apply formatting updates to the SSL transport, HTTP async client, logger registry, and SSL transport tests after the review fixes.
bettercallsaulj
force-pushed
the
fix_tls_issue
branch
from
July 7, 2026 14:13
d08091d to
0824cf8
Compare
dIvYaNshhh
approved these changes
Jul 7, 2026
dIvYaNshhh
left a comment
dIvYaNshhh
left a comment
Collaborator
There was a problem hiding this comment.
LGTM ✅ — all previously raised findings are addressed. Thanks for the thorough fixes.
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
3 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.
Issues:
Fix SSL transport use-after-free in posted lambdas (#245)
Null-guard OpenSSL 3.x error-string accessors to prevent crash (#246)
Fix SSL transport data loss on full network BIO (#247)
Route MCP responses to the originating connection (#248)
Improve transport and MCP flow logging (#249)