Resolve SSL transport data loss on full network BIO - #244
Conversation
|
@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. |
dIvYaNshhh
left a comment
dIvYaNshhh
left a comment
There was a problem hiding this comment.
Automated review — 3 blockers (2 UAFs + an empty-body regression) plus mediums/low. Inline comments below.
caa3a75 to
3901ef2
Compare
44c9613 to
3d4b49e
Compare
Done |
|
@bettercallsaulj why do you so many blocker and medium level issue to address? Don't you need good design before coding? Where is the design? |
This whole issue is about crashing on Linux. |
3901ef2 to
2f40053
Compare
3d4b49e to
4a053dc
Compare
gophergogo
changed the title
There was a problem hiding this comment.
the empty-body, handleParserError use-after-free, and close() callback issues from the earlier round are all addressed here, and the total body-size cap is a nice add. Two smaller notes below. One bigger point for the description: this PR fixes the data loss on the read side only; the write side (moveFromBio(), from #242) still drops encrypted bytes on a full socket, so the data-loss goal isn't fully met yet — see the comment on #242.
2f40053 to
d088a49
Compare
4a053dc to
92cdd3b
Compare
dIvYaNshhh
left a comment
dIvYaNshhh
left a comment
There was a problem hiding this comment.
LGTM ✅ — all previously raised findings are addressed. Thanks for the thorough fixes.
d088a49 to
0f850c1
Compare
Summary: - Add default HTTP header storage to MCP client and connection configuration so SDK-created clients can send runtime Authorization headers. - Add per-request HTTP header overloads for sendRequest, listTools, and callTool so gateway callers can pass the client bearer token through to backend MCP servers. - Propagate configured and per-request headers into HTTP/SSE and streamable HTTP request formatting, with per-request headers overriding static defaults. - Preserve generated transport headers such as Host, Content-Type, Content-Length, Accept, Connection, User-Agent, and Cache-Control.
Summary: - Avoid reading peer certificate metadata for client transports that intentionally disable peer verification. - Prevent the Linux HTTP/SSE native client from crashing in SSL_get_peer_certificate during MCP tool discovery.
Summary: - Stream client HTTP response body chunks downstream without accumulating them in the codec stream state. - Avoid duplicate forwarding and unbounded retention for long-lived HTTP/SSE responses during MCP initialization.
Summary: - Reject null or oversized HTTP body callback chunks before allocating response strings. - Convert body allocation failures into parser errors so the event thread does not terminate.
Summary: - Store parser callback failures and report them after llhttp execution unwinds. - Avoid synchronous MCP disconnect and transport destruction from inside body callbacks.
Summary: - Report parser errors through the dispatcher instead of directly from the read callback stack. - Avoid MCP disconnect and HTTP/SSE transport destruction while filters are still unwinding.
Summary: - Keep active MCP connections alive after close() schedules dispatcher-owned shutdown. - Let the existing close event path move the connection into deferred deletion instead of resetting it immediately.
Summary: - Link the Linux gopher-mcp shared library with -Bsymbolic-functions. - Prevent Node.js llhttp symbol interposition from corrupting HTTP parser callback arguments.
Respect the configured preferred transport for HTTP and HTTPS MCP URLs when auto negotiation is enabled. This lets callers explicitly choose Streamable HTTP or HTTP+SSE instead of always falling back to URI path heuristics.
Summary: Fetch llhttp sources without configuring upstream shared targets, build the SDK parser dependency as a private static target with hidden visibility, and narrow the Linux version script to the llhttp entry points used by the SDK.
Summary: Clear the MCP connection manager callback pointer synchronously during client shutdown before posting close work or requesting dispatcher exit, and destroy dispatcher-owned resources before deleting the dispatcher. Add a regression test that verifies callback clearing does not depend on queued dispatcher work.
Summary: Apply formatting cleanup to the MCP client memory and connection manager tests after the header and shutdown fixes.
92cdd3b to
541b7e0
Compare
3 participants
This PR must be merged after #243
Issue:
Skip TLS peer metadata when verification is disabled (#251)
Avoid retaining streamed HTTP client bodies (#260)
Guard oversized HTTP body callbacks (#255)
Defer HTTP parser callback errors (#256)
Post HTTP parser error callbacks (#252)
Defer active connection destruction on close (#257)
Bind llhttp symbols inside shared library (#259)
Honor preferred HTTP transport (#253)