[BUG] Remove a curl easy handle before releasing what it points at

Teardown touched the easy handle in the wrong order and on the wrong thread.
Cleanup cleared CURLOPT_PRIVATE and called curl_easy_reset on it, from
whichever thread ran the cancel, while the transfer could still be active.
doRemoveSessions then freed the header list before curl_multi_remove_handle
and cleaned the handle up whatever that call returned.

libcurl is explicit on all three: changing options while a transfer is in
progress may have undefined behaviour, the header list has to outlive the
handle that points at it, and a handle has to leave the multi handle before it
can be cleaned up.

Cleanup now hands the resource over untouched. doRemoveSessions removes it,
checks the CURLMcode, and only then frees the list and the handle. A failed
removal leaves both alone: leaking one easy handle is better than freeing one
the multi stack may still own. A handle that was never added reports CURLM_OK,
so no separate bookkeeping is needed to tell the two apart.

Clearing CURLOPT_PRIVATE was load bearing rather than tidy up. The IO loop
reads it back to decide whether a CURLMSG_DONE belongs to a session that has
already gone, so PerformCurlMessage checks is_cleaned_ instead. That keeps the
behaviour without writing to a handle from a thread that does not own it.

Fixes #4391.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Author: thc1006

CHANGELOG.md

@@ -44,6 +44,9 @@ Increment the:
   deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`,
   `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents
   ([#4349](https://github.com/open-telemetry/opentelemetry-cpp/pull/4349))
+* [BUG] Remove a curl easy handle from the multi handle before freeing that
+  handle and the header list it points at
+  [#4391](https://github.com/open-telemetry/opentelemetry-cpp/issues/4391)
 
 * [CONFIGURATION] Add SDK component builder interfaces to the registry
   [#4358](https://github.com/open-telemetry/opentelemetry-cpp/issues/4358)

ext/src/http/client/curl/http_client_curl.cc

@@ -11,6 +11,7 @@
 #include <functional>
 #include <list>
 #include <mutex>
+#include <ostream>
 #include <string>
 #include <thread>
 #include <unordered_map>
@@ -23,6 +24,7 @@
 #include "opentelemetry/ext/http/common/url_parser.h"
 #include "opentelemetry/nostd/shared_ptr.h"
 #include "opentelemetry/nostd/string_view.h"
+#include "opentelemetry/sdk/common/global_log_handler.h"
 #include "opentelemetry/sdk/common/thread_instrumentation.h"
 #include "opentelemetry/version.h"
 
@@ -33,8 +35,6 @@
 #  include <array>
 
 #  include "opentelemetry/nostd/type_traits.h"
-#else
-#  include "opentelemetry/sdk/common/global_log_handler.h"
 #endif
 
 OPENTELEMETRY_BEGIN_NAMESPACE
@@ -811,13 +811,36 @@ bool HttpClient::doRemoveSessions()
 
     for (auto &removing_handle : pending_to_remove_session_handles)
     {
-      if (nullptr != removing_handle.second.headers_chunk)
+      auto &resource = removing_handle.second;
+      if (nullptr == resource.easy_handle)
+      {
+        continue;
+      }
+
+      // Take it out of the multi handle first. libcurl does not allow an easy handle to be
+      // cleaned up while a multi handle still owns it, and the header list has to stay alive
+      // until the handle is no longer used for a transfer. A handle that was never added
+      // reports CURLM_OK here, so this does not need to know which is which.
+      const CURLMcode rc = curl_multi_remove_handle(multi_handle_, resource.easy_handle);
+      if (CURLM_OK != rc)
+      {
+        // The multi handle may still own it. Leaking one easy handle is the better half of
+        // this trade: freeing it here would corrupt whatever still holds it.
+        OTEL_INTERNAL_LOG_ERROR(
+            "[HTTP Client Curl] curl_multi_remove_handle failed, leaving "
+            "the handle alone: "
+            << curl_multi_strerror(rc));
+        continue;
+      }
+
+      if (nullptr != resource.headers_chunk)
       {
-        curl_slist_free_all(removing_handle.second.headers_chunk);
+        curl_slist_free_all(resource.headers_chunk);
+        resource.headers_chunk = nullptr;
       }
 
-      curl_multi_remove_handle(multi_handle_, removing_handle.second.easy_handle);
-      curl_easy_cleanup(removing_handle.second.easy_handle);
+      curl_easy_cleanup(resource.easy_handle);
+      resource.easy_handle = nullptr;
     }
 
     for (auto &removing_session : pending_to_remove_sessions)

ext/src/http/client/curl/http_operation_curl.cc

@@ -542,15 +542,13 @@ void HttpOperation::Cleanup()
   // Only cleanup async once even in recursive calls
   if (async_data_)
   {
-    // Just reset and move easy_handle to owner if in async mode
+    // Move the easy handle to the owner untouched. It may still be attached to the multi handle
+    // and running, and the thread inside curl_multi_perform owns it until the IO thread takes it
+    // out, so writing to it here would be a write to a live transfer. It is pointless as well:
+    // the next thing that happens to the handle is curl_easy_cleanup.
     Session *session = async_data_->session.exchange(nullptr, std::memory_order_acq_rel);
     if (session != nullptr)
     {
-      if (curl_resource_.easy_handle != nullptr)
-      {
-        curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_PRIVATE, NULL);
-        curl_easy_reset(curl_resource_.easy_handle);
-      }
       session->GetHttpClient().ScheduleRemoveSession(session->GetSessionId(),
                                                      std::move(curl_resource_));
     }
@@ -1525,6 +1523,14 @@ void HttpOperation::Abort()
 
 void HttpOperation::PerformCurlMessage(CURLcode code)
 {
+  if (is_cleaned_.load(std::memory_order_acquire))
+  {
+    // Already torn down, and the handle is queued for removal. The multi handle can still hold a
+    // message buffered from before that, and acting on it would dispatch a second round of
+    // terminal events and can put a handle that is waiting to be freed back on the retry queue.
+    return;
+  }
+
   ++retry_attempts_;
   last_attempt_time_      = std::chrono::system_clock::now();
   last_curl_result_       = code;