Run into EXC_BAD_ACCESS crash occasionally at curl_multi_cleanup() when DOH enabled since 8.5.0.

[luozhaohui]

I did this

1. First, enable DoH.
2. Then, initiate a request using curl_multi_*, and call curl_multi_remove_handle() to abort the request.
3. There is a small chance that the test program will trigger an EXC_BAD_ACCESS crash in curl_multi_cleanup().

Dependency versions:

• cares: v1.19.0
• zlib: v1.3.1

Below is a complete C++ test program for this case.

The specific version reproduction situation is as follows:

• v8.3.0, v8.4.0, v8.5.0, v8.6.0, v8.7.0, v8.8.0
  If the enable_DoH in the code is set to false, it will not crash.

• v8.3.0, v8.4.0
  If the enable_DoH in the code is set to true, it will not crash.

• v8.5.0, v8.6.0, v8.7.0, v8.8.0
  If the enable_DoH in the code is set to true, it will crash.

#include <iostream>
#include <iterator>
#include <memory>
#include <ostream>
#include <string>
#include <mutex>
#include <thread>
#include <atomic>
#include <chrono>
#include <string>
#include <string.h>
#include <assert.h>
#include <curl/curl.h>

static bool globalInitCalled = false;

void doInit()
{
    static bool initialized = ([] {
        curl_global_init(CURL_GLOBAL_ALL);
        globalInitCalled = true;
        return true;
    })();

    // silence unused warning
    (void)initialized;
}

enum Method
{
    HEAD,
    GET,
    POST,
    PUT,
};

class Session
{
public:
    enum State
    {
        NEW,
        PERFORMING,
        ABORTED,
        HTTP_OK,
        HTTP_ERR,
        OTHER_ERR
    };

    Session(Method method, const std::string& url)
        : state_(NEW)
        , statusStr_("new")
        , origUrl_(url)
    {
        doInit();
        curl_ = curl_easy_init();

        // Set URL
        setopt(CURLOPT_URL, url.c_str());

        // Set method
        switch (method)
        {
            case HEAD:
                setopt(CURLOPT_NOBODY, 1L);
                break;
            case GET:
                setopt(CURLOPT_HTTPGET, 1L);
                break;
            case POST:
                setopt(CURLOPT_POST, 1L);
                break;
            case PUT:
                setopt(CURLOPT_CUSTOMREQUEST, "PUT");
                setopt(CURLOPT_UPLOAD, 1L);
                break;
        }

        // encoding
        setopt(CURLOPT_ACCEPT_ENCODING, "");
        setopt(CURLOPT_SSL_VERIFYPEER, 0L);
    }

    ~Session()
    {
        if (curl_ != nullptr)
        {
            curl_easy_cleanup(curl_);
            curl_ = nullptr;
        }
    }

    void setAbortFlag(std::shared_ptr<std::atomic_bool> f)
    {
        assert(state_ == NEW);
        isAborted_ = [f]()
        {
            bool ret = f->load();
            if (ret)
            {
                std::cout << "> abort flag set." << std::endl;
            }

            return f->load();
        };
    }

    void enableDoH(const std::string& v)
    {
        assert(state_ == NEW);

        setopt(CURLOPT_DOH_URL, v.c_str());
        setopt(CURLOPT_DOH_SSL_VERIFYPEER, 0);
        setopt(CURLOPT_DOH_SSL_VERIFYHOST, 0);
    }

    bool checkAborted()
    {
        return isAborted_();
    }

    void start()
    {
        assert(state_ == NEW);
        setopt(CURLOPT_PRIVATE, this);
        state_ = PERFORMING;
        statusStr_ = "performing";
    }

    void abort()
    {
        assert(state_ == PERFORMING);
        state_ = ABORTED;
        statusStr_ = "aborted";
    }

    State getState()
    {
        return state_;
    }

    const std::string& getStatusStr() const
    {
        return statusStr_;
    }

    CURL* handle() const
    {
        return curl_;
    }

    void finish(CURLcode res)
    {
        if (res == CURLE_OK)
        {
            state_ = HTTP_OK;
            statusStr_ = "OK";
        }
        else
        {
            state_ = OTHER_ERR;
            statusStr_ = "error";
        }
    }

private:
    template <typename T>
    void setopt(CURLoption opt, T&& v) const
    {
        curl_easy_setopt(curl_, opt, v);
    }

private:
    State state_;
    std::string statusStr_;
    std::string origUrl_;
    CURL* curl_ = nullptr;

    std::function<bool()> isAborted_ = []() {
        return false;
    };
};

class Client
{
public:
    Client()
    {
        doInit();
        curlm_ = curl_multi_init();
    }

    ~Client()
    {
        if (curlm_ != nullptr)
        {
            curl_multi_cleanup(curlm_);
            curlm_ = nullptr;
        }
    }

    std::unique_ptr<Session> get(const std::string& url, bool enable_DoH, std::shared_ptr<std::atomic_bool> v)
    {
        auto session = create(Method::GET, url);
        if (v)
        {
            session->setAbortFlag(v);
        }

        if (enable_DoH)
        {
            session->enableDoH("https://1.1.1.1/dns-query");
        }

        add(*session);

        while (!session->checkAborted() && step() > 0)
        {
            wait();
        }

        abortIfInProgress(*session);

        return std::move(session);
    }

private:
    std::unique_ptr<Session> create(Method method, const std::string& url)
    {
        return std::make_unique<Session>(method, url);
    }

    void add(Session& s)
    {
        s.start();
        curl_multi_add_handle(curlm_, s.handle());
    }

    void remove(Session& s)
    {
        curl_multi_remove_handle(curlm_, s.handle());
    }

    void abortIfInProgress(Session& s)
    {
        if (s.getState() == Session::PERFORMING)
        {
            std::cout << ">> abort session" << std::endl;
            remove(s);
            s.abort();
        }
    }

    void wait(int timeoutMs = 20)
    {
        curl_multi_poll(curlm_, NULL, 0, timeoutMs, NULL);
    }

    int step()
    {
        int active = 0;
        CURLMcode mc = curl_multi_perform(curlm_, &active);
        if (!mc)
        {
            CURLMsg* msg = 0;

            do {
                int nmsg = 0;
                msg = curl_multi_info_read(curlm_, &nmsg);
                if (msg && (msg->msg == CURLMSG_DONE))
                {
                    Session* session = nullptr;
                    curl_easy_getinfo(msg->easy_handle, CURLINFO_PRIVATE, &session);
                    assert(session);
                    session->finish(msg->data.result);
                    remove(*session);
                }

                std::cout << "-- active: " << active << ", number of message: " << nmsg << std::endl;
            }
            while(msg);
        }
        else
        {
            std::cout << "curl_multi_perform() failed. [curl error "
                << mc << "]: " << curl_multi_strerror(mc) << std::endl;
        }

        return active;
    }

private:
    CURLM* curlm_ = nullptr;
};

int main(int argc, char* argv[])
{
    std::cout << "=== curl test ===" << std::endl;

    auto url = "https://httpbin.org/delay/5";
    auto test_abort_func = [=](Client& cli, bool enable_DoH, int abortAfterMs) {
        std::cout << "\n\n=== abort request after " << abortAfterMs << " ms." << std::endl;

        auto abort_flag = std::make_shared<std::atomic_bool>(false);
        auto th = std::thread{[&]()
        {
            std::this_thread::sleep_for(std::chrono::milliseconds(abortAfterMs));
            abort_flag->store(true);
        }};

        auto session = cli.get(url, enable_DoH, abort_flag);
        std::cout << ">> abort test " << abortAfterMs << " ms: " << session->getStatusStr() << std::endl;

        th.join();
    };

    // the program may crash if enable DoH
    bool enable_DoH = true;
    for (uint32_t loop = 0; loop < 50; ++loop)
    {
        for (uint32_t t = 1; t < 500; t = t + 10)
        {
            std::unique_ptr<Client> client(new Client());
            // auto session = client->get(url, false, nullptr);
            // std::cout << ">> normal request " << session->getStatusStr() << std::endl;

            test_abort_func(*client, enable_DoH, t);
        }
    }

    return 0;
}

I expected the following

No EXC_BAD_ACCESS crash.

curl/libcurl version

Tested the following versions:

curl 8.3.0-DEV (x86_64-apple-darwin23.3.0) libcurl/8.3.0-DEV SecureTransport zlib/1.3.1 c-ares/1.19.0
Release-Date: [unreleased]
Protocols: dict file ftp ftps http https
Features: AsynchDNS HSTS HTTPS-proxy IPv6 Largefile libz NTLM NTLM_WB SSL threadsafe UnixSockets

curl 8.4.0 (x86_64-apple-darwin23.3.0) libcurl/8.4.0 SecureTransport zlib/1.3.1 c-ares/1.19.0
Release-Date: 2023-10-11
Protocols: dict file ftp ftps http https
Features: AsynchDNS HSTS HTTPS-proxy IPv6 Largefile libz NTLM SSL threadsafe UnixSockets

curl 8.5.0 (x86_64-apple-darwin23.3.0) libcurl/8.5.0 SecureTransport zlib/1.3.1 c-ares/1.19.0
Release-Date: 2023-12-06
Protocols: dict file ftp ftps http https
Features: AsynchDNS HSTS HTTPS-proxy IPv6 Largefile libz NTLM SSL threadsafe UnixSockets

curl 8.6.0 (x86_64-apple-darwin23.3.0) libcurl/8.6.0 SecureTransport zlib/1.3.1 c-ares/1.19.0
Release-Date: 2024-01-31
Protocols: dict file ftp ftps http https ipfs ipns
Features: AsynchDNS HSTS HTTPS-proxy IPv6 Largefile libz NTLM SSL threadsafe UnixSockets

operating system

MacOS 14.3

This issue occurs on Windows 10 and Ubuntu 1804 too.

[luozhaohui]

Here is the callstack of one crash with curl v8.8.0.

Thread 1 Queue : com.apple.main-thread (serial)
#0	0x000000010003fd10 in multi_done at curl/curl/lib/multi.c:699
#1	0x000000010003f576 in curl_multi_cleanup at curl/curl/lib/multi.c:2891
#2	0x000000010000abcc in Client::~Client() at curl-test/src/main.cpp:202
#3	0x000000010000aba5 in Client::~Client() at curl-test/src/main.cpp:201
#4	0x000000010000ab5b in std::__1::default_delete<Client>::operator()[abi:v160006](Client*) const at /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/__memory/unique_ptr.h:65
#5	0x000000010000aadc in std::__1::unique_ptr<Client, std::__1::default_delete<Client>>::reset[abi:v160006](Client*) at /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/__memory/unique_ptr.h:297
#6	0x000000010000aa79 in std::__1::unique_ptr<Client, std::__1::default_delete<Client>>::~unique_ptr[abi:v160006]() at /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/__memory/unique_ptr.h:263
#7	0x0000000100003275 in std::__1::unique_ptr<Client, std::__1::default_delete<Client>>::~unique_ptr[abi:v160006]() at /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/__memory/unique_ptr.h:263
#8	0x0000000100002e5a in main at curl-test/src/main.cpp:332
#9	0x00007ff80268d386 in start ()

[icing]

Thanks for the detailed report and the sample program. That made reproducing the issue easy.

I have #14212 as a fix for this, making the sample program run without hickups. Hope this works for you too.

[luozhaohui]

@icing Thank you very much. The patch works well.