[BUG] Fix use-after-free when an HTTP handler closes the connection

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)
 

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)

ext/test/http/curl_http_test.cc

@@ -140,6 +140,7 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe
   std::atomic<bool> is_setup_{false};
   std::atomic<bool> is_running_{false};
   std::vector<HTTP_SERVER_NS::HttpRequest> received_requests_;
+  std::atomic<unsigned> 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();