diff --git a/CHANGELOG.md b/CHANGELOG.md index 819a741b73..bb0cbbe1ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ Increment the: * [CODE HEALTH] Move remaining API test helpers into anonymous namespaces [#4301](https://github.com/open-telemetry/opentelemetry-cpp/pull/4301) +* [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) diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index fed8518e94..b0f8299047 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -367,7 +367,16 @@ 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); + // 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. + m_connections.erase(connIt); + } } void handleConnection(Connection &conn) @@ -522,7 +531,14 @@ class HttpServer : private SocketTools::Reactor::SocketCallback if (conn.state == Connection::Processing) { - processRequest(conn); + if (processRequest(conn) == RequestOutcome::CloseConnection) + { + // 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; + } std::ostringstream os; os << conn.request.protocol << ' ' << conn.response.code << ' ' << conn.response.message @@ -736,7 +752,18 @@ class HttpServer : private SocketTools::Reactor::SocketCallback return result; } - void processRequest(Connection &conn) + enum class RequestOutcome : std::uint8_t + { + SendResponse, + CloseConnection + }; + + /** + * Run the registered handlers for a request and fill in the response. + * @return CloseConnection if a handler asked for the connection to be terminated. Closing is + * left to the caller, which owns the connection's lifetime. + */ + RequestOutcome processRequest(Connection &conn) { conn.response.message.clear(); conn.response.headers.clear(); @@ -766,7 +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 RequestOutcome::CloseConnection; } } @@ -779,6 +806,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 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 0807a405dc..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; } @@ -204,6 +206,13 @@ 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. + close_requests_.fetch_add(1, std::memory_order_relaxed); + response_status = -1; + } cv_got_events.notify_one(); @@ -463,6 +472,28 @@ TEST_F(BasicCurlHttpTests, SendGetRequestSync) EXPECT_EQ(result.GetSessionState(), http_client::SessionState::Response); } +TEST_F(BasicCurlHttpTests, HandlerRequestedCloseKeepsServerUsable) +{ + 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); + 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); + EXPECT_EQ(result, true); + EXPECT_EQ(result.GetSessionState(), http_client::SessionState::Response); +} + TEST_F(BasicCurlHttpTests, SendGetRequestSyncTimeout) { received_requests_.clear();