open-telemetry / open-telemetry/opentelemetry-cpp

[BUG] The curl client mutates and frees easy handle resources before removing the handle from the multi handle

Open
#4,391 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-triage
Dominant language
C++
Stars
1.4k
Forks
632
Avg merge
1d 13h
Merged PRs (30d)
75

Description

Describe your environment main at f6e48180, libcurl 8.14.1. This is a code reading report, not a crash report. I have not seen any of it misbehave at runtime, and I say where that matters below.

Three places in the curl client touch an easy handle, or free something the handle still points at, before the handle has been taken out of the multi handle, and the CURLMcode that would say whether the removal worked is dropped on the floor.

Teardown resets the handle before it is removed

HttpOperation::Cleanup() clears CURLOPT_PRIVATE and resets the handle, and only then hands it over to be removed:

      if (curl_resource_.easy_handle != nullptr)
      {
        curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_PRIVATE, NULL);   // :556
        curl_easy_reset(curl_resource_.easy_handle);                           // :557
      }
      session->GetHttpClient().ScheduleRemoveSession(...);                     // :559

curl_multi_remove_handle does not run until the IO thread gets to doRemoveSessions, at http_client_curl.cc:819. On a cancel the transfer can still be active at :556, and curl_easy_setopt says:

Changing options with curl_easy_setopt() while a transfer is still in progress may cause undefined and undesired behavior.

The header list is freed before the removal too
      if (nullptr != removing_handle.second.headers_chunk)
      {
        curl_slist_free_all(removing_handle.second.headers_chunk);             // :816
      }

      curl_multi_remove_handle(multi_handle_, removing_handle.second.easy_handle);  // :819
      curl_easy_cleanup(removing_handle.second.easy_handle);                        // :820

CURLOPT_HTTPHEADER:

When this option is passed to curl_easy_setopt, libcurl does not copy the entire list so you must keep it around until you no longer use this handle for a transfer before you call curl_slist_free_all on the list.

At :816 the handle is still attached, so the list is freed while it is arguably still in use for the transfer. Swapping :816 and :819 costs nothing.

The removal result is not checked

:819 ignores its CURLMcode and :820 cleans the handle up regardless. If the removal ever fails, that frees a handle the multi stack still knows about. The retry path has the same shape at :862:

      auto easy_handle = operation->GetCurlEasyHandle();
      curl_multi_remove_handle(multi_handle_, easy_handle);                    // :863
      curl_multi_add_handle(multi_handle_, easy_handle);                       // :864

No null check, and neither return value is read. GetCurlEasyHandle() returns null once Cleanup() has moved curl_resource_ out, and passing null to those two is harmless, so the concrete risk here is small. It is the silence I would rather not keep: a session can leave pending_to_retry_sessions_ at :865 whether or not it was actually re-armed.

What is the expected behavior? The IO thread removes the handle from the multi handle, checks the result, and only then resets it, frees the header list, and cleans it up. A failed removal is visible rather than assumed away.

What is the actual behavior? Options are set and memory is freed while the handle may still be attached, and a failed removal looks exactly like a successful one.

Additional context No reproduction. Reordering :816 and :819 and checking the two return codes is small and self contained, and I am happy to send it if you want it, but it is your call whether the retry path belongs in the same change or somewhere else.

Found while working on #4375, which is on the same file but is a different problem.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in http_client_curl.cc at HttpOperation::Cleanup() and the IO-thread doRemoveSessions entry point around lines 816-820; also read the retry path around lines 862-865. Confirm the removal result is handled before reset, header-list freeing, and cleanup, and that retry state reflects whether re-arming succeeded.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.