[BUG] Reusing a curl Session destroys the HttpOperation that is still running its own completion

[thc1006]

Describe your environment main at f6e4818, Ubuntu 24.04, gcc 14, libcurl 8.14.1, CMake, -fsanitize=address.

Session::SendRequest() replaces the operation the session owns:

  curl_operation_.reset(                                       // http_client_curl.cc:203
      new HttpOperation(http_request_->method_, url, ...));

The response callback runs from inside the operation it belongs to. HttpOperation::Cleanup() takes the callback and calls it while this is still very much in use:

    callback.swap(async_data_->callback);                      // :563
    if (callback)
    {
      HttpOperationAccessor::SetThreadId(*async_data_, std::this_thread::get_id());
      callback(*this);                                         // :567  OnResponse runs here
      HttpOperationAccessor::SetThreadId(*async_data_, std::thread::id());  // :568
    }

So a handler that starts another request on the same session from OnResponse destroys, through unique_ptr::reset, the object whose Cleanup() is on the stack two frames up. Line :568 then reads *async_data_ out of freed memory, and :572 and :574 do it again.

Nothing in the public Session interface says a session is single use, and nothing stops the second call.

Steps to reproduce A handler that re-sends from OnResponse, using the existing fixture:

class ReentrantSendHandler : public CustomEventHandler
{
public:
  http_client::Session *session_ = nullptr;

  void OnResponse(http_client::Response &) noexcept override
  {
    if (session_ != nullptr && !resent_)
    {
      resent_ = true;
      auto again = session_->CreateRequest();
      again->SetUri("get/");
      session_->SendRequest(std::make_shared<GetEventHandler>());
    }
  }
  void OnEvent(http_client::SessionState, nostd::string_view) noexcept override {}

private:
  bool resent_ = false;
};

TEST_F(BasicCurlHttpTests, ReentrantSendFromOnResponse)
{
  auto session_manager = std::make_shared<http_client::curl::HttpCurlClientFactory>()->Create();
  auto session = session_manager->CreateSession("http://127.0.0.1:19000");
  auto request = session->CreateRequest();
  request->SetUri("get/");

  auto handler      = std::make_shared<ReentrantSendHandler>();
  handler->session_ = session.get();

  session->SendRequest(handler);
  ASSERT_TRUE(waitForRequests(30, 1));
  session->FinishSession();
  session_manager->FinishAllSessions();
}

Under -fsanitize=address:

ERROR: AddressSanitizer: heap-use-after-free on address 0x516000006290
READ of size 8 at 0x516000006290 thread T2
    #3 HttpOperation::Cleanup() ext/src/http/client/curl/http_operation_curl.cc:568
    #4 HttpOperation::PerformCurlMessage(CURLcode) ext/src/http/client/curl/http_operation_curl.cc:1629
    #5 HttpClient::MaybeSpawnBackgroundThread()::<lambda> ext/src/http/client/curl/http_client_curl.cc:527
SUMMARY: AddressSanitizer: heap-use-after-free in std::__uniq_ptr_impl<HttpOperation::AsyncData>::_M_ptr()

It happens on the IO thread, which is the thread that owns the multi handle.

What is the expected behavior? Either a session can be reused and the second send waits for or replaces the first safely, or it cannot and the second call is refused with a terminal event. Either way the first operation is not destroyed while it is running.

What is the actual behavior? The operation frees itself part way through its own completion path.

Additional context The cheap half of this is a one way flag on Session, so a second SendRequest reports CreateFailed instead of replacing the operation. That documents the interface as single use, which is what the implementation already assumes everywhere else.

There is a second, thinner version of the same problem without any handler involved: two threads calling SendRequest() on one session race the same reset. I have not reproduced that one and it is the same fix.

The longer question is whether HttpOperation should hold a snapshot of the request rather than references into headers, body, SSL options and compression, since a caller that edits the request after SendRequest() is racing the transfer today. That is a bigger change than the flag and I would rather ask than assume.

Found while looking at the cancellation paths for #4375 and #4390.