curl_easy_cleanup without curl_multi_remove_handle can cause incorrect results in future operations

[BrianHarris]

I did this

The documentation for curl_easy_cleanup does say:

To close an easy handle that has been used with the multi interface, make sure to first call curl_multi_remove_handle to remove it from the multi handle before it is closed.

However the code for curl_easy_cleanup automatically calls curl_multi_remove_handle, so this is normally not strictly required...

Except that curl_multi_remove_handle is called after Curl_detach_connection(data); which could leave the connection in a very bad state, leading to future operations on that connection receiving the data for the operation that was just cleaned up.

I believe this could be improved by moving the Curl_detach_connection(data); down below curl_multi_remove_handle . That note from the documentation could also be removed if this change is made.

I expected the following

No response

curl/libcurl version

libcurl 8.14.0

operating system

All

[bagder]

@BrianHarris any chance you can provide a smallish example that reproduces this problem case to help us add that as a test case?

[bagder]

This said, I like the consistency of having one remove call for every add and one cleanup for every init, so I think we should still document that as the way to do it. Even if we the code as a convenience actually handles a cleanup directly without a previous remove.

[BrianHarris]

Yes, here's a sample program that will trigger the problem:

#include <iostream>
#include <curl/curl.h>
#include <chrono>
#include <thread>
#include <string_view>

static int debug_callback(CURL* handle, curl_infotype type, char* data, size_t size, void* userptr) {
    if (type == CURLINFO_TEXT)
    {
        std::string_view message(data, size);
        std::cout << "curl: " << message;
    }
    return 0;
}

void setopts(CURL* easy_handle, char const* url)
{
    curl_easy_setopt(easy_handle, CURLOPT_URL, url);
    curl_easy_setopt(easy_handle, CURLOPT_NOBODY, 1L);

    curl_easy_setopt(easy_handle, CURLOPT_VERBOSE, 1L);
    curl_easy_setopt(easy_handle, CURLOPT_DEBUGFUNCTION, debug_callback);
    curl_easy_setopt(easy_handle, CURLOPT_DEBUGDATA, 0);

    curl_easy_setopt(easy_handle, CURLOPT_NOSIGNAL, 1L);
    curl_easy_setopt(easy_handle, CURLOPT_TIMEOUT, 0L);
    curl_easy_setopt(easy_handle, CURLOPT_LOW_SPEED_TIME, 60L);
    curl_easy_setopt(easy_handle, CURLOPT_LOW_SPEED_LIMIT, 1L);

    curl_easy_setopt(easy_handle, CURLOPT_SSL_VERIFYPEER, 1L);
    curl_easy_setopt(easy_handle, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2);

    curl_easy_setopt(easy_handle, CURLOPT_ACCEPT_ENCODING, "");
    curl_easy_setopt(easy_handle, CURLOPT_CONNECTTIMEOUT, 60L);
    curl_easy_setopt(easy_handle, CURLOPT_FOLLOWLOCATION, 1L);
    curl_easy_setopt(easy_handle, CURLOPT_MAXREDIRS, 128L);
    curl_easy_setopt(easy_handle, CURLOPT_NOPROGRESS, 1L);
    curl_easy_setopt(easy_handle, CURLOPT_TCP_KEEPALIVE, 1L);
    curl_easy_setopt(easy_handle, CURLOPT_TCP_NODELAY, 1L);
    curl_easy_setopt(easy_handle, CURLOPT_HEADER, 0L);
    curl_easy_setopt(easy_handle, CURLOPT_BUFFERSIZE, CURL_MAX_READ_SIZE);
}

int main() {
    curl_global_init(CURL_GLOBAL_ALL);
    
    CURLM* multi_handle = curl_multi_init();
    if (!multi_handle) {
        std::cerr << "Failed to create multi handle" << std::endl;
        return 1;
    }

    std::cout << std::endl << "Starting first request" << std::endl;
    {
        CURL* easy_handle = curl_easy_init();
        if (!easy_handle) {
            std::cerr << "Failed to create easy handle" << std::endl;
            curl_multi_cleanup(multi_handle);
            return 1;
        }

        setopts(easy_handle, "https://curl.se/404");
        curl_easy_setopt(easy_handle, CURLOPT_DEBUGDATA, 1);

        curl_multi_add_handle(multi_handle, easy_handle);

        // You will probably need to adjust this number to get the bug to happen
        // Too large and there's no problem
        // Too small and you get "Connection #0 is not open enough, cannot reuse"
        // I've sometimes seen "Connection 0 seems to be dead\n shutting down connection #0"
        for (int i = 0; i < 8; i++) {
            int running_handles;
            CURLMcode mc = curl_multi_perform(multi_handle, &running_handles);
            if (mc != CURLM_OK) {
                std::cerr << "curl_multi_perform failed: " << curl_multi_strerror(mc) << std::endl;
            }
            std::this_thread::sleep_for(std::chrono::milliseconds(10));
        }

        // Uncomment this line to fix the bug
        // curl_multi_remove_handle(multi_handle, easy_handle);
        curl_easy_cleanup(easy_handle);
    }

    std::cout  << std::endl<< "Starting second request" << std::endl;
    {
        CURL* easy_handle = curl_easy_init();
        if (!easy_handle) {
            std::cerr << "Failed to create easy handle" << std::endl;
            curl_multi_cleanup(multi_handle);
            return 1;
        }

        setopts(easy_handle, "https://curl.se/");

        curl_multi_add_handle(multi_handle, easy_handle);

        int running_handles;
        do {
            CURLMcode mc = curl_multi_perform(multi_handle, &running_handles);
            if (mc != CURLM_OK) {
                std::cerr << "curl_multi_perform failed: " << curl_multi_strerror(mc) << std::endl;
                break;
            }

            int numfds;
            mc = curl_multi_wait(multi_handle, nullptr, 0, 1000, &numfds);
            if (mc != CURLM_OK) {
                std::cerr << "curl_multi_wait failed: " << curl_multi_strerror(mc) << std::endl;
                break;
            }
        } while (running_handles);

        long response_code;
        curl_easy_getinfo(easy_handle, CURLINFO_RESPONSE_CODE, &response_code);
        std::cout << "Response code: " << response_code << std::endl;

        curl_multi_remove_handle(multi_handle, easy_handle);
        curl_easy_cleanup(easy_handle);
    }

    curl_multi_cleanup(multi_handle);
    curl_global_cleanup();

    return 0;
} 

As you can see it's very timing sensitive. When the bug triggers, the output will be:

Starting first request
curl: Host curl.se:443 was resolved.
curl: IPv6: (none)
curl: IPv4: 151.101.65.91, 151.101.129.91, 151.101.193.91, 151.101.1.91
curl:   Trying 151.101.65.91:443...
curl: schannel: disabled automatic use of client certificate
curl: ALPN: curl offers http/1.1
curl: ALPN: server accepted http/1.1
curl: Connected to curl.se (151.101.65.91) port 443
curl: using HTTP/1.x
curl: Request completely sent off

Starting second request
curl: Re-using existing https: connection with host curl.se
curl: Request completely sent off
curl: Connection #0 to host curl.se left intact
Response code: 404

With the curl_multi_remove_handle line un-commented, the output will be:

Starting first request
curl: Host curl.se:443 was resolved.
curl: IPv6: (none)
curl: IPv4: 151.101.1.91, 151.101.65.91, 151.101.129.91, 151.101.193.91
curl:   Trying 151.101.1.91:443...
curl: closing connection #0

Starting second request
curl: Hostname curl.se was found in DNS cache
curl:   Trying 151.101.1.91:443...
curl: schannel: disabled automatic use of client certificate
curl: ALPN: curl offers http/1.1
curl: ALPN: server accepted http/1.1
curl: Connected to curl.se (151.101.1.91) port 443
curl: using HTTP/1.x
curl: Request completely sent off
curl: Connection #1 to host curl.se left intact
Response code: 200

I'm not sure if all the opts are necessary, but those are the ones we use in our application.

Some additional context: We do call curl_multi_remove_handle in all cases except one edge case that was missed, where we abort a request that is in-progress. Since the edge case in our code was rare, and behavior is timing sensitive, it took a very long time to track down the root cause. All we had to go on was reports from users of very randomly receiving incorrect data.

[icing]

Thanks for the sample! I converted that into a new test case and fixed the code, basically the way you suggested, in PR #17607.

[BrianHarris]

Great news, thank you!

[jay]

Thanks