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

[thc1006]

Fixes #4288
Refs #4287

Correction to an earlier revision of this description

The first version of this PR said the test reproduced the use-after-free under AddressSanitizer. That was not true. The /close/ branch was added to the test handler but never registered with addHandler(), so the request fell through to the default 404, the client saw an ordinary response, and the assertion measured nothing. That is what broke the test jobs. I wrote a reproduction claim I had not run, and a reviewer caught it before it wasted anyone else's time.

Both are fixed below, and the reproduction is now something I actually executed.

The bug

m_connections is a std::map<SocketTools::Socket, Connection>, so the m_connections.erase(connIt) at the end of handleConnectionClosed() destroys the mapped Connection. processRequest() called it and did not return, so everything after that point wrote to a destroyed object: conn.response.message, then the Host, Connection, Date and Content-Length headers. Control then went back to handleConnection(), which read conn.request.protocol and conn.response.* to build the status line and called sendMore(conn) on an already closed socket.

-1 is a documented part of the handler contract rather than an internal value; file_http_server.h states that a handler returning -1 terminates the connection.

Fail before, pass after

A standalone harness drives HttpServer with a handler that returns -1, then connects a raw client and sends one request. On origin/main:

==11==ERROR: AddressSanitizer: heap-use-after-free
READ of size 4 at 0x5150000001d8 thread T1
    #0 HttpServer::processRequest(Connection&)   http_server.h:775
    #1 HttpServer::handleConnection(Connection&) http_server.h:525
    #2 HttpServer::onSocketReadable(Socket)      http_server.h:293
freed by thread T1 here:
    #8 HttpServer::handleConnectionClosed(Connection&) http_server.h:370
    #9 HttpServer::processRequest(Connection&)         http_server.h:769

Freed by the erase at :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 and handleConnection() performs the close:

if (processRequest(conn) == RequestOutcome::CloseConnection)
{
  conn.state = Connection::Closing;
  handleConnectionClosed(conn);
  return;
}

This came out of review, and it is better than the bool version I first pushed for three reasons: connection lifetime stays with the one function that owns it, so a future caller cannot reintroduce the same use-after-free by adding a line after the call; the enum says why rather than just that something failed; and setting the state to Closing first means a handler-requested close is no longer logged as connection closed unexpectedly, which was a real behavior bug on a fully supported code path.

handleConnectionClosed() keeps the end() 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 in onSocketClosed().

Test

/close/ is now registered in SetUp() and returns -1. HandlerRequestedCloseKeepsServerUsable asserts 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-format 18 clean on both files.
• clang-tidy 18 with the repo .clang-tidy on http_server.h: 3 warnings before and 3 after. The first version of the enum added a performance-enum-size warning, so it now has an explicit std::uint8_t base type. warning_limit is untouched.
• Syntax-only build of file_http_server.h, which pulls in the subclass and an HttpServer instantiation, 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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.25%. Comparing base (aa60678) to head (c537b41).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4289      +/-   ##
==========================================
+ Coverage   81.24%   81.25%   +0.02%     
==========================================
  Files         446      446              
  Lines       18872    18878       +6     
==========================================
+ Hits        15330    15338       +8     
+ Misses       3542     3540       -2     

Files with missing lines	Coverage Δ
...nclude/opentelemetry/ext/http/server/http_server.h	66.67% <100.00%> (+0.76%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[owent]

LGTM, thanks.