From eaecdc0f63d7c9bc601ac301f82ad482610b65d4 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:24:48 +0800 Subject: [PATCH 1/3] [BUG] Fix use-after-free when a handler closes the connection 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 #4288 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/server/http_server.h | 22 ++++++++++++++--- ext/test/http/curl_http_test.cc | 24 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index fed8518e94..67f0c6e43b 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -367,7 +367,11 @@ class HttpServer : private SocketTools::Reactor::SocketCallback m_reactor.removeSocket(conn.socket); auto connIt = m_connections.find(conn.socket); conn.socket.close(); - m_connections.erase(connIt); + if (connIt != m_connections.end()) + { + // Destroys the Connection that conn refers to: callers must not use conn afterwards. + m_connections.erase(connIt); + } } void handleConnection(Connection &conn) @@ -522,7 +526,11 @@ class HttpServer : private SocketTools::Reactor::SocketCallback if (conn.state == Connection::Processing) { - processRequest(conn); + if (!processRequest(conn)) + { + // A handler closed the connection: conn has been erased and must not be touched. + return; + } std::ostringstream os; os << conn.request.protocol << ' ' << conn.response.code << ' ' << conn.response.message @@ -736,7 +744,12 @@ class HttpServer : private SocketTools::Reactor::SocketCallback return result; } - void processRequest(Connection &conn) + /** + * Run the registered handlers for a request and fill in the response. + * @return false if a handler asked to close the connection, in which case the connection has + * already been closed and \p conn must not be used again by the caller. + */ + bool processRequest(Connection &conn) { conn.response.message.clear(); conn.response.headers.clear(); @@ -767,6 +780,7 @@ class HttpServer : private SocketTools::Reactor::SocketCallback { LOG_TRACE("HttpServer: [%s] closing by request", conn.request.client.c_str()); handleConnectionClosed(conn); + return false; } } @@ -779,6 +793,8 @@ class HttpServer : private SocketTools::Reactor::SocketCallback conn.response.headers["Connection"] = (conn.keepalive ? "keep-alive" : "close"); conn.response.headers["Date"] = formatTimestamp(time(nullptr)); conn.response.headers["Content-Length"] = std::to_string(conn.response.body.size()); + + return true; } static std::string formatTimestamp(time_t time) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 0807a405dc..d0a171fead 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -204,6 +204,12 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe response.headers["Content-Type"] = "text/plain"; response_status = 429; } + else if (request.uri == "/close/") + { + // -1 is the documented way for a handler to ask the server to terminate the + // connection immediately without sending a response. + response_status = -1; + } cv_got_events.notify_one(); @@ -463,6 +469,24 @@ TEST_F(BasicCurlHttpTests, SendGetRequestSync) EXPECT_EQ(result.GetSessionState(), http_client::SessionState::Response); } +TEST_F(BasicCurlHttpTests, HandlerRequestedCloseKeepsServerUsable) +{ + received_requests_.clear(); + curl::HttpClientSync http_client; + http_client::Headers m1 = {}; + + // A handler returning -1 closes the connection without a response. The server used to + // keep reading and writing the Connection it had just erased, which AddressSanitizer + // reports as a use-after-free on this request. + auto closed = http_client.GetNoSsl("http://127.0.0.1:19000/close/", m1); + EXPECT_EQ(closed, false); + + // The server has to still be serving afterwards. + auto result = http_client.GetNoSsl("http://127.0.0.1:19000/get/", m1); + EXPECT_EQ(result, true); + EXPECT_EQ(result.GetSessionState(), http_client::SessionState::Response); +} + TEST_F(BasicCurlHttpTests, SendGetRequestSyncTimeout) { received_requests_.clear(); From dd5ceb7e8b6803f6d045c5b0120e886031ef9eb8 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:26:30 +0800 Subject: [PATCH 2/3] Add CHANGELOG entry for the connection use-after-free fix Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8baba4503c..194e07f29c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ Increment the: namespaces [#4303](https://github.com/open-telemetry/opentelemetry-cpp/pull/4303) +* [BUG] Fix use-after-free when an HTTP handler closes the connection + [#4289](https://github.com/open-telemetry/opentelemetry-cpp/pull/4289) + * [CODE HEALTH] Move metrics storage test fixtures into anonymous namespace [#4286](https://github.com/open-telemetry/opentelemetry-cpp/pull/4286) From 8c1a6f5a7358d0b918f955a89db844430df12365 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 01:03:40 +0800 Subject: [PATCH 3/3] Register /close/, prove the regression, and let the caller close 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 #4288 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/server/http_server.h | 29 ++++++++++++++----- ext/test/http/curl_http_test.cc | 17 +++++++---- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index 67f0c6e43b..b0f8299047 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -367,6 +367,11 @@ class HttpServer : private SocketTools::Reactor::SocketCallback m_reactor.removeSocket(conn.socket); auto connIt = m_connections.find(conn.socket); conn.socket.close(); + // Every caller passes a connection that is in the map. The guard keeps a broken invariant + // from turning into erase(end()), and the assert keeps it from passing unnoticed in a + // debug build. This does not make the function idempotent: the reactor removal and the + // socket close above have already run. + assert(connIt != m_connections.end()); if (connIt != m_connections.end()) { // Destroys the Connection that conn refers to: callers must not use conn afterwards. @@ -526,9 +531,12 @@ class HttpServer : private SocketTools::Reactor::SocketCallback if (conn.state == Connection::Processing) { - if (!processRequest(conn)) + if (processRequest(conn) == RequestOutcome::CloseConnection) { - // A handler closed the connection: conn has been erased and must not be touched. + // Mark the close as intentional so it is not reported as unexpected, then stop: + // handleConnectionClosed() erases conn, so it must not be touched afterwards. + conn.state = Connection::Closing; + handleConnectionClosed(conn); return; } @@ -744,12 +752,18 @@ class HttpServer : private SocketTools::Reactor::SocketCallback return result; } + enum class RequestOutcome : std::uint8_t + { + SendResponse, + CloseConnection + }; + /** * Run the registered handlers for a request and fill in the response. - * @return false if a handler asked to close the connection, in which case the connection has - * already been closed and \p conn must not be used again by the caller. + * @return CloseConnection if a handler asked for the connection to be terminated. Closing is + * left to the caller, which owns the connection's lifetime. */ - bool processRequest(Connection &conn) + RequestOutcome processRequest(Connection &conn) { conn.response.message.clear(); conn.response.headers.clear(); @@ -779,8 +793,7 @@ class HttpServer : private SocketTools::Reactor::SocketCallback if (conn.response.code == -1) { LOG_TRACE("HttpServer: [%s] closing by request", conn.request.client.c_str()); - handleConnectionClosed(conn); - return false; + return RequestOutcome::CloseConnection; } } @@ -794,7 +807,7 @@ class HttpServer : private SocketTools::Reactor::SocketCallback conn.response.headers["Date"] = formatTimestamp(time(nullptr)); conn.response.headers["Content-Length"] = std::to_string(conn.response.body.size()); - return true; + return RequestOutcome::SendResponse; } static std::string formatTimestamp(time_t time) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index d0a171fead..e22bec4e1e 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -140,6 +140,7 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe std::atomic is_setup_{false}; std::atomic is_running_{false}; std::vector received_requests_; + std::atomic close_requests_{0}; std::mutex cv_mtx_requests; std::mutex mtx_requests; std::condition_variable cv_got_events; @@ -165,6 +166,7 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe server_.addHandler("/get/", *this); server_.addHandler("/post/", *this); server_.addHandler("/retry/", *this); + server_.addHandler("/close/", *this); server_.start(); is_running_ = true; } @@ -208,6 +210,7 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe { // -1 is the documented way for a handler to ask the server to terminate the // connection immediately without sending a response. + close_requests_.fetch_add(1, std::memory_order_relaxed); response_status = -1; } @@ -471,15 +474,19 @@ TEST_F(BasicCurlHttpTests, SendGetRequestSync) TEST_F(BasicCurlHttpTests, HandlerRequestedCloseKeepsServerUsable) { - received_requests_.clear(); curl::HttpClientSync http_client; http_client::Headers m1 = {}; - // A handler returning -1 closes the connection without a response. The server used to - // keep reading and writing the Connection it had just erased, which AddressSanitizer - // reports as a use-after-free on this request. + // A handler returning -1 closes the connection without a response. The server used to keep + // reading and writing the Connection it had just erased, which AddressSanitizer reports as + // a use-after-free on this request. auto closed = http_client.GetNoSsl("http://127.0.0.1:19000/close/", m1); - EXPECT_EQ(closed, false); + ASSERT_EQ(closed, false); + + // Prove the request actually reached the handler. Without this, an unregistered route or a + // typo would leave the server answering 404 and the assertion above would be measuring the + // wrong thing. + EXPECT_EQ(close_requests_.load(std::memory_order_relaxed), 1U); // The server has to still be serving afterwards. auto result = http_client.GetNoSsl("http://127.0.0.1:19000/get/", m1);