Conversation
14 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4289 +/- ##
==========================================
+ Coverage 81.15% 81.16% +0.01%
==========================================
Files 446 446
Lines 18922 18929 +7
==========================================
+ Hits 15355 15361 +6
- Misses 3567 3568 +1
🚀 New features to boost your workflow:
|
thc1006
force-pushed
the
bugfix/http-server-connection-uaf-4288
branch
from
July 24, 2026 17:52
4dfb26f to
5d4cc53
Compare
thc1006
marked this pull request as ready for review
July 25, 2026 07:49
processRequest() called handleConnectionClosed() and then kept going. That erases the Connection from m_connections, so every later access in processRequest() and in its caller handleConnection() touched a destroyed object, and sendMore() then ran on a closed socket. processRequest() now reports whether the connection survived, and handleConnection() returns immediately when it did not. The erase in handleConnectionClosed() is also guarded against a missed find(). Adds a regression test: a handler on /close/ returns -1 and the test checks that the server still serves afterwards. The unfixed code trips AddressSanitizer on that request. Fixes open-telemetry#4288 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The test route was never reachable: the /close/ branch was added to
onHttpRequest() but SetUp() never called addHandler("/close/", *this),
so the request fell through to the default 404, the client saw a normal
response, and the assertion measured nothing. That is what broke the
test jobs. The route is now registered and the test also asserts a
handler invocation counter, so an unregistered or misspelled route fails
loudly instead of silently passing.
processRequest() no longer destroys the connection it was handed. It
returns a RequestOutcome and handleConnection() performs the close, so
lifetime stays with the one function that owns it and a future caller
cannot reintroduce the same use-after-free. The state is set to Closing
first, so a handler-requested close is no longer logged as "connection
closed unexpectedly".
handleConnectionClosed() keeps the end() guard and now also asserts, so
a broken invariant is caught in a debug build instead of being swallowed.
Verified with a standalone harness driving HttpServer with a handler
returning -1. On origin/main, AddressSanitizer reports
heap-use-after-free with the read at http_server.h:775 and the free at
:370. With this change the same harness is clean under both ASan and
UBSan.
Fixes open-telemetry#4288
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
force-pushed
the
bugfix/http-server-connection-uaf-4288
branch
from
July 25, 2026 08:54
5d4cc53 to
8c1a6f5
Compare
…nnection-uaf-4288 # Conflicts: # CHANGELOG.md
6 tasks
marcalff
approved these changes
Jul 28, 2026
marcalff
left a comment
Member
There was a problem hiding this comment.
LGTM, thanks for the analysis and fix.
Member
|
Please check CI, there is a test failure in bazel (MacOS) |
HandlerRequestedCloseKeepsServerUsable aborted on macOS with 'Assertion failed: (it != m_sockets.end())' in Reactor::onThread. kevent() and epoll_wait() both report a batch of events. Handling one of them can close a connection, which calls removeSocket() and drops that socket from m_sockets straight away, so a later entry in the same batch can still name it. The lookup then reaches m_sockets.end(), and reading it->socket dereferences that iterator. A debug build stops at the assert; without NDEBUG there is nothing to stop it. Handler-requested close makes this reachable from a read callback, which is what the new test does. Both loops now skip an event whose socket is no longer registered. The Windows path handles one event per wait rather than a batch, so it does not have the same window.
Member
Author
|
Thanks for catching that. Root cause and fix are pushed.
A handler asking to close now happens inside a read callback, which is what makes the new test reach it. Both loops skip an event whose socket is no longer registered. The Windows path takes one event per wait rather than a batch, so I left it alone. macOS Bazel and macOS Conan were the same test and the same assertion, so this covers both. |
Member
|
This patch changes test code only, no code change in SDK or exporters. |
1 task
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
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.
Fixes #4288
Refs #4287
The bug
m_connectionsis astd::map<SocketTools::Socket, Connection>, so them_connections.erase(connIt)at the end ofhandleConnectionClosed()destroys the mappedConnection.processRequest()called it and did not return, so everything after that point wrote to a destroyed object:conn.response.message, then theHost,Connection,DateandContent-Lengthheaders. Control then went back tohandleConnection(), which readconn.request.protocolandconn.response.*to build the status line and calledsendMore(conn)on an already closed socket.-1is a documented part of the handler contract rather than an internal value;file_http_server.hstates that a handler returning-1terminates the connection.Fail before, pass after
A standalone harness drives
HttpServerwith a handler that returns-1, then connects a raw client and sends one request. Onorigin/main:Freed by the
eraseat:370, read at:775. With this change the same harness runs clean under both ASan and UBSan, the handler is invoked once, and the client observes the connection closed with no response.The fix
processRequest()no longer destroys the connection it was handed. It reports an outcome andhandleConnection()performs the close:handleConnectionClosed()keeps theend()guard and now also asserts, so a broken invariant is caught in a debug build rather than silently swallowed. The guard does not make the function idempotent, since the reactor removal and socket close above it have already run, and there is a comment saying so.The other two callers were already correct:
onSocketReadable()returns immediately afterwards, and it is the last statement inonSocketClosed().Test
/close/is now registered inSetUp()and returns-1.HandlerRequestedCloseKeepsServerUsableasserts that the request fails, that a handler invocation counter is exactly 1, and that the next request is still served. The counter is the part that matters: without it, an unregistered or misspelled route passes the first assertion for the wrong reason, which is exactly what happened.bazel test --config=asan //...covers//ext/test/http:curl_http_test.Verification
.clang-tidyonhttp_server.h: 3 warnings before and 3 after. The first version of the enum added aperformance-enum-sizewarning, so it now has an explicitstd::uint8_tbase type.warning_limitis untouched.file_http_server.h, which pulls in the subclass and anHttpServerinstantiation, compiles clean.Scope
Only the handler-requested close. The rest of #4287, including fatal
send()errors and the Windows partial-write stall, is left for its own PRs.