From 7b43dd4504e1a3764cda2cca23ab77fa0f50b9b9 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 10 Jun 2026 13:44:38 -0500 Subject: [PATCH 01/29] fix: guard HAVE_MAT_LIVEEVENTINSPECTOR/PRIVACYGUARD against redefinition Under -Werror on Linux/macOS, the modules-repo CI (build-posix-latest-exp) has been failing for ~2 weeks with: config-default.h:36: error: 'HAVE_MAT_LIVEEVENTINSPECTOR' macro redefined [-Werror,-Wmacro-redefined] config-default.h:37: error: 'HAVE_MAT_PRIVACYGUARD' macro redefined tests/functests/CMakeLists.txt and tests/unittests/CMakeLists.txt add -DHAVE_MAT_LIVEEVENTINSPECTOR / -DHAVE_MAT_PRIVACYGUARD on the command line when BUILD_LIVEEVENTINSPECTOR / BUILD_PRIVACYGUARD (default YES) and the respective module dir exists. The three config-default headers then redefined them unconditionally, which is fatal under -Werror (added by #1415). Wrapping the two defines in #ifndef in all three config-default*.h headers preserves all existing behavior: - Without command-line -D: macros get defined here as before. - With command-line -D: header skips the redefinition, no warning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/include/mat/config-default-cs4.h | 8 ++++++++ lib/include/mat/config-default-exp.h | 8 ++++++++ lib/include/mat/config-default.h | 8 ++++++++ 3 files changed, 24 insertions(+) diff --git a/lib/include/mat/config-default-cs4.h b/lib/include/mat/config-default-cs4.h index 7aae9fc7e..71a79c10f 100644 --- a/lib/include/mat/config-default-cs4.h +++ b/lib/include/mat/config-default-cs4.h @@ -27,8 +27,16 @@ /* #define HAVE_MAT_EVT_TRACEID */ #define HAVE_MAT_STORAGE #define HAVE_MAT_DEFAULT_HTTP_CLIENT +// The two macros below are also added on the command line by +// tests/{functests,unittests}/CMakeLists.txt when BUILD_LIVEEVENTINSPECTOR +// / BUILD_PRIVACYGUARD are ON. Guard against -Wmacro-redefined under +// -Werror on Linux/macOS. +#ifndef HAVE_MAT_LIVEEVENTINSPECTOR #define HAVE_MAT_LIVEEVENTINSPECTOR +#endif +#ifndef HAVE_MAT_PRIVACYGUARD #define HAVE_MAT_PRIVACYGUARD +#endif //#define HAVE_MAT_DEFAULT_FILTER #if defined(_WIN32) && !defined(_WINRT_DLL) #define HAVE_MAT_NETDETECT diff --git a/lib/include/mat/config-default-exp.h b/lib/include/mat/config-default-exp.h index 609692a01..256dfe615 100644 --- a/lib/include/mat/config-default-exp.h +++ b/lib/include/mat/config-default-exp.h @@ -25,8 +25,16 @@ /* #define HAVE_MAT_EVT_TRACEID */ #define HAVE_MAT_STORAGE #define HAVE_MAT_DEFAULT_HTTP_CLIENT +// The two macros below are also added on the command line by +// tests/{functests,unittests}/CMakeLists.txt when BUILD_LIVEEVENTINSPECTOR +// / BUILD_PRIVACYGUARD are ON. Guard against -Wmacro-redefined under +// -Werror on Linux/macOS. +#ifndef HAVE_MAT_LIVEEVENTINSPECTOR #define HAVE_MAT_LIVEEVENTINSPECTOR +#endif +#ifndef HAVE_MAT_PRIVACYGUARD #define HAVE_MAT_PRIVACYGUARD +#endif //#define HAVE_MAT_DEFAULT_FILTER #if defined(_WIN32) && !defined(_WINRT_DLL) #define HAVE_MAT_NETDETECT diff --git a/lib/include/mat/config-default.h b/lib/include/mat/config-default.h index 9617611c9..2ddce7dfc 100644 --- a/lib/include/mat/config-default.h +++ b/lib/include/mat/config-default.h @@ -33,8 +33,16 @@ /* #define HAVE_MAT_EVT_TRACEID */ #define HAVE_MAT_STORAGE #define HAVE_MAT_DEFAULT_HTTP_CLIENT +// The two macros below are also added on the command line by +// tests/{functests,unittests}/CMakeLists.txt when BUILD_LIVEEVENTINSPECTOR +// / BUILD_PRIVACYGUARD are ON. Guard against -Wmacro-redefined under +// -Werror on Linux/macOS. +#ifndef HAVE_MAT_LIVEEVENTINSPECTOR #define HAVE_MAT_LIVEEVENTINSPECTOR +#endif +#ifndef HAVE_MAT_PRIVACYGUARD #define HAVE_MAT_PRIVACYGUARD +#endif //#define HAVE_MAT_DEFAULT_FILTER #if defined(_WIN32) && !defined(_WINRT_DLL) #define HAVE_MAT_NETDETECT From fc7375aaf2a28566ea0fd1c59e2f67a3f314ba4d Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 11 Jun 2026 03:16:33 -0500 Subject: [PATCH 02/29] fix: prevent EDEADLK self-join in ~CurlHttpOperation on async-thread destruction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The modules-repo CI test ECSClientFuncTests.GetConfigs (and every test in the ECSClientFuncTests suite) crashed on Linux/macOS with: terminate called after throwing an instance of 'std::system_error' what(): Resource deadlock avoided Aborted (core dumped) Root cause ========== SendAsync() runs Send() + the user callback on a std::async worker thread. The callback owns a strong ref to CurlHttpOperation, so when it releases the last ref the ~CurlHttpOperation destructor runs on the async thread itself. libstdc++'s std::future<>::~future implicitly calls _Async_state_impl::~_Async_state_impl, which calls _M_complete_async -> _M_join via std::call_once. On the async thread that's a self-join; call_once throws std::system_error(EDEADLK). Because the throw escapes a noexcept destructor, terminate() aborts the process. A try/catch around the future cannot rescue this — destructors of std::future are noexcept. Fix === Move the future onto a detached helper thread before its destructor runs. The helper is by definition NOT the async thread (we'd only be on the async thread if its work already finished), so the implicit join completes immediately. On the common path (destruction from the caller thread) it costs one short-lived thread spawn that exits in microseconds. Verified locally with sister + modules linked: all 113 FuncTests pass, including all 25 ECSClientFuncTests (which include the formerly-fatal GetConfigs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 972cc4fec..012a2fd99 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -169,11 +169,26 @@ class CurlHttpOperation { */ virtual ~CurlHttpOperation() { - // Given the request has not been aborted we should wait for completion here - // This guarantees the lifetime of this request. + // libstdc++'s std::future<>::~future implicitly joins the async + // thread via call_once during destruction. If this destructor runs + // ON that same async thread (e.g. the user callback released the + // last shared_ptr from inside the lambda), the implicit self-join + // throws std::system_error("Resource deadlock avoided"), and because + // the throw originates inside a noexcept destructor it aborts the + // process. try/catch around `result` cannot rescue it. + // + // Defuse by moving the future onto a detached helper thread that is + // by definition NOT the async thread, so its implicit join succeeds + // immediately (the work has already finished, which is the only way + // we could be running this destructor on the async thread). On the + // common path (destructed from the caller thread) this just spawns + // a no-op helper that exits in microseconds. if (result.valid()) { - result.wait(); + std::thread([f = std::move(result)]() mutable { + // f goes out of scope here. ~future joins on this new + // thread (!= the original async thread), so no EDEADLK. + }).detach(); } DispatchEvent(OnDestroy); res = CURLE_OK; From 3554d8df14784caffb4b91a62c28cd2436cf12d2 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 13 Jun 2026 16:47:37 -0500 Subject: [PATCH 03/29] Make ~CurlHttpOperation detach conditional on self-join (fix UAF regression) Code review found that the previous fix detached the async future's join for EVERY destruction. That removed the cross-thread lifetime guarantee the old result.wait() provided: when the operation is destroyed from another thread while the async Send() is still running, the destructor would proceed to curl_easy_cleanup()/ReleaseResponse() and destroy the by-reference request body while the worker thread is still using them -> use-after-free. Restore the guarantee while keeping the EDEADLK self-join fix: - Record the async task's thread id (atomic) when SendAsync's task starts. - In the destructor, compare std::this_thread::get_id(): * self-join (destroyed from within our own async callback, e.g. EraseRequest drops the last reference): the work is necessarily complete, so defer the future's join to a detached helper thread instead of joining on this (the async) thread, avoiding EDEADLK. * cross-thread: result.wait() to keep the curl handle, response buffer and by-reference request body alive until the async Send() finishes. - Heap-allocate the deferred future first so a rare std::thread spawn failure leaks the already-finished future rather than self-joining (EDEADLK) or letting std::system_error escape this noexcept destructor (std::terminate). - Refresh the stale HttpClient_Curl.cpp lifetime comment. Logic validated with a standalone C++11 repro under AddressSanitizer: the cross-thread path waits (no UAF) and the self-join path does not deadlock. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 5 ++- lib/http/HttpClient_Curl.hpp | 61 ++++++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index b910cdf28..50ab062f4 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -84,7 +84,10 @@ namespace MAT_NS_BEGIN { auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, curlRequest->m_body, false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); curlRequest->SetOperation(curlOperation); - // The lifetime of curlOperation is guarnteed by the call to result.wait() in the d'tor. + // The lifetime of curlOperation across the async Send is guaranteed by + // ~CurlHttpOperation: when this shared_ptr is released from another + // thread it waits for the async result; when the callback below drops + // the last reference (EraseRequest) it defers the join instead. curlOperation->SendAsync([this, callback, requestId](CurlHttpOperation& operation) { this->EraseRequest(requestId); diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 6b5530ad0..5d2d6cfd9 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -173,26 +174,46 @@ class CurlHttpOperation { */ virtual ~CurlHttpOperation() { - // libstdc++'s std::future<>::~future implicitly joins the async - // thread via call_once during destruction. If this destructor runs - // ON that same async thread (e.g. the user callback released the - // last shared_ptr from inside the lambda), the implicit self-join - // throws std::system_error("Resource deadlock avoided"), and because - // the throw originates inside a noexcept destructor it aborts the - // process. try/catch around `result` cannot rescue it. + // libstdc++'s std::future<>::~future implicitly joins the async thread + // during destruction. If this destructor runs ON that same async thread + // (the Send() callback dropped the last reference to us, e.g. via + // EraseRequest), that join is a self-join and throws + // std::system_error("Resource deadlock avoided"); since it originates in + // this noexcept destructor it aborts the process. // - // Defuse by moving the future onto a detached helper thread that is - // by definition NOT the async thread, so its implicit join succeeds - // immediately (the work has already finished, which is the only way - // we could be running this destructor on the async thread). On the - // common path (destructed from the caller thread) this just spawns - // a no-op helper that exits in microseconds. + // Distinguish the two cases by the thread id recorded when the async + // task started: + // * self-join -> the work is necessarily complete; defer the + // future's join to a detached helper thread instead + // of blocking/joining on this (the async) thread. + // * cross-thread -> the async Send() may still be running, so wait() + // to keep the curl handle, response buffer and the + // by-reference request body alive until it finishes. if (result.valid()) { - std::thread([f = std::move(result)]() mutable { - // f goes out of scope here. ~future joins on this new - // thread (!= the original async thread), so no EDEADLK. - }).detach(); + if (std::this_thread::get_id() == m_asyncThreadId.load(std::memory_order_acquire)) + { + // Heap-allocate first so a rare std::thread spawn failure leaks + // the already-finished future rather than joining it on this + // async thread (EDEADLK) or letting std::system_error escape + // this noexcept destructor. + std::future* pending = new (std::nothrow) std::future(std::move(result)); + if (pending != nullptr) + { + try + { + std::thread([pending]() { delete pending; }).detach(); + } + catch (...) + { + // Thread exhaustion: intentionally leak *pending. + } + } + } + else + { + result.wait(); + } } DispatchEvent(OnDestroy); res = CURLE_OK; @@ -334,6 +355,7 @@ class CurlHttpOperation { std::future & SendAsync(std::function callback = nullptr) { result = std::async(std::launch::async, [this, callback] { + m_asyncThreadId.store(std::this_thread::get_id(), std::memory_order_release); long result = Send(); if (callback!=nullptr) callback(*this); @@ -452,6 +474,11 @@ class CurlHttpOperation { CURL *curl; // Local curl instance CURLcode res = CURLE_OK; // Curl result OR HTTP status code if successful + + // Id of the thread running the async Send() task (set when the task starts). + // Lets ~CurlHttpOperation detect a self-join (destruction from within the + // async callback) and avoid the EDEADLK that joining the future would raise. + std::atomic m_asyncThreadId{}; IHttpResponseCallback* m_callback = nullptr; From b10fa89ae55affdea4e45e9cb6bfcc684335840f Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 13 Jun 2026 17:21:28 -0500 Subject: [PATCH 04/29] Address Copilot round 2 on #1481: include , handle nothrow-new failure - Add #include so std::nothrow is not relied on transitively (review). - If new (std::nothrow) returns nullptr (OOM), result stays valid and would self-join (EDEADLK) at end of the noexcept dtor; abort() as a last resort instead of falling through to that, per review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 5d2d6cfd9..c7902a3f2 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -198,16 +199,22 @@ class CurlHttpOperation { // async thread (EDEADLK) or letting std::system_error escape // this noexcept destructor. std::future* pending = new (std::nothrow) std::future(std::move(result)); - if (pending != nullptr) + if (pending == nullptr) { - try - { - std::thread([pending]() { delete pending; }).detach(); - } - catch (...) - { - // Thread exhaustion: intentionally leak *pending. - } + // Out of memory: `result` is still valid and would self-join + // (EDEADLK) when destroyed on this async thread at the end of + // the destructor, and there is no allocation-free way to move + // it off-thread. Abort as a last resort rather than fall + // through to a guaranteed EDEADLK abort. + std::abort(); + } + try + { + std::thread([pending]() { delete pending; }).detach(); + } + catch (...) + { + // Thread exhaustion: intentionally leak *pending. } } else From 3dda53bc8ce2c0fb6f3c016eb542aac84fcc36dd Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 13 Jun 2026 17:37:50 -0500 Subject: [PATCH 05/29] Address Copilot round 3 on #1481: avoid atomic, fix lifetime comments - Replace std::atomic (not guaranteed supported across standard libraries) with a plain std::thread::id published via an std::atomic flag using release/acquire ordering. - Correct the lifetime comments: the operation's last shared_ptr is held by the owning CurlHttpRequest (via SetOperation), not by EraseRequest (which only removes the raw id from m_requests). The self-join occurs when the async callback leads to that request being destroyed on the async thread (OnHttpResponse -> EventsUploadContext::clear()). Re-validated the wait-vs-detach logic with a standalone C++11 repro under AddressSanitizer + UBSan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 9 ++++++--- lib/http/HttpClient_Curl.hpp | 29 ++++++++++++++++++----------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index 50ab062f4..e8c620d61 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -85,9 +85,12 @@ namespace MAT_NS_BEGIN { curlRequest->SetOperation(curlOperation); // The lifetime of curlOperation across the async Send is guaranteed by - // ~CurlHttpOperation: when this shared_ptr is released from another - // thread it waits for the async result; when the callback below drops - // the last reference (EraseRequest) it defers the join instead. + // ~CurlHttpOperation. After this function returns, the only remaining + // shared_ptr is the one held by the owning CurlHttpRequest. When that + // request is destroyed from another thread, the destructor waits for the + // async result; if the callback below leads to the request being + // destroyed on the async thread itself (OnHttpResponse -> + // EventsUploadContext::clear()), the destructor defers the join instead. curlOperation->SendAsync([this, callback, requestId](CurlHttpOperation& operation) { this->EraseRequest(requestId); diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index c7902a3f2..58ec0c85c 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -177,12 +177,13 @@ class CurlHttpOperation { { // libstdc++'s std::future<>::~future implicitly joins the async thread // during destruction. If this destructor runs ON that same async thread - // (the Send() callback dropped the last reference to us, e.g. via - // EraseRequest), that join is a self-join and throws - // std::system_error("Resource deadlock avoided"); since it originates in - // this noexcept destructor it aborts the process. + // (the async callback led to the owning CurlHttpRequest being destroyed + // on that thread, e.g. OnHttpResponse -> EventsUploadContext::clear()), + // that join is a self-join and throws std::system_error("Resource + // deadlock avoided"); since it originates in this noexcept destructor it + // aborts the process. // - // Distinguish the two cases by the thread id recorded when the async + // Distinguish the two cases by the thread id published when the async // task started: // * self-join -> the work is necessarily complete; defer the // future's join to a detached helper thread instead @@ -192,7 +193,8 @@ class CurlHttpOperation { // by-reference request body alive until it finishes. if (result.valid()) { - if (std::this_thread::get_id() == m_asyncThreadId.load(std::memory_order_acquire)) + if (m_asyncThreadIdSet.load(std::memory_order_acquire) && + std::this_thread::get_id() == m_asyncThreadId) { // Heap-allocate first so a rare std::thread spawn failure leaks // the already-finished future rather than joining it on this @@ -362,7 +364,8 @@ class CurlHttpOperation { std::future & SendAsync(std::function callback = nullptr) { result = std::async(std::launch::async, [this, callback] { - m_asyncThreadId.store(std::this_thread::get_id(), std::memory_order_release); + m_asyncThreadId = std::this_thread::get_id(); + m_asyncThreadIdSet.store(true, std::memory_order_release); long result = Send(); if (callback!=nullptr) callback(*this); @@ -482,10 +485,14 @@ class CurlHttpOperation { CURL *curl; // Local curl instance CURLcode res = CURLE_OK; // Curl result OR HTTP status code if successful - // Id of the thread running the async Send() task (set when the task starts). - // Lets ~CurlHttpOperation detect a self-join (destruction from within the - // async callback) and avoid the EDEADLK that joining the future would raise. - std::atomic m_asyncThreadId{}; + // Id of the thread running the async Send() task, published via the + // atomic flag below (release/acquire). ~CurlHttpOperation uses these + // to detect a self-join (destruction from within the async callback) and + // avoid the EDEADLK that joining the future would raise. A plain thread::id + // plus an atomic flag is used instead of std::atomic, + // which is not guaranteed to be supported across standard libraries. + std::thread::id m_asyncThreadId{}; + std::atomic m_asyncThreadIdSet{ false }; IHttpResponseCallback* m_callback = nullptr; From 7e22ed22d85329419bbc7727241ccecf60b36d66 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 13 Jun 2026 17:50:09 -0500 Subject: [PATCH 06/29] Address Copilot round 4 on #1481: precise self-join comment, reset flag on reuse - Reword the self-join comment: in that case Send() has returned (we are in its callback) but the async task itself has not yet returned (the destructor runs inside it), so the deferred helper's ~future join completes only after this destructor unwinds. Avoids implying the async task is already finished. - Reset m_asyncThreadIdSet to false at the start of SendAsync so self-join detection stays correct if the operation were ever reused (it is single-use today: one SendAsync per request). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 58ec0c85c..33d217924 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -185,9 +185,13 @@ class CurlHttpOperation { // // Distinguish the two cases by the thread id published when the async // task started: - // * self-join -> the work is necessarily complete; defer the - // future's join to a detached helper thread instead - // of blocking/joining on this (the async) thread. + // * self-join -> Send() has returned (we are running inside its + // callback), but the async task itself has not yet + // returned (this destructor is executing inside it), + // so defer the future's join to a detached helper + // thread rather than joining on this (the async) + // thread; the helper's join completes once the task + // returns after this destructor unwinds. // * cross-thread -> the async Send() may still be running, so wait() // to keep the curl handle, response buffer and the // by-reference request body alive until it finishes. @@ -363,6 +367,10 @@ class CurlHttpOperation { } std::future & SendAsync(std::function callback = nullptr) { + // Reset the publication flag before launching so self-join detection + // stays correct even if this operation were ever reused (today each + // CurlHttpOperation is single-use: one SendAsync call per request). + m_asyncThreadIdSet.store(false, std::memory_order_release); result = std::async(std::launch::async, [this, callback] { m_asyncThreadId = std::this_thread::get_id(); m_asyncThreadIdSet.store(true, std::memory_order_release); From 150e376ee35552b44b7531bbb0d775574fd637b5 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 16:25:07 -0500 Subject: [PATCH 07/29] Replace std::async with a self-keepalive detached worker (real fix for #1481) The EDEADLK self-join was a symptom of using std::async(std::launch::async) for the HTTP send: the returned std::future joins its worker thread on destruction, so when the async callback caused the operation to be destroyed on that same worker thread (OnHttpResponse -> EventsUploadContext::clear()), ~future self-joined and aborted the process out of the noexcept destructor. Rather than detect-and-defer that self-join (the previous approach: published thread id + atomic flag + heap-move the future to a detached helper, with OOM/ thread-exhaustion fallbacks), remove the joining future entirely: - CurlHttpOperation now derives from enable_shared_from_this. SendAsync runs Send() on a detached std::thread that holds a shared_ptr keepalive to the operation, so the operation (and its curl handle, response buffer, and by-reference request body) stays alive until the worker finishes -- the same lifetime guarantee the destructor's result.wait() used to provide. - There is no future, so ~CurlHttpOperation never joins anything and is safe on any thread, including the worker thread itself. The destructor drops to plain curl cleanup. - Removes the future member, the m_asyncThreadId/m_asyncThreadIdSet machinery, and the / includes. Net -54 lines in the client. Adds HttpClientCurlTests.SendAsync_DestroyOnWorkerThread_NoSelfJoin, which drops the last external reference from inside the callback (on the worker thread) -- the exact #1481 trigger. It aborts the process on the old std::async code and passes on this fix. Verified on Linux GCC 13: all HttpClientCurlTests (12) pass including the new regression; the full FuncTests suite (39) passes with the curl client. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 17 ++-- lib/http/HttpClient_Curl.hpp | 107 ++++++------------------ tests/unittests/HttpClientCurlTests.cpp | 41 +++++++++ 3 files changed, 76 insertions(+), 89 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index e8c620d61..3c4ca31ad 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -83,14 +83,15 @@ namespace MAT_NS_BEGIN { auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, curlRequest->m_body, false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); curlRequest->SetOperation(curlOperation); - - // The lifetime of curlOperation across the async Send is guaranteed by - // ~CurlHttpOperation. After this function returns, the only remaining - // shared_ptr is the one held by the owning CurlHttpRequest. When that - // request is destroyed from another thread, the destructor waits for the - // async result; if the callback below leads to the request being - // destroyed on the async thread itself (OnHttpResponse -> - // EventsUploadContext::clear()), the destructor defers the join instead. + + // The async Send() runs on a detached worker that holds its own shared_ptr + // to curlOperation (see CurlHttpOperation::SendAsync), so the operation -- + // and its curl handle, response buffer and by-reference request body -- stay + // alive until Send() and the callback below have finished, regardless of + // when the owning CurlHttpRequest is released. If the callback leads to that + // request being destroyed on the worker thread (OnHttpResponse -> + // EventsUploadContext::clear()), the operation is simply destroyed there + // once the worker returns; there is no future to join. curlOperation->SendAsync([this, callback, requestId](CurlHttpOperation& operation) { this->EraseRequest(requestId); diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 33d217924..f8c2e21c1 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -20,10 +20,9 @@ #include #include -#include #include #include -#include +#include #include #include @@ -71,7 +70,7 @@ class HttpClient_Curl : public IHttpClient { std::string m_sslCaInfo; }; -class CurlHttpOperation { +class CurlHttpOperation : public std::enable_shared_from_this { public: void DispatchEvent(HttpStateEvent type) @@ -175,59 +174,13 @@ class CurlHttpOperation { */ virtual ~CurlHttpOperation() { - // libstdc++'s std::future<>::~future implicitly joins the async thread - // during destruction. If this destructor runs ON that same async thread - // (the async callback led to the owning CurlHttpRequest being destroyed - // on that thread, e.g. OnHttpResponse -> EventsUploadContext::clear()), - // that join is a self-join and throws std::system_error("Resource - // deadlock avoided"); since it originates in this noexcept destructor it - // aborts the process. - // - // Distinguish the two cases by the thread id published when the async - // task started: - // * self-join -> Send() has returned (we are running inside its - // callback), but the async task itself has not yet - // returned (this destructor is executing inside it), - // so defer the future's join to a detached helper - // thread rather than joining on this (the async) - // thread; the helper's join completes once the task - // returns after this destructor unwinds. - // * cross-thread -> the async Send() may still be running, so wait() - // to keep the curl handle, response buffer and the - // by-reference request body alive until it finishes. - if (result.valid()) - { - if (m_asyncThreadIdSet.load(std::memory_order_acquire) && - std::this_thread::get_id() == m_asyncThreadId) - { - // Heap-allocate first so a rare std::thread spawn failure leaks - // the already-finished future rather than joining it on this - // async thread (EDEADLK) or letting std::system_error escape - // this noexcept destructor. - std::future* pending = new (std::nothrow) std::future(std::move(result)); - if (pending == nullptr) - { - // Out of memory: `result` is still valid and would self-join - // (EDEADLK) when destroyed on this async thread at the end of - // the destructor, and there is no allocation-free way to move - // it off-thread. Abort as a last resort rather than fall - // through to a guaranteed EDEADLK abort. - std::abort(); - } - try - { - std::thread([pending]() { delete pending; }).detach(); - } - catch (...) - { - // Thread exhaustion: intentionally leak *pending. - } - } - else - { - result.wait(); - } - } + // The async Send() runs on a detached worker that holds a shared_ptr to + // this operation (see SendAsync), so this destructor runs only after that + // worker has finished and released its reference. The curl handle, response + // buffer and by-reference request body are therefore no longer in use. + // There is no future to join, so destruction is safe on any thread -- + // including the worker thread itself, which is where it happens when the + // callback drops the last other reference (issue #1481). DispatchEvent(OnDestroy); res = CURLE_OK; curl_easy_cleanup(curl); @@ -366,20 +319,23 @@ class CurlHttpOperation { return res; } - std::future & SendAsync(std::function callback = nullptr) { - // Reset the publication flag before launching so self-join detection - // stays correct even if this operation were ever reused (today each - // CurlHttpOperation is single-use: one SendAsync call per request). - m_asyncThreadIdSet.store(false, std::memory_order_release); - result = std::async(std::launch::async, [this, callback] { - m_asyncThreadId = std::this_thread::get_id(); - m_asyncThreadIdSet.store(true, std::memory_order_release); - long result = Send(); - if (callback!=nullptr) - callback(*this); - return result; - }); - return result; + void SendAsync(std::function callback = nullptr) { + // Run the blocking Send() on a detached worker that keeps this operation + // alive for the duration by holding a shared_ptr to itself. This replaces + // std::async, whose returned future joins its worker thread on destruction: + // when the callback below caused this operation to be destroyed on the + // async thread (OnHttpResponse -> EventsUploadContext::clear()), that join + // was a self-join and raised std::system_error("Resource deadlock avoided") + // out of the noexcept destructor, aborting the process (issue #1481). With + // the self-keepalive there is no future and no join: the worker simply + // exits, releasing the last reference, and ~CurlHttpOperation runs + // trivially on whichever thread drops it. + auto self = shared_from_this(); + std::thread([self, callback]() { + self->Send(); + if (callback != nullptr) + callback(*self); + }).detach(); } /** @@ -493,15 +449,6 @@ class CurlHttpOperation { CURL *curl; // Local curl instance CURLcode res = CURLE_OK; // Curl result OR HTTP status code if successful - // Id of the thread running the async Send() task, published via the - // atomic flag below (release/acquire). ~CurlHttpOperation uses these - // to detect a self-join (destruction from within the async callback) and - // avoid the EDEADLK that joining the future would raise. A plain thread::id - // plus an atomic flag is used instead of std::atomic, - // which is not guaranteed to be supported across standard libraries. - std::thread::id m_asyncThreadId{}; - std::atomic m_asyncThreadIdSet{ false }; - IHttpResponseCallback* m_callback = nullptr; // Request values @@ -528,8 +475,6 @@ class CurlHttpOperation { size_t sendlen = 0; // # bytes sent by client size_t acklen = 0; // # bytes ack by server - std::future result; - /** * Helper routine to wait for data on socket * diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index c9894b90d..17b4f1eb7 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -12,6 +12,10 @@ #include "http/HttpClient_Curl.hpp" #include "config/RuntimeConfig_Default.hpp" +#include +#include +#include + using namespace testing; using namespace MAT; @@ -126,4 +130,41 @@ TEST_F(HttpClientCurlTests, SetSslVerification_ConcurrentCallsNoRace) SUCCEED(); } +// --- Regression: issue #1481 (EDEADLK self-join in ~CurlHttpOperation) --- + +// When the async callback drops the last *external* reference to the operation, +// ~CurlHttpOperation runs on the worker thread. The old std::async design joined +// its own future there (self-join) and aborted the process with +// std::system_error("Resource deadlock avoided"). The worker now holds a +// shared_ptr keepalive and there is no future, so destruction on the worker thread +// is trivial and safe. This test aborts the process on the old code and passes on +// the fix. +TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) +{ + std::promise callbackDone; + auto done = callbackDone.get_future(); + + // Closed local port -> Send() fails fast (connection refused), no network wait. + auto op = std::make_shared( + "GET", "http://127.0.0.1:9/", nullptr, m_headers, m_body, + false, 1 /*connTimeout*/, false /*sslVerify*/, ""); + + // Move the only external reference into a heap box the callback will delete, + // then release our own reference. After SendAsync the live references are the + // box and the worker's keepalive. + auto* box = new std::shared_ptr(std::move(op)); + + (*box)->SendAsync([box, &callbackDone](CurlHttpOperation&) { + // Runs on the worker thread. Drop the last external reference here. On the + // old code this destroyed the operation on this thread and self-joined its + // own future -> abort. With the keepalive fix the worker still holds a + // reference, so this is safe and the operation is destroyed once the worker + // returns. + delete box; + callbackDone.set_value(); + }); + + ASSERT_EQ(done.wait_for(std::chrono::seconds(15)), std::future_status::ready); +} + #endif // MATSDK_PAL_CPP11 && !_MSC_VER && HAVE_MAT_DEFAULT_HTTP_CLIENT From a12525e9c7525131dc89f9371ef2fb1dafbc2c07 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 16:59:16 -0500 Subject: [PATCH 08/29] Address Copilot round on #1481: own the body, catch worker exceptions, tidy test - requestBody use-after-free (comments 1 & 3): the old blocking destructor kept the by-reference body alive because destroying the request waited for Send(). With the self-keepalive worker the operation can outlive the request, so a reference into CurlHttpRequest::m_body could dangle mid-send. CurlHttpOperation now takes the body by value and owns it, so it is valid for the operation's whole lifetime regardless of when the request is released. Costs one body copy per request (the prior zero-copy relied on the blocking wait that caused #1481). - Detached-worker exceptions (comment 2): an exception escaping Send()/callback would call std::terminate, whereas the old std::async captured (and effectively swallowed) it. Wrap the worker body in try/catch to preserve the non-terminating behavior. - Test (comment 4): replace the raw new/delete shared_ptr box with a shared_ptr> whose contained pointer is reset in the callback, so it cannot leak if SendAsync throws. Verified on Linux GCC 13: all HttpClientCurlTests (12) pass including the self-join regression; full FuncTests (39) pass with the by-value body. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 8 ++--- lib/http/HttpClient_Curl.hpp | 43 +++++++++++++++++-------- tests/unittests/HttpClientCurlTests.cpp | 10 +++--- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index 3c4ca31ad..a1554f305 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -86,10 +86,10 @@ namespace MAT_NS_BEGIN { // The async Send() runs on a detached worker that holds its own shared_ptr // to curlOperation (see CurlHttpOperation::SendAsync), so the operation -- - // and its curl handle, response buffer and by-reference request body -- stay - // alive until Send() and the callback below have finished, regardless of - // when the owning CurlHttpRequest is released. If the callback leads to that - // request being destroyed on the worker thread (OnHttpResponse -> + // and its curl handle, response buffer and owned copy of the request body -- + // stay alive until Send() and the callback below have finished, regardless + // of when the owning CurlHttpRequest is released. If the callback leads to + // that request being destroyed on the worker thread (OnHttpResponse -> // EventsUploadContext::clear()), the operation is simply destroyed there // once the worker returns; there is no future to join. curlOperation->SendAsync([this, callback, requestId](CurlHttpOperation& operation) { diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index f8c2e21c1..97937c890 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -95,11 +96,13 @@ class CurlHttpOperation : public std::enable_shared_from_this std::string method, std::string url, IHttpResponseCallback* callback, - // requestHeaders is copied into the curl_slist during construction - // and need not outlive this operation. requestBody is stored by - // reference and read by Send(), so it must outlive this operation. + // requestHeaders is copied into the curl_slist during construction and + // need not outlive this operation. requestBody is taken by value and + // owned by this operation: the detached worker in SendAsync can outlive + // the caller's request, so a reference into it could dangle during + // Send() (issue #1481). const std::map& requestHeaders, - const std::vector& requestBody, + std::vector requestBody, // Default connectivity and response size options bool rawResponse = false, size_t httpConnTimeout = HTTP_CONN_TIMEOUT, @@ -117,7 +120,7 @@ class CurlHttpOperation : public std::enable_shared_from_this m_sslCaInfo(sslCaInfo), // Local vars - requestBody(requestBody) + requestBody(std::move(requestBody)) { TRACE("--------------------------------------------------------------------------------------------------\n"); response.memory = nullptr; @@ -332,9 +335,24 @@ class CurlHttpOperation : public std::enable_shared_from_this // trivially on whichever thread drops it. auto self = shared_from_this(); std::thread([self, callback]() { - self->Send(); - if (callback != nullptr) - callback(*self); + // The worker is detached, so an escaping exception would call + // std::terminate. std::async previously captured exceptions in the + // (never-get()) future, i.e. swallowed them; preserve that by + // catching here so a throwing Send()/callback cannot crash the process. + try + { + self->Send(); + if (callback != nullptr) + callback(*self); + } + catch (const std::exception& e) + { + TRACE("CurlHttpOperation worker terminated by exception: %s\n", e.what()); + } + catch (...) + { + TRACE("CurlHttpOperation worker terminated by unknown exception\n"); + } }).detach(); } @@ -455,11 +473,10 @@ class CurlHttpOperation : public std::enable_shared_from_this std::string m_method; std::string m_url; std::string m_sslCaInfo; - // The SDK upload path keeps the owning IHttpRequest alive through the - // callback context until Send() completes; copying this body would duplicate - // every upload payload. Unlike CURLOPT_CAINFO, the body pointer is set and - // consumed during Send(), not retained from construction. - const std::vector& requestBody; + // Owned copy of the request body, read by Send(). Owned (not a reference into + // the caller's IHttpRequest) because the detached worker in SendAsync can + // outlive that request, so a reference could dangle mid-send (issue #1481). + std::vector requestBody; struct curl_slist *m_headersChunk = nullptr; // Processed response headers and body diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 17b4f1eb7..109a38e7b 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -149,10 +149,10 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) "GET", "http://127.0.0.1:9/", nullptr, m_headers, m_body, false, 1 /*connTimeout*/, false /*sslVerify*/, ""); - // Move the only external reference into a heap box the callback will delete, - // then release our own reference. After SendAsync the live references are the - // box and the worker's keepalive. - auto* box = new std::shared_ptr(std::move(op)); + // A shared box holds the only external reference. The callback resets the + // contained shared_ptr (on the worker thread) to drop the last external + // reference -- the exact #1481 trigger -- without raw new/delete. + auto box = std::make_shared>(std::move(op)); (*box)->SendAsync([box, &callbackDone](CurlHttpOperation&) { // Runs on the worker thread. Drop the last external reference here. On the @@ -160,7 +160,7 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) // own future -> abort. With the keepalive fix the worker still holds a // reference, so this is safe and the operation is destroyed once the worker // returns. - delete box; + box->reset(); callbackDone.set_value(); }); From 23486a6f6f149496374db89276f6c2e861cd1b63 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 17:50:00 -0500 Subject: [PATCH 09/29] Address Copilot round 2 on #1481: move body, deterministic test host, tidy comment HttpClient_Curl.cpp:84 (comment 3547544648): the operation takes the request body by value, so hand it curlRequest->m_body via std::move instead of copying. m_body is a per-send copy of the EventsUploadContext body (the retry source of truth), so moving it is safe and avoids duplicating peak upload memory. HttpClientCurlTests.cpp:150 (comment 3547544635): replace the fixed port 9 URL with an RFC 6761 .invalid host so Send() fails fast and deterministically on any environment (a fixed port could happen to be open). connTimeout=1 still bounds it. HttpClient_Curl.hpp:183 (comment 3547544604): the destructor comment now says the request body is owned (by value), not by-reference, matching the current design. Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass (incl. SendAsync_DestroyOnWorkerThread_NoSelfJoin) and full FuncTests 39/39 pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 7 ++++++- lib/http/HttpClient_Curl.hpp | 2 +- tests/unittests/HttpClientCurlTests.cpp | 6 ++++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index a1554f305..9e073e7b6 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -81,7 +81,12 @@ namespace MAT_NS_BEGIN { sslCaInfo = m_sslCaInfo; } - auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, curlRequest->m_body, false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); + // Move the request body into the operation (it is taken by value there): + // curlRequest->m_body is a per-send copy of the EventsUploadContext body + // (the retry source of truth), so moving it avoids duplicating large upload + // payloads while still giving the operation an owned buffer for its detached + // worker (issue #1481). + auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, std::move(curlRequest->m_body), false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); curlRequest->SetOperation(curlOperation); // The async Send() runs on a detached worker that holds its own shared_ptr diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 97937c890..700999f62 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -180,7 +180,7 @@ class CurlHttpOperation : public std::enable_shared_from_this // The async Send() runs on a detached worker that holds a shared_ptr to // this operation (see SendAsync), so this destructor runs only after that // worker has finished and released its reference. The curl handle, response - // buffer and by-reference request body are therefore no longer in use. + // buffer and owned request body are therefore no longer in use. // There is no future to join, so destruction is safe on any thread -- // including the worker thread itself, which is where it happens when the // callback drops the last other reference (issue #1481). diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 109a38e7b..2db42745a 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -144,9 +144,11 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) std::promise callbackDone; auto done = callbackDone.get_future(); - // Closed local port -> Send() fails fast (connection refused), no network wait. + // Host under the RFC 6761 reserved .invalid TLD never resolves, so Send() fails + // fast and deterministically (name resolution error) on any environment -- + // unlike a fixed port, which could happen to be open. connTimeout=1 bounds it. auto op = std::make_shared( - "GET", "http://127.0.0.1:9/", nullptr, m_headers, m_body, + "GET", "http://selfjoin.regression.invalid/", nullptr, m_headers, m_body, false, 1 /*connTimeout*/, false /*sslVerify*/, ""); // A shared box holds the only external reference. The callback resets the From 4ccc9ea7f15afa2e3f869468dff0f51473d877e8 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 18:15:52 -0500 Subject: [PATCH 10/29] Address Copilot round 3 on #1481: guard worker-thread start, harden test promise HttpClient_Curl.hpp SendAsync (comment 3547753859): if std::thread creation throws (e.g. resource exhaustion) the exception previously escaped SendAsync(), which both violates the IHttpClient::SendRequestAsync contract that the callback is always invoked and, on the PAL worker thread (no try/catch), would terminate the process. The worker body is now a named lambda; thread start is wrapped in try/catch and on failure the operation runs synchronously as a fallback so the callback still fires and no exception escapes. HttpClientCurlTests.cpp (comment 3547753886): the regression test captured the stack std::promise by reference, so if the ASSERT timed out and the test returned early, the detached worker could call set_value() on a destroyed promise. The promise is now heap-owned (shared_ptr) and captured by value, so an early return cannot turn into a use-after-scope. Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass and FuncTests compiles clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 16 ++++++++++++++-- tests/unittests/HttpClientCurlTests.cpp | 11 +++++++---- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 700999f62..648d852db 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -334,7 +334,7 @@ class CurlHttpOperation : public std::enable_shared_from_this // exits, releasing the last reference, and ~CurlHttpOperation runs // trivially on whichever thread drops it. auto self = shared_from_this(); - std::thread([self, callback]() { + auto worker = [self, callback]() { // The worker is detached, so an escaping exception would call // std::terminate. std::async previously captured exceptions in the // (never-get()) future, i.e. swallowed them; preserve that by @@ -353,7 +353,19 @@ class CurlHttpOperation : public std::enable_shared_from_this { TRACE("CurlHttpOperation worker terminated by unknown exception\n"); } - }).detach(); + }; + try + { + std::thread(worker).detach(); + } + catch (const std::system_error& e) + { + // Starting the worker thread failed (e.g. resource exhaustion). Run the + // operation synchronously as a fallback so the IHttpClient callback is + // still always invoked and the exception does not escape SendAsync(). + TRACE("CurlHttpOperation could not start worker thread: %s; running synchronously\n", e.what()); + worker(); + } } /** diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 2db42745a..f24a823f4 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -141,8 +141,11 @@ TEST_F(HttpClientCurlTests, SetSslVerification_ConcurrentCallsNoRace) // the fix. TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) { - std::promise callbackDone; - auto done = callbackDone.get_future(); + // Heap-owned promise so a captured copy keeps it alive: if the ASSERT below + // fails and the test returns early, the still-detached worker can safely call + // set_value() on it instead of touching a destroyed stack promise. + auto callbackDone = std::make_shared>(); + auto done = callbackDone->get_future(); // Host under the RFC 6761 reserved .invalid TLD never resolves, so Send() fails // fast and deterministically (name resolution error) on any environment -- @@ -156,14 +159,14 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) // reference -- the exact #1481 trigger -- without raw new/delete. auto box = std::make_shared>(std::move(op)); - (*box)->SendAsync([box, &callbackDone](CurlHttpOperation&) { + (*box)->SendAsync([box, callbackDone](CurlHttpOperation&) { // Runs on the worker thread. Drop the last external reference here. On the // old code this destroyed the operation on this thread and self-joined its // own future -> abort. With the keepalive fix the worker still holds a // reference, so this is safe and the operation is destroyed once the worker // returns. box->reset(); - callbackDone.set_value(); + callbackDone->set_value(); }); ASSERT_EQ(done.wait_for(std::chrono::seconds(15)), std::future_status::ready); From f9262e01d3a5fa53d59a705cbb0b08e1e5a0d6dd Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 20:26:28 -0500 Subject: [PATCH 11/29] Address Copilot round 5 on #1481: include, broaden thread-start catch, fix test comment HttpClient_Curl.hpp (comment 3548205832): WaitOnSocket() uses std::numeric_limits but the header only included , not -- it had relied on (removed by this PR) to pull transitively. Added an explicit include so the header is self-contained. HttpClient_Curl.hpp SendAsync (comment 3548205850): the thread-start fallback only caught std::system_error, but std::thread construction can also throw std::bad_alloc while allocating the callable. Broadened the catch to const std::exception& so any thread-start failure still falls back to a synchronous run and never escapes SendAsync() (which would terminate on the PAL worker thread). HttpClientCurlTests.cpp (comment 3548205863): dropped the misleading "connTimeout=1 bounds it" note -- CurlHttpOperation ignores its httpConnTimeout arg (WaitOnSocket uses the HTTP_CONN_TIMEOUT constant), so the .invalid host's immediate name- resolution failure, not the timeout, is what makes Send() fail fast. Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 11 +++++++---- tests/unittests/HttpClientCurlTests.cpp | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 648d852db..b23c13d70 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -358,11 +359,13 @@ class CurlHttpOperation : public std::enable_shared_from_this { std::thread(worker).detach(); } - catch (const std::system_error& e) + catch (const std::exception& e) { - // Starting the worker thread failed (e.g. resource exhaustion). Run the - // operation synchronously as a fallback so the IHttpClient callback is - // still always invoked and the exception does not escape SendAsync(). + // Starting the worker thread failed -- std::thread construction can throw + // std::system_error (e.g. resource exhaustion) or std::bad_alloc while + // allocating the callable. Run the operation synchronously as a fallback + // so the IHttpClient callback is still always invoked and the exception + // does not escape SendAsync(). TRACE("CurlHttpOperation could not start worker thread: %s; running synchronously\n", e.what()); worker(); } diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index f24a823f4..8099bcf67 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -149,7 +149,7 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) // Host under the RFC 6761 reserved .invalid TLD never resolves, so Send() fails // fast and deterministically (name resolution error) on any environment -- - // unlike a fixed port, which could happen to be open. connTimeout=1 bounds it. + // unlike a fixed port, which could happen to be open. auto op = std::make_shared( "GET", "http://selfjoin.regression.invalid/", nullptr, m_headers, m_body, false, 1 /*connTimeout*/, false /*sslVerify*/, ""); From a1da06f83bb4f334aa4b0b9021596797a9f8403c Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 20:59:24 -0500 Subject: [PATCH 12/29] Address Copilot round 6 on #1481: guard shared_from_this() in SendAsync Comment 3548251461: SendAsync() called shared_from_this() unconditionally. Every CurlHttpOperation is created via make_shared (HttpClient_Curl.cpp:89), so this is safe today, but if a future caller ever constructs one outside a shared_ptr (stack / unique_ptr) shared_from_this() throws std::bad_weak_ptr, which would escape SendAsync() BEFORE the thread-start try/catch and could terminate the caller thread -- breaking the "SendAsync never lets an exception escape / the callback is always invoked" property established in the earlier rounds. Guarded shared_from_this() with a std::bad_weak_ptr catch that falls back to a synchronous run (the caller owns the non-shared object for the duration). Also extracted the shared Send()+callback body into RunSendAndCallback() so the detached worker, the thread-start fallback, and this new no-shared fallback all use one implementation. Added regression test SendAsync_NotSharedOwned_RunsSynchronouslyNoThrow. Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 61 ++++++++++++++++--------- tests/unittests/HttpClientCurlTests.cpp | 18 ++++++++ 2 files changed, 58 insertions(+), 21 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index b23c13d70..c47b7f783 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -323,6 +323,28 @@ class CurlHttpOperation : public std::enable_shared_from_this return res; } + // Runs the blocking Send() and then the callback, swallowing any exception. + // A detached worker must not let an exception escape (that would call + // std::terminate), and std::async previously captured exceptions in its + // never-observed future; this preserves that. Shared by the detached worker + // and the synchronous fallbacks in SendAsync(). + void RunSendAndCallback(const std::function& callback) { + try + { + Send(); + if (callback != nullptr) + callback(*this); + } + catch (const std::exception& e) + { + TRACE("CurlHttpOperation worker terminated by exception: %s\n", e.what()); + } + catch (...) + { + TRACE("CurlHttpOperation worker terminated by unknown exception\n"); + } + } + void SendAsync(std::function callback = nullptr) { // Run the blocking Send() on a detached worker that keeps this operation // alive for the duration by holding a shared_ptr to itself. This replaces @@ -334,27 +356,24 @@ class CurlHttpOperation : public std::enable_shared_from_this // the self-keepalive there is no future and no join: the worker simply // exits, releasing the last reference, and ~CurlHttpOperation runs // trivially on whichever thread drops it. - auto self = shared_from_this(); - auto worker = [self, callback]() { - // The worker is detached, so an escaping exception would call - // std::terminate. std::async previously captured exceptions in the - // (never-get()) future, i.e. swallowed them; preserve that by - // catching here so a throwing Send()/callback cannot crash the process. - try - { - self->Send(); - if (callback != nullptr) - callback(*self); - } - catch (const std::exception& e) - { - TRACE("CurlHttpOperation worker terminated by exception: %s\n", e.what()); - } - catch (...) - { - TRACE("CurlHttpOperation worker terminated by unknown exception\n"); - } - }; + std::shared_ptr self; + try + { + self = shared_from_this(); + } + catch (const std::bad_weak_ptr&) + { + // The detached-worker self-keepalive requires this operation to be owned + // by a std::shared_ptr (it always is in practice -- created via + // make_shared in HttpClient_Curl.cpp). If a future caller ever constructs + // one outside a shared_ptr (stack / unique_ptr), shared_from_this() throws; + // fall back to a synchronous run on the caller's thread rather than letting + // std::bad_weak_ptr escape SendAsync(). The caller owns the object for the + // duration and the callback is still invoked. + RunSendAndCallback(callback); + return; + } + auto worker = [self, callback]() { self->RunSendAndCallback(callback); }; try { std::thread(worker).detach(); diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 8099bcf67..f3d39af88 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -172,4 +172,22 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) ASSERT_EQ(done.wait_for(std::chrono::seconds(15)), std::future_status::ready); } +// A stack-constructed operation is not owned by a shared_ptr, so shared_from_this() +// throws std::bad_weak_ptr. SendAsync() must not let that escape: it falls back to a +// synchronous run and still invokes the callback (issue #1481 review round 6). +TEST_F(HttpClientCurlTests, SendAsync_NotSharedOwned_RunsSynchronouslyNoThrow) +{ + CurlHttpOperation op( + "GET", "http://selfjoin.regression.invalid/", nullptr, m_headers, m_body, + false, 1 /*connTimeout*/, false /*sslVerify*/, ""); + + bool callbackRan = false; + // No shared owner -> the fallback runs Send()+callback synchronously on this + // thread, so SendAsync() returns only after the callback has run. Capturing + // callbackRan by reference is therefore safe. + op.SendAsync([&callbackRan](CurlHttpOperation&) { callbackRan = true; }); + + EXPECT_TRUE(callbackRan); +} + #endif // MATSDK_PAL_CPP11 && !_MSC_VER && HAVE_MAT_DEFAULT_HTTP_CLIENT From 23b6f9abe96bbb1d26ca0af5fa7f54095765d1ce Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 21:27:04 -0500 Subject: [PATCH 13/29] Correct the body-move comment in SendRequestAsync (#1481 review round 7) Comment 3548399891: the note claimed curlRequest->m_body was a "per-send copy of the EventsUploadContext body (the retry source of truth)". That's inaccurate -- the encoder MOVES ctx->body into the request (SimpleHttpRequest::SetBody does m_body = std::move(body), IHttpClient.hpp:310) and then clears ctx->body (HttpRequestEncoder.cpp:165-167), so m_body is the sole owner of the payload and ctx->body is not a retained retry buffer. Reworded to describe the actual ownership and why moving m_body is safe (the request is single-use and released with the EventsUploadContext). No code change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index 9e073e7b6..eeb30168e 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -81,11 +81,14 @@ namespace MAT_NS_BEGIN { sslCaInfo = m_sslCaInfo; } - // Move the request body into the operation (it is taken by value there): - // curlRequest->m_body is a per-send copy of the EventsUploadContext body - // (the retry source of truth), so moving it avoids duplicating large upload - // payloads while still giving the operation an owned buffer for its detached - // worker (issue #1481). + // The operation takes the request body by value, so move it in rather than + // copy. curlRequest->m_body already holds the sole copy of the encoded payload: + // the encoder moves ctx->body into it (SimpleHttpRequest::SetBody does + // m_body = std::move(body)) and clears the source (HttpRequestEncoder.cpp:165-167). + // The request is used for a single send and is then released with the + // EventsUploadContext (see the AddRequest note above), so m_body is not read + // again after this point -- moving it avoids duplicating a potentially large + // upload buffer while giving the detached worker an owned buffer (issue #1481). auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, std::move(curlRequest->m_body), false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); curlRequest->SetOperation(curlOperation); From 62395c712070d44e8c68cc4ce9e8cc20a7fdac90 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 21:49:36 -0500 Subject: [PATCH 14/29] Harden NoSelfJoin test timeout path (#1481 review round 8) Comment 3548517158: on the (practically unreachable) 15s-timeout path the detached worker could still be running when the fixture tears down -- and the fixture holds HttpClient_Curl m_client (its dtor calls curl_global_cleanup) plus the m_headers/m_body the worker may still read -- risking a secondary crash unrelated to the regression. On timeout, best-effort cancel the still-running operation and wait briefly before failing, so the worker is much less likely to outlive teardown. The cancel handle is a std::weak_ptr so it does not keep the operation alive (an owning ref would defeat the test: the callback's box->reset() must remain the last external ref). Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass (NoSelfJoin normal path still ~45ms). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/HttpClientCurlTests.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index f3d39af88..ff65722da 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -154,6 +154,11 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) "GET", "http://selfjoin.regression.invalid/", nullptr, m_headers, m_body, false, 1 /*connTimeout*/, false /*sslVerify*/, ""); + // Non-owning handle, used only to cancel the worker on the timeout path below. + // It must not keep the operation alive, or the callback's box->reset() would no + // longer drop the last external reference (the exact scenario under test). + std::weak_ptr weakOp = op; + // A shared box holds the only external reference. The callback resets the // contained shared_ptr (on the worker thread) to drop the last external // reference -- the exact #1481 trigger -- without raw new/delete. @@ -169,7 +174,18 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) callbackDone->set_value(); }); - ASSERT_EQ(done.wait_for(std::chrono::seconds(15)), std::future_status::ready); + if (done.wait_for(std::chrono::seconds(15)) != std::future_status::ready) + { + // The detached worker is unexpectedly still running (Send() against the + // non-resolving host should fail within milliseconds). Best-effort: signal + // it to abort and give it a moment to finish so it does not outlive fixture + // teardown, which destroys m_client (curl_global_cleanup) and the + // m_headers/m_body it may still be reading. Then fail. + if (auto liveOp = weakOp.lock()) + liveOp->Abort(); + done.wait_for(std::chrono::seconds(5)); + FAIL() << "SendAsync did not complete within 15s"; + } } // A stack-constructed operation is not owned by a shared_ptr, so shared_from_this() From 9ae7dd5069ad946a01b9977f1c96945d974de104 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 22:11:09 -0500 Subject: [PATCH 15/29] Move worker-lambda construction inside the try in SendAsync (#1481 review round 9) Comment 3548602231: the worker lambda was constructed before the try/catch. Copying callback (a std::function) into it can throw std::bad_alloc, which would escape SendAsync() despite the intent that any failure fall back to a synchronous run. Construct the lambda inline inside the std::thread() call within the try so a throwing capture-copy is caught alongside a thread-start failure; the catch now calls RunSendAndCallback(callback) directly (self keeps this operation alive for the synchronous run). This also drops the separate named worker variable. Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index c47b7f783..06fbbb907 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -373,20 +373,22 @@ class CurlHttpOperation : public std::enable_shared_from_this RunSendAndCallback(callback); return; } - auto worker = [self, callback]() { self->RunSendAndCallback(callback); }; try { - std::thread(worker).detach(); + // Constructing the worker lambda copies `callback` (a std::function, + // which can throw std::bad_alloc), and std::thread construction can throw + // std::system_error / std::bad_alloc -- both are inside this try. The + // worker holds `self`, keeping this operation alive for the detached run. + std::thread([self, callback]() { self->RunSendAndCallback(callback); }).detach(); } catch (const std::exception& e) { - // Starting the worker thread failed -- std::thread construction can throw - // std::system_error (e.g. resource exhaustion) or std::bad_alloc while - // allocating the callable. Run the operation synchronously as a fallback - // so the IHttpClient callback is still always invoked and the exception - // does not escape SendAsync(). + // Building the callable or starting the worker thread failed. Run the + // operation synchronously as a fallback so the IHttpClient callback is + // still always invoked and the exception does not escape SendAsync(). + // `self` keeps this operation alive for the duration of the run. TRACE("CurlHttpOperation could not start worker thread: %s; running synchronously\n", e.what()); - worker(); + RunSendAndCallback(callback); } } From 1ff4b52a111a76b12ff65af1dd1e9940f3f18e23 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 9 Jul 2026 11:01:09 -0500 Subject: [PATCH 16/29] Drop issue-number references from code comments Reword comments in the curl HTTP client and its tests to describe the behavior without citing tracking numbers; no code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 2 +- lib/http/HttpClient_Curl.hpp | 8 ++++---- tests/unittests/HttpClientCurlTests.cpp | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index eeb30168e..8e659b4f0 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -88,7 +88,7 @@ namespace MAT_NS_BEGIN { // The request is used for a single send and is then released with the // EventsUploadContext (see the AddRequest note above), so m_body is not read // again after this point -- moving it avoids duplicating a potentially large - // upload buffer while giving the detached worker an owned buffer (issue #1481). + // upload buffer while giving the detached worker an owned buffer. auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, std::move(curlRequest->m_body), false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); curlRequest->SetOperation(curlOperation); diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 06fbbb907..3bcf6cd1e 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -101,7 +101,7 @@ class CurlHttpOperation : public std::enable_shared_from_this // need not outlive this operation. requestBody is taken by value and // owned by this operation: the detached worker in SendAsync can outlive // the caller's request, so a reference into it could dangle during - // Send() (issue #1481). + // Send(). const std::map& requestHeaders, std::vector requestBody, // Default connectivity and response size options @@ -184,7 +184,7 @@ class CurlHttpOperation : public std::enable_shared_from_this // buffer and owned request body are therefore no longer in use. // There is no future to join, so destruction is safe on any thread -- // including the worker thread itself, which is where it happens when the - // callback drops the last other reference (issue #1481). + // callback drops the last other reference. DispatchEvent(OnDestroy); res = CURLE_OK; curl_easy_cleanup(curl); @@ -352,7 +352,7 @@ class CurlHttpOperation : public std::enable_shared_from_this // when the callback below caused this operation to be destroyed on the // async thread (OnHttpResponse -> EventsUploadContext::clear()), that join // was a self-join and raised std::system_error("Resource deadlock avoided") - // out of the noexcept destructor, aborting the process (issue #1481). With + // out of the noexcept destructor, aborting the process. With // the self-keepalive there is no future and no join: the worker simply // exits, releasing the last reference, and ~CurlHttpOperation runs // trivially on whichever thread drops it. @@ -511,7 +511,7 @@ class CurlHttpOperation : public std::enable_shared_from_this std::string m_sslCaInfo; // Owned copy of the request body, read by Send(). Owned (not a reference into // the caller's IHttpRequest) because the detached worker in SendAsync can - // outlive that request, so a reference could dangle mid-send (issue #1481). + // outlive that request, so a reference could dangle mid-send. std::vector requestBody; struct curl_slist *m_headersChunk = nullptr; diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index ff65722da..f0cceb725 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -130,7 +130,7 @@ TEST_F(HttpClientCurlTests, SetSslVerification_ConcurrentCallsNoRace) SUCCEED(); } -// --- Regression: issue #1481 (EDEADLK self-join in ~CurlHttpOperation) --- +// --- Regression: EDEADLK self-join in ~CurlHttpOperation --- // When the async callback drops the last *external* reference to the operation, // ~CurlHttpOperation runs on the worker thread. The old std::async design joined @@ -161,7 +161,7 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) // A shared box holds the only external reference. The callback resets the // contained shared_ptr (on the worker thread) to drop the last external - // reference -- the exact #1481 trigger -- without raw new/delete. + // reference -- the exact trigger -- without raw new/delete. auto box = std::make_shared>(std::move(op)); (*box)->SendAsync([box, callbackDone](CurlHttpOperation&) { @@ -190,7 +190,7 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) // A stack-constructed operation is not owned by a shared_ptr, so shared_from_this() // throws std::bad_weak_ptr. SendAsync() must not let that escape: it falls back to a -// synchronous run and still invokes the callback (issue #1481 review round 6). +// synchronous run and still invokes the callback. TEST_F(HttpClientCurlTests, SendAsync_NotSharedOwned_RunsSynchronouslyNoThrow) { CurlHttpOperation op( From b1e03d8a97fcfd28ab7a42b934d2bba18058985f Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 9 Jul 2026 11:39:13 -0500 Subject: [PATCH 17/29] Guarantee the callback fires even when Send() throws Copilot review: the fallback comments state the callback is 'always invoked', but RunSendAndCallback skipped the callback if Send() itself threw (the callback call was inside the same try). If Send() threw, the request was left outstanding and its IHttpClient callback never completed, which could hang the upload/cancel path. Restructured so Send() is guarded on its own, a thrown Send() sets a failure result (res = CURLE_FAILED_INIT), and the callback is then invoked unconditionally (itself guarded so a throwing callback can't escape the detached worker). The 'always invoked' contract now holds literally. Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 3bcf6cd1e..d71bb9462 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -323,25 +323,43 @@ class CurlHttpOperation : public std::enable_shared_from_this return res; } - // Runs the blocking Send() and then the callback, swallowing any exception. - // A detached worker must not let an exception escape (that would call - // std::terminate), and std::async previously captured exceptions in its - // never-observed future; this preserves that. Shared by the detached worker - // and the synchronous fallbacks in SendAsync(). + // Runs the blocking Send() and then the callback, guaranteeing the callback is + // invoked exactly once and that no exception escapes (a detached worker must not + // let one escape -> std::terminate; std::async previously captured exceptions in + // its never-observed future). Shared by the detached worker and the synchronous + // fallbacks in SendAsync(). void RunSendAndCallback(const std::function& callback) { try { Send(); - if (callback != nullptr) - callback(*this); } catch (const std::exception& e) { - TRACE("CurlHttpOperation worker terminated by exception: %s\n", e.what()); + TRACE("CurlHttpOperation Send() failed by exception: %s\n", e.what()); + res = CURLE_FAILED_INIT; // report a failure result to the callback } catch (...) { - TRACE("CurlHttpOperation worker terminated by unknown exception\n"); + TRACE("CurlHttpOperation Send() failed by unknown exception\n"); + res = CURLE_FAILED_INIT; + } + // Invoke the callback even if Send() threw, so the operation is always + // completed (with the failure result set above) and the request is never + // left outstanding. Guard it so a throwing callback cannot escape either. + if (callback != nullptr) + { + try + { + callback(*this); + } + catch (const std::exception& e) + { + TRACE("CurlHttpOperation callback threw: %s\n", e.what()); + } + catch (...) + { + TRACE("CurlHttpOperation callback threw unknown exception\n"); + } } } From cf9bc95d4226e0375b1d94c88d738f985f862ce3 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 10:49:03 -0500 Subject: [PATCH 18/29] Fix use-after-free dispatching OnDestroy after the completion callback The self-keepalive fix keeps the operation alive on the detached worker until Send() and the completion callback finish, so ~CurlHttpOperation can now run after the completion callback. In synchronous-handler builds (USE_SYNC_HTTPRESPONSE_HANDLER, which is defined by default) that callback runs HttpClientManager::onHttpResponse, which deletes the IHttpResponseCallback before returning. The destructor then dispatched OnDestroy through the now-dangling m_callback -- a use-after-free on every completed request (benign until the freed memory is reused; caught by ASAN). Track completion in an atomic flag set right after the completion callback runs, and skip the destructor's OnDestroy dispatch once completed. OnDestroy still fires when the operation is destroyed before completing (aborted, or a construction/dispatch failure), where m_callback is still valid. Add a regression test (SendAsync_NoOnDestroyDispatchAfterCompletion) that keeps the callback alive and asserts OnDestroy is not dispatched after completion; it fails without the guard and passes with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 20 +++++++++- tests/unittests/HttpClientCurlTests.cpp | 53 +++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index d71bb9462..c2470bf0e 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -85,6 +85,11 @@ class CurlHttpOperation : public std::enable_shared_from_this std::atomic isAborted { false }; // Set to 'true' when async callback is aborted + // Set once the completion callback has run. After that point the externally + // owned IHttpResponseCallback (m_callback) may already be destroyed, so it must + // not be dispatched to again (see ~CurlHttpOperation). + std::atomic m_completed { false }; + /** * Create local CURL instance for url and body * @@ -185,7 +190,16 @@ class CurlHttpOperation : public std::enable_shared_from_this // There is no future to join, so destruction is safe on any thread -- // including the worker thread itself, which is where it happens when the // callback drops the last other reference. - DispatchEvent(OnDestroy); + // Only notify OnDestroy when the operation is destroyed before its + // completion callback ran (e.g. aborted, or a construction/dispatch + // failure). Once the callback has run, m_callback may already be freed -- + // synchronous-handler builds run onHttpResponse, which deletes the + // IHttpResponseCallback, inside the completion callback -- so dispatching + // through it here would be a use-after-free. + if (!m_completed.load(std::memory_order_acquire)) + { + DispatchEvent(OnDestroy); + } res = CURLE_OK; curl_easy_cleanup(curl); curl_slist_free_all(m_headersChunk); @@ -360,6 +374,10 @@ class CurlHttpOperation : public std::enable_shared_from_this { TRACE("CurlHttpOperation callback threw unknown exception\n"); } + // The completion callback may have destroyed the IHttpResponseCallback + // (synchronous-handler builds run onHttpResponse, which deletes it), so + // m_callback must not be dispatched to after this point. + m_completed.store(true, std::memory_order_release); } } diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index f0cceb725..0ac7f8e38 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -15,6 +15,8 @@ #include #include #include +#include +#include using namespace testing; using namespace MAT; @@ -206,4 +208,55 @@ TEST_F(HttpClientCurlTests, SendAsync_NotSharedOwned_RunsSynchronouslyNoThrow) EXPECT_TRUE(callbackRan); } +// Regression test for the completion-path use-after-free: in synchronous-handler +// builds the IHttpResponseCallback is deleted inside the completion callback +// (HttpClientManager::onHttpResponse), while the operation is kept alive slightly +// longer by the detached worker's self-reference. The destructor must therefore +// NOT dispatch OnDestroy through m_callback once the completion callback has run, +// or it would touch a freed callback. Here the callback is kept alive so the +// dispatch is observable: it must not happen after completion. +TEST_F(HttpClientCurlTests, SendAsync_NoOnDestroyDispatchAfterCompletion) +{ + struct TrackingCallback : public IHttpResponseCallback + { + std::atomic completed { false }; + std::atomic onDestroyAfterComplete { 0 }; + void OnHttpResponse(IHttpResponse* response) override { delete response; } + void OnHttpStateEvent(HttpStateEvent state, void*, size_t) override + { + if (state == OnDestroy && completed.load()) + onDestroyAfterComplete++; + } + }; + TrackingCallback cb; + + auto callbackDone = std::make_shared>(); + auto done = callbackDone->get_future(); + + auto op = std::make_shared( + "GET", "http://selfjoin.regression.invalid/", &cb, m_headers, m_body, + false, 1 /*connTimeout*/, false /*sslVerify*/, ""); + std::weak_ptr weakOp = op; + auto box = std::make_shared>(std::move(op)); + + (*box)->SendAsync([box, callbackDone, &cb](CurlHttpOperation&) { + // Mark completion, then drop the last external reference on the worker + // thread -- mirroring onHttpResponse deleting the callback and releasing + // the request while the worker still holds its self-reference. + cb.completed.store(true); + box->reset(); + callbackDone->set_value(); + }); + + ASSERT_EQ(done.wait_for(std::chrono::seconds(15)), std::future_status::ready); + + // The operation is destroyed once the worker returns and releases its + // self-reference; wait for that so the destructor has run. + for (int i = 0; i < 500 && !weakOp.expired(); ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + ASSERT_TRUE(weakOp.expired()); + + EXPECT_EQ(cb.onDestroyAfterComplete.load(), 0); +} + #endif // MATSDK_PAL_CPP11 && !_MSC_VER && HAVE_MAT_DEFAULT_HTTP_CLIENT From f2af5ec82afeaed169c9ed6c4785fda9d33a9061 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 11:30:14 -0500 Subject: [PATCH 19/29] Address review: include , correct OnDestroy comment, harden test Follow-ups from code review of the completion-path UAF fix: - HttpClient_Curl.hpp uses std::move but relied on a transitive ; include it directly. - The destructor comment claimed OnDestroy still fires on abort. It does not: every SendAsync path (including abort and the synchronous fallbacks) runs the completion callback and sets m_completed first, so OnDestroy is suppressed for any request that was actually sent. Correct the comment to say so. - Harden SendAsync_NoOnDestroyDispatchAfterCompletion: on the wait_for timeout path, abort the worker and wait so it can't outlive the stack frame whose cb/m_headers/m_body it reads; and let the destructor body finish before asserting so a missing guard is observed rather than raced past. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 16 ++++++++++------ tests/unittests/HttpClientCurlTests.cpp | 18 ++++++++++++++++-- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index c2470bf0e..a3022bd2e 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -190,12 +191,15 @@ class CurlHttpOperation : public std::enable_shared_from_this // There is no future to join, so destruction is safe on any thread -- // including the worker thread itself, which is where it happens when the // callback drops the last other reference. - // Only notify OnDestroy when the operation is destroyed before its - // completion callback ran (e.g. aborted, or a construction/dispatch - // failure). Once the callback has run, m_callback may already be freed -- - // synchronous-handler builds run onHttpResponse, which deletes the - // IHttpResponseCallback, inside the completion callback -- so dispatching - // through it here would be a use-after-free. + // OnDestroy is dispatched only when this operation is destroyed before its + // send completed -- i.e. it was never sent, or construction failed. Every + // SendAsync path (normal, abort, and the synchronous fallbacks) runs the + // completion callback and sets m_completed first, and once that callback has + // run m_callback may already be freed: synchronous-handler builds delete the + // IHttpResponseCallback inside onHttpResponse, called from the completion + // callback. Dispatching through it then would be a use-after-free, so it is + // suppressed. (Consequently the curl client does not emit OnDestroy for a + // request that was actually sent.) if (!m_completed.load(std::memory_order_acquire)) { DispatchEvent(OnDestroy); diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 0ac7f8e38..e5c61c400 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -248,13 +248,27 @@ TEST_F(HttpClientCurlTests, SendAsync_NoOnDestroyDispatchAfterCompletion) callbackDone->set_value(); }); - ASSERT_EQ(done.wait_for(std::chrono::seconds(15)), std::future_status::ready); + if (done.wait_for(std::chrono::seconds(15)) != std::future_status::ready) + { + // The detached worker is unexpectedly still running (Send() against the + // non-resolving host should fail within milliseconds). Abort it and wait so + // it does not outlive this stack frame, which owns cb / m_headers / m_body + // that the worker may still read. Then fail. + if (auto liveOp = weakOp.lock()) + liveOp->Abort(); + for (int i = 0; i < 500 && !weakOp.expired(); ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + FAIL() << "SendAsync did not complete within 15s"; + } // The operation is destroyed once the worker returns and releases its - // self-reference; wait for that so the destructor has run. + // self-reference; wait for that, then let the destructor body finish so a + // missing guard (which would increment the counter inside ~CurlHttpOperation) + // is observed rather than raced past. for (int i = 0; i < 500 && !weakOp.expired(); ++i) std::this_thread::sleep_for(std::chrono::milliseconds(10)); ASSERT_TRUE(weakOp.expired()); + std::this_thread::sleep_for(std::chrono::milliseconds(50)); EXPECT_EQ(cb.onDestroyAfterComplete.load(), 0); } From 4af84014b6eb3a777d0c0df4c1e0bc65ee15d3fa Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 14:23:31 -0500 Subject: [PATCH 20/29] Fix two curl-client lifetime issues found in review - SendRequestAsync moved the request body out of the request, but the request is read again after the send: HttpResponseDecoder emits the request payload on EVT_HTTP_OK / EVT_HTTP_ERROR when requestDone runs the decode chain. Moving it out left those debug events with an empty payload (a curl-only regression vs the WinInet and NSURLSession clients). Copy the body into the operation instead -- it still gets an owned buffer for the detached send, and the request keeps its body for the decoder. - ~HttpClient_Curl ran curl_global_cleanup, but detached workers run curl_easy_cleanup in ~CurlHttpOperation after the request callback has already been removed from HttpClientManager's tracking, so the shutdown drain could return before an operation's easy-handle cleanup finished -- curl_global_cleanup then races easy-handle cleanup (undefined behavior). Track in-flight operations and have ~HttpClient_Curl wait (bounded to 5s) for them before global cleanup. All 14 curl unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 34 ++++++++++++++++++++-------- lib/http/HttpClient_Curl.hpp | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index 8e659b4f0..3ba34ae6c 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -54,6 +54,19 @@ namespace MAT_NS_BEGIN { HttpClient_Curl::~HttpClient_Curl() { + // Detached worker threads run curl_easy_cleanup in ~CurlHttpOperation after + // the request callback has already been removed from HttpClientManager's + // tracking, so waiting only on that tracking is not enough. Wait (bounded) + // for all in-flight operations to finish their easy-handle cleanup before + // curl_global_cleanup, which must not run concurrently with it. + { + std::unique_lock lock(m_activeOps->mtx); + if (!m_activeOps->cv.wait_for(lock, std::chrono::seconds(5), + [this] { return m_activeOps->inFlight == 0; })) + { + TRACE("~HttpClient_Curl: %d operation(s) still in flight after 5s\n", m_activeOps->inFlight); + } + } curl_global_cleanup(); TRACE("Destroyed HttpClient_Curl.\n"); }; @@ -81,15 +94,18 @@ namespace MAT_NS_BEGIN { sslCaInfo = m_sslCaInfo; } - // The operation takes the request body by value, so move it in rather than - // copy. curlRequest->m_body already holds the sole copy of the encoded payload: - // the encoder moves ctx->body into it (SimpleHttpRequest::SetBody does - // m_body = std::move(body)) and clears the source (HttpRequestEncoder.cpp:165-167). - // The request is used for a single send and is then released with the - // EventsUploadContext (see the AddRequest note above), so m_body is not read - // again after this point -- moving it avoids duplicating a potentially large - // upload buffer while giving the detached worker an owned buffer. - auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, std::move(curlRequest->m_body), false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); + // Copy the request body into the operation instead of moving it out. The + // detached send needs an owned buffer (the request can be released -- e.g. by + // cancellation -- while the worker is still sending), but the request's + // m_body is also read again after the send: HttpResponseDecoder emits the + // request payload on EVT_HTTP_OK / EVT_HTTP_ERROR when requestDone runs the + // decode chain, before the request is released. Moving it out would leave + // those debug events with an empty payload -- a curl-only regression versus + // the WinInet and NSURLSession clients, which leave the request intact. + auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, curlRequest->m_body, false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); + // Count this operation before the async send starts so ~HttpClient_Curl waits + // for its curl_easy_cleanup to complete before curl_global_cleanup. + curlOperation->trackWith(m_activeOps); curlRequest->SetOperation(curlOperation); // The async Send() runs on a detached worker that holds its own shared_ptr diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index a3022bd2e..21f792475 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -26,6 +26,9 @@ #include #include #include +#include +#include +#include #include #include @@ -48,6 +51,15 @@ namespace MAT_NS_BEGIN { +// Tracks the number of in-flight CurlHttpOperations so ~HttpClient_Curl can wait for +// their detached-worker curl_easy_cleanup to finish before it runs +// curl_global_cleanup (the two must not run concurrently). +struct CurlOperationTracker { + std::mutex mtx; + std::condition_variable cv; + int inFlight = 0; +}; + /** * Curl-based HTTP client */ @@ -71,6 +83,10 @@ class HttpClient_Curl : public IHttpClient { std::map m_requests; std::atomic m_sslVerify { true }; std::string m_sslCaInfo; + + // Tracks in-flight CurlHttpOperations so the destructor can wait for their + // curl_easy_cleanup to complete before curl_global_cleanup. + std::shared_ptr m_activeOps { std::make_shared() }; }; class CurlHttpOperation : public std::enable_shared_from_this { @@ -208,6 +224,30 @@ class CurlHttpOperation : public std::enable_shared_from_this curl_easy_cleanup(curl); curl_slist_free_all(m_headersChunk); ReleaseResponse(); + + // Signal HttpClient_Curl that this operation's curl_easy_cleanup is done, so + // its destructor can safely run curl_global_cleanup once all operations end. + if (m_tracker) + { + std::lock_guard lock(m_tracker->mtx); + if (--m_tracker->inFlight == 0) + { + m_tracker->cv.notify_all(); + } + } + } + + // Associate this operation with HttpClient_Curl's in-flight tracker so its + // lifetime (through the curl_easy_cleanup in the destructor above) is awaited + // before curl_global_cleanup. Called once, before the async send starts. + void trackWith(std::shared_ptr tracker) + { + m_tracker = std::move(tracker); + if (m_tracker) + { + std::lock_guard lock(m_tracker->mtx); + ++m_tracker->inFlight; + } } /** @@ -545,6 +585,9 @@ class CurlHttpOperation : public std::enable_shared_from_this IHttpResponseCallback* m_callback = nullptr; + // In-flight tracker shared with HttpClient_Curl; decremented in the destructor. + std::shared_ptr m_tracker; + // Request values std::string m_method; std::string m_url; From 89491fdd17a54b592665cee30c010f0f23fb55b8 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 15:14:08 -0500 Subject: [PATCH 21/29] Harden curl-client shutdown: complete-on-send and skip unsafe global cleanup Address review findings on the async self-join fix: - Set m_completed after Send() regardless of whether a completion callback was provided. It was only set inside the non-null-callback branch, so a SendAsync() call with the default null callback left m_completed false and ~CurlHttpOperation would still DispatchEvent(OnDestroy) for a request that had actually been sent -- the use-after-free the guard exists to prevent. - Skip curl_global_cleanup() when the bounded in-flight drain times out. curl_global_cleanup must not run concurrently with the curl_easy_cleanup that in-flight operation destructors run on detached workers; proceeding after a timeout could crash. Leaking libcurl global state once at shutdown is the safer choice in that pathological case. Files: lib/http/HttpClient_Curl.hpp, lib/http/HttpClient_Curl.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 18 ++++++++++++++---- lib/http/HttpClient_Curl.hpp | 10 ++++++---- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index 3ba34ae6c..7a11f11d2 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -59,15 +59,25 @@ namespace MAT_NS_BEGIN { // tracking, so waiting only on that tracking is not enough. Wait (bounded) // for all in-flight operations to finish their easy-handle cleanup before // curl_global_cleanup, which must not run concurrently with it. + bool drained; { std::unique_lock lock(m_activeOps->mtx); - if (!m_activeOps->cv.wait_for(lock, std::chrono::seconds(5), - [this] { return m_activeOps->inFlight == 0; })) + drained = m_activeOps->cv.wait_for(lock, std::chrono::seconds(5), + [this] { return m_activeOps->inFlight == 0; }); + if (!drained) { - TRACE("~HttpClient_Curl: %d operation(s) still in flight after 5s\n", m_activeOps->inFlight); + TRACE("~HttpClient_Curl: %d operation(s) still in flight after 5s; skipping curl_global_cleanup\n", m_activeOps->inFlight); } } - curl_global_cleanup(); + // curl_global_cleanup must not run concurrently with any other libcurl use, + // including the curl_easy_cleanup that in-flight CurlHttpOperation destructors + // run on their detached workers. If the drain timed out, skip it: leaking + // libcurl's global state once at shutdown is safer than the crash/UB of tearing + // it down while an easy handle is still live on another thread. + if (drained) + { + curl_global_cleanup(); + } TRACE("Destroyed HttpClient_Curl.\n"); }; diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 21f792475..59a86dbb0 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -418,11 +418,13 @@ class CurlHttpOperation : public std::enable_shared_from_this { TRACE("CurlHttpOperation callback threw unknown exception\n"); } - // The completion callback may have destroyed the IHttpResponseCallback - // (synchronous-handler builds run onHttpResponse, which deletes it), so - // m_callback must not be dispatched to after this point. - m_completed.store(true, std::memory_order_release); } + // The send has completed. The completion callback (if any) may have destroyed + // the IHttpResponseCallback -- synchronous-handler builds run onHttpResponse, + // which deletes it -- so m_callback must not be dispatched to after this point. + // Set completion regardless of whether a callback was provided: a request that + // was actually sent must never emit OnDestroy from the destructor. + m_completed.store(true, std::memory_order_release); } void SendAsync(std::function callback = nullptr) { From 9d69b19f71c3918fe9462efc28862d78c267e7c1 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 15:54:49 -0500 Subject: [PATCH 22/29] Harden the completion-guard regression test against the timeout path Heap-own the TrackingCallback and capture the shared_ptr by value in the completion lambda so its lifetime is tied to the detached worker. Previously the stack callback was captured by reference: in the timeout/FAIL path the worker can still be running when the test returns, so it could read the callback after destruction (a use-after-free that could crash the whole test process). The final assertion now dereferences the shared_ptr. Files: tests/unittests/HttpClientCurlTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/HttpClientCurlTests.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index e5c61c400..884b0e373 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -228,22 +228,26 @@ TEST_F(HttpClientCurlTests, SendAsync_NoOnDestroyDispatchAfterCompletion) onDestroyAfterComplete++; } }; - TrackingCallback cb; + // Heap-own the callback and tie its lifetime to the detached worker (the completion + // lambda below captures the shared_ptr by value). In the timeout/FAIL path the worker + // may still be running when this test returns, so a stack callback captured by + // reference could be read after it is destroyed -- a use-after-free. + auto cb = std::make_shared(); auto callbackDone = std::make_shared>(); auto done = callbackDone->get_future(); auto op = std::make_shared( - "GET", "http://selfjoin.regression.invalid/", &cb, m_headers, m_body, + "GET", "http://selfjoin.regression.invalid/", cb.get(), m_headers, m_body, false, 1 /*connTimeout*/, false /*sslVerify*/, ""); std::weak_ptr weakOp = op; auto box = std::make_shared>(std::move(op)); - (*box)->SendAsync([box, callbackDone, &cb](CurlHttpOperation&) { + (*box)->SendAsync([box, callbackDone, cb](CurlHttpOperation&) { // Mark completion, then drop the last external reference on the worker // thread -- mirroring onHttpResponse deleting the callback and releasing // the request while the worker still holds its self-reference. - cb.completed.store(true); + cb->completed.store(true); box->reset(); callbackDone->set_value(); }); @@ -252,8 +256,9 @@ TEST_F(HttpClientCurlTests, SendAsync_NoOnDestroyDispatchAfterCompletion) { // The detached worker is unexpectedly still running (Send() against the // non-resolving host should fail within milliseconds). Abort it and wait so - // it does not outlive this stack frame, which owns cb / m_headers / m_body - // that the worker may still read. Then fail. + // it does not outlive this stack frame, which owns the m_headers / m_body the + // worker may still read. (cb is heap-owned and captured by the worker, so it + // stays alive on its own.) Then fail. if (auto liveOp = weakOp.lock()) liveOp->Abort(); for (int i = 0; i < 500 && !weakOp.expired(); ++i) @@ -270,7 +275,7 @@ TEST_F(HttpClientCurlTests, SendAsync_NoOnDestroyDispatchAfterCompletion) ASSERT_TRUE(weakOp.expired()); std::this_thread::sleep_for(std::chrono::milliseconds(50)); - EXPECT_EQ(cb.onDestroyAfterComplete.load(), 0); + EXPECT_EQ(cb->onDestroyAfterComplete.load(), 0); } #endif // MATSDK_PAL_CPP11 && !_MSC_VER && HAVE_MAT_DEFAULT_HTTP_CLIENT From bfabf8c23ad79add6e437c3ab3a116d4d0df4697 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 16:03:18 -0500 Subject: [PATCH 23/29] Clarify ~CurlHttpOperation comment for never-sent and synchronous-fallback cases The destructor runs after the detached worker releases its reference only when Send() ran asynchronously; it also runs for operations that were never sent or when SendAsync fell back to a synchronous run. Destruction is safe on any thread in all cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 59a86dbb0..50be90bac 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -200,13 +200,15 @@ class CurlHttpOperation : public std::enable_shared_from_this */ virtual ~CurlHttpOperation() { - // The async Send() runs on a detached worker that holds a shared_ptr to - // this operation (see SendAsync), so this destructor runs only after that - // worker has finished and released its reference. The curl handle, response - // buffer and owned request body are therefore no longer in use. - // There is no future to join, so destruction is safe on any thread -- - // including the worker thread itself, which is where it happens when the - // callback drops the last other reference. + // When Send() ran asynchronously, it was on a detached worker that held a + // shared_ptr to this operation (see SendAsync), so this destructor runs only + // after that worker finished and released its reference; the curl handle, + // response buffer and owned request body are then no longer in use. It can also + // run without any async worker: for an operation that was never sent, or when + // SendAsync fell back to a synchronous run on the caller's thread. There is no + // future to join in any case, so destruction is safe on any thread -- including + // the worker thread itself, which is where it happens when the callback drops + // the last other reference. // OnDestroy is dispatched only when this operation is destroyed before its // send completed -- i.e. it was never sent, or construction failed. Every // SendAsync path (normal, abort, and the synchronous fallbacks) runs the From e25c43c4295204a14ace8760ea4f12f3086eb4a9 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 16:16:54 -0500 Subject: [PATCH 24/29] Correct the OnDestroy comment: suppressed once a send is attempted RunSendAndCallback sets m_completed regardless of the send result (including an immediate curl_easy_init failure), so OnDestroy is dispatched only when the operation is destroyed without SendAsync ever having run -- not on construction failure. Reword the comment to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.hpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 50be90bac..cfdde4e90 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -209,15 +209,15 @@ class CurlHttpOperation : public std::enable_shared_from_this // future to join in any case, so destruction is safe on any thread -- including // the worker thread itself, which is where it happens when the callback drops // the last other reference. - // OnDestroy is dispatched only when this operation is destroyed before its - // send completed -- i.e. it was never sent, or construction failed. Every - // SendAsync path (normal, abort, and the synchronous fallbacks) runs the - // completion callback and sets m_completed first, and once that callback has - // run m_callback may already be freed: synchronous-handler builds delete the - // IHttpResponseCallback inside onHttpResponse, called from the completion - // callback. Dispatching through it then would be a use-after-free, so it is - // suppressed. (Consequently the curl client does not emit OnDestroy for a - // request that was actually sent.) + // OnDestroy is dispatched only when this operation is destroyed without its send + // ever having run -- i.e. SendAsync was never called. Once RunSendAndCallback + // runs it sets m_completed regardless of the result (even when Send() fails + // immediately, e.g. curl_easy_init returns an error), and once the completion + // callback has run m_callback may already be freed: synchronous-handler builds + // delete the IHttpResponseCallback inside onHttpResponse, called from the + // completion callback. Dispatching through it then would be a use-after-free, so + // it is suppressed. (Consequently the curl client does not emit OnDestroy for a + // request whose send was attempted.) if (!m_completed.load(std::memory_order_acquire)) { DispatchEvent(OnDestroy); From 8e6575b5307876e57f99ccdf4e6085212561b0ca Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 16:36:18 -0500 Subject: [PATCH 25/29] Wait for the operation to be destroyed in the self-join test's timeout path Mirror the stronger teardown from SendAsync_NoOnDestroyDispatchAfterCompletion: on the (unexpected) timeout path, wait for weakOp to expire after Abort so the detached worker cannot outlive fixture teardown (m_client/curl_global_cleanup, m_headers, m_body) and cause secondary crashes that obscure the real failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/HttpClientCurlTests.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 884b0e373..64a791c68 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -179,13 +179,15 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) if (done.wait_for(std::chrono::seconds(15)) != std::future_status::ready) { // The detached worker is unexpectedly still running (Send() against the - // non-resolving host should fail within milliseconds). Best-effort: signal - // it to abort and give it a moment to finish so it does not outlive fixture - // teardown, which destroys m_client (curl_global_cleanup) and the + // non-resolving host should fail within milliseconds). Signal it to abort, then + // wait for the operation to actually be destroyed (weakOp expires once the + // worker releases its self-reference) so the worker does not outlive this stack + // frame / fixture teardown, which destroys m_client (curl_global_cleanup) and the // m_headers/m_body it may still be reading. Then fail. if (auto liveOp = weakOp.lock()) liveOp->Abort(); - done.wait_for(std::chrono::seconds(5)); + for (int i = 0; i < 500 && !weakOp.expired(); ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(10)); FAIL() << "SendAsync did not complete within 15s"; } } From d2c69ddc0c3d8de747e9d6c3d324d14b2f26abed Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 17:40:06 -0500 Subject: [PATCH 26/29] Wait for the operation to be destroyed on the self-join test's success path The callback sets the promise, but the detached worker still holds its self- reference until RunSendAndCallback returns. Wait (bounded) for weakOp to expire before the test returns so the operation's curl_easy_cleanup cannot race with fixture teardown, matching the other async regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/HttpClientCurlTests.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 64a791c68..214b999a9 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -190,6 +190,14 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) std::this_thread::sleep_for(std::chrono::milliseconds(10)); FAIL() << "SendAsync did not complete within 15s"; } + + // Success path: the callback set the promise, but the detached worker still holds + // its self-reference until RunSendAndCallback returns. Wait (bounded) for the + // operation to be destroyed so its curl_easy_cleanup cannot race with fixture + // teardown (m_client -> curl_global_cleanup). + for (int i = 0; i < 500 && !weakOp.expired(); ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + EXPECT_TRUE(weakOp.expired()); } // A stack-constructed operation is not owned by a shared_ptr, so shared_from_this() From 8650ce1b0f5bf3515ebecf0a26a34d98787de307 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 17:55:10 -0500 Subject: [PATCH 27/29] Guarantee async ops are destroyed before teardown in both self-join tests Extract a shared DrainOperation helper that waits for the operation to be destroyed and aborts a stuck worker as a fallback, then hard-asserts it is gone. Both async regression tests now ensure the detached worker (and its curl_easy_cleanup) cannot outlive fixture teardown (m_client -> curl_global_cleanup) on either the success or timeout path, rather than returning while the worker might still run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/HttpClientCurlTests.cpp | 75 +++++++++++-------------- 1 file changed, 34 insertions(+), 41 deletions(-) diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 214b999a9..83bd30bfd 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -30,6 +30,24 @@ class HttpClientCurlTests : public ::testing::Test const std::vector m_body; }; +// Ensure a detached async operation is fully destroyed before the test returns, so the +// worker's curl_easy_cleanup cannot race fixture teardown (m_client -> curl_global_cleanup). +// The .invalid host fails DNS in milliseconds, so this normally completes immediately; a +// stuck worker is aborted as a fallback. Returns whether the operation was destroyed. +static bool DrainOperation(const std::weak_ptr& weakOp) +{ + for (int i = 0; i < 500 && !weakOp.expired(); ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + if (!weakOp.expired()) + { + if (auto liveOp = weakOp.lock()) + liveOp->Abort(); + for (int i = 0; i < 500 && !weakOp.expired(); ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + return weakOp.expired(); +} + // --- SetSslVerification wiring --- TEST_F(HttpClientCurlTests, SslVerification_DefaultsToTrue) @@ -176,28 +194,15 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) callbackDone->set_value(); }); - if (done.wait_for(std::chrono::seconds(15)) != std::future_status::ready) - { - // The detached worker is unexpectedly still running (Send() against the - // non-resolving host should fail within milliseconds). Signal it to abort, then - // wait for the operation to actually be destroyed (weakOp expires once the - // worker releases its self-reference) so the worker does not outlive this stack - // frame / fixture teardown, which destroys m_client (curl_global_cleanup) and the - // m_headers/m_body it may still be reading. Then fail. - if (auto liveOp = weakOp.lock()) - liveOp->Abort(); - for (int i = 0; i < 500 && !weakOp.expired(); ++i) - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - FAIL() << "SendAsync did not complete within 15s"; - } + const bool completed = (done.wait_for(std::chrono::seconds(15)) == std::future_status::ready); - // Success path: the callback set the promise, but the detached worker still holds - // its self-reference until RunSendAndCallback returns. Wait (bounded) for the - // operation to be destroyed so its curl_easy_cleanup cannot race with fixture - // teardown (m_client -> curl_global_cleanup). - for (int i = 0; i < 500 && !weakOp.expired(); ++i) - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - EXPECT_TRUE(weakOp.expired()); + // Make sure the operation is destroyed before this test returns, regardless of + // whether the send completed: the callback sets the promise while the detached + // worker still holds its self-reference, so the worker (and its curl_easy_cleanup) + // can outlive this frame and race fixture teardown (m_client -> curl_global_cleanup). + // Drain (with an abort fallback) so the operation is gone first. + ASSERT_TRUE(DrainOperation(weakOp)) << "operation still alive after abort; worker may outlive teardown"; + EXPECT_TRUE(completed) << "SendAsync did not complete within 15s"; } // A stack-constructed operation is not owned by a shared_ptr, so shared_from_this() @@ -262,27 +267,15 @@ TEST_F(HttpClientCurlTests, SendAsync_NoOnDestroyDispatchAfterCompletion) callbackDone->set_value(); }); - if (done.wait_for(std::chrono::seconds(15)) != std::future_status::ready) - { - // The detached worker is unexpectedly still running (Send() against the - // non-resolving host should fail within milliseconds). Abort it and wait so - // it does not outlive this stack frame, which owns the m_headers / m_body the - // worker may still read. (cb is heap-owned and captured by the worker, so it - // stays alive on its own.) Then fail. - if (auto liveOp = weakOp.lock()) - liveOp->Abort(); - for (int i = 0; i < 500 && !weakOp.expired(); ++i) - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - FAIL() << "SendAsync did not complete within 15s"; - } + const bool completed = (done.wait_for(std::chrono::seconds(15)) == std::future_status::ready); - // The operation is destroyed once the worker returns and releases its - // self-reference; wait for that, then let the destructor body finish so a - // missing guard (which would increment the counter inside ~CurlHttpOperation) - // is observed rather than raced past. - for (int i = 0; i < 500 && !weakOp.expired(); ++i) - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - ASSERT_TRUE(weakOp.expired()); + // Ensure the operation is destroyed before this test returns so the worker cannot + // outlive fixture teardown (m_client -> curl_global_cleanup); cb is heap-owned and + // captured by the worker, so it stays alive on its own. Abort a stuck worker. + ASSERT_TRUE(DrainOperation(weakOp)) << "operation still alive after abort; worker may outlive teardown"; + ASSERT_TRUE(completed) << "SendAsync did not complete within 15s"; + // Let the destructor body finish so a missing OnDestroy guard (which would increment + // the counter inside ~CurlHttpOperation) is observed rather than raced past. std::this_thread::sleep_for(std::chrono::milliseconds(50)); EXPECT_EQ(cb->onDestroyAfterComplete.load(), 0); From bd49de40a3d2d67cbd85566eb97cf5059b1b7d55 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 18:12:11 -0500 Subject: [PATCH 28/29] Hard-stop if a detached curl worker refuses to drain before teardown These directly-constructed operations are not tracked by HttpClient_Curl::m_activeOps, so nothing else bounds the race between a lingering worker's curl_easy_cleanup and the fixture's curl_global_cleanup. DrainOperationOrDie now aborts a stuck worker and, if the operation is still alive afterward (a genuine keepalive/abort regression), records a failure and std::abort()s rather than returning into fixture teardown with an in-flight curl worker. In practice the .invalid host fails DNS in milliseconds so the operation is always gone immediately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/HttpClientCurlTests.cpp | 28 ++++++++++++++++--------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 83bd30bfd..383c5f565 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -17,6 +17,7 @@ #include #include #include +#include using namespace testing; using namespace MAT; @@ -30,11 +31,14 @@ class HttpClientCurlTests : public ::testing::Test const std::vector m_body; }; -// Ensure a detached async operation is fully destroyed before the test returns, so the -// worker's curl_easy_cleanup cannot race fixture teardown (m_client -> curl_global_cleanup). -// The .invalid host fails DNS in milliseconds, so this normally completes immediately; a -// stuck worker is aborted as a fallback. Returns whether the operation was destroyed. -static bool DrainOperation(const std::weak_ptr& weakOp) +// Wait for a detached async operation to be fully destroyed before the test returns, so +// the worker's curl_easy_cleanup cannot race fixture teardown (m_client -> +// curl_global_cleanup). These operations are not tracked by HttpClient_Curl::m_activeOps, +// so nothing else bounds that race. The .invalid host fails DNS in milliseconds, so this +// normally completes immediately; a stuck worker is aborted as a fallback. If the +// operation is STILL alive after that (a genuine keepalive/abort regression), hard-stop +// the process rather than proceed into curl_global_cleanup with an in-flight curl worker. +static void DrainOperationOrDie(const std::weak_ptr& weakOp) { for (int i = 0; i < 500 && !weakOp.expired(); ++i) std::this_thread::sleep_for(std::chrono::milliseconds(10)); @@ -45,7 +49,12 @@ static bool DrainOperation(const std::weak_ptr& weakOp) for (int i = 0; i < 500 && !weakOp.expired(); ++i) std::this_thread::sleep_for(std::chrono::milliseconds(10)); } - return weakOp.expired(); + if (!weakOp.expired()) + { + ADD_FAILURE() << "detached curl worker did not terminate after abort; hard-stopping " + "so curl_global_cleanup cannot run concurrently with an in-flight worker"; + std::abort(); + } } // --- SetSslVerification wiring --- @@ -200,8 +209,7 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) // whether the send completed: the callback sets the promise while the detached // worker still holds its self-reference, so the worker (and its curl_easy_cleanup) // can outlive this frame and race fixture teardown (m_client -> curl_global_cleanup). - // Drain (with an abort fallback) so the operation is gone first. - ASSERT_TRUE(DrainOperation(weakOp)) << "operation still alive after abort; worker may outlive teardown"; + DrainOperationOrDie(weakOp); EXPECT_TRUE(completed) << "SendAsync did not complete within 15s"; } @@ -271,8 +279,8 @@ TEST_F(HttpClientCurlTests, SendAsync_NoOnDestroyDispatchAfterCompletion) // Ensure the operation is destroyed before this test returns so the worker cannot // outlive fixture teardown (m_client -> curl_global_cleanup); cb is heap-owned and - // captured by the worker, so it stays alive on its own. Abort a stuck worker. - ASSERT_TRUE(DrainOperation(weakOp)) << "operation still alive after abort; worker may outlive teardown"; + // captured by the worker, so it stays alive on its own. + DrainOperationOrDie(weakOp); ASSERT_TRUE(completed) << "SendAsync did not complete within 15s"; // Let the destructor body finish so a missing OnDestroy guard (which would increment // the counter inside ~CurlHttpOperation) is observed rather than raced past. From 42d448ca867086128f1ad4ce949e27aa0e86a756 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sun, 12 Jul 2026 19:48:38 -0500 Subject: [PATCH 29/29] Fix curl worker shutdown lifetime Move curl request tracking into shared state captured by detached workers so late callbacks do not dereference HttpClient_Curl after the shutdown drain times out. Preserve the bounded drain before curl_global_cleanup and abandon late callbacks/logging when shutdown cannot safely wait. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Curl.cpp | 70 +++++++++++++++++++++++------------- lib/http/HttpClient_Curl.hpp | 33 ++++++++++++----- 2 files changed, 70 insertions(+), 33 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index 7a11f11d2..e69b7b31e 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -54,6 +54,9 @@ namespace MAT_NS_BEGIN { HttpClient_Curl::~HttpClient_Curl() { + auto state = m_state; + auto activeOps = state->activeOps; + // Detached worker threads run curl_easy_cleanup in ~CurlHttpOperation after // the request callback has already been removed from HttpClientManager's // tracking, so waiting only on that tracking is not enough. Wait (bounded) @@ -61,14 +64,25 @@ namespace MAT_NS_BEGIN { // curl_global_cleanup, which must not run concurrently with it. bool drained; { - std::unique_lock lock(m_activeOps->mtx); - drained = m_activeOps->cv.wait_for(lock, std::chrono::seconds(5), - [this] { return m_activeOps->inFlight == 0; }); + std::unique_lock lock(activeOps->mtx); + drained = activeOps->cv.wait_for(lock, std::chrono::seconds(5), + [activeOps] { return activeOps->inFlight == 0; }); if (!drained) { - TRACE("~HttpClient_Curl: %d operation(s) still in flight after 5s; skipping curl_global_cleanup\n", m_activeOps->inFlight); + TRACE("~HttpClient_Curl: %d operation(s) still in flight after 5s; skipping curl_global_cleanup\n", activeOps->inFlight); } } + if (!drained) + { + activeOps->abandonCallbacks.store(true, std::memory_order_release); + std::lock_guard lock(state->requestsMtx); + // Detached workers capture this shared state, not HttpClient_Curl. If the + // bounded drain times out, the client object is about to be destroyed; do + // not retain raw request pointers or dispatch late response/logging + // callbacks that may refer to shutdown-owned state. The worker will erase + // no-op and drop the response instead of dereferencing the destroyed client. + state->requests.clear(); + } // curl_global_cleanup must not run concurrently with any other libcurl use, // including the curl_easy_cleanup that in-flight CurlHttpOperation destructors // run on their detached workers. If the drain timed out, skip it: leaking @@ -88,6 +102,8 @@ namespace MAT_NS_BEGIN { void HttpClient_Curl::SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) { + auto state = m_state; + // Note: 'request' is never owned by IHttpClient and gets deleted in EventsUploadContext.clear() AddRequest(request); auto curlRequest = static_cast(request); @@ -100,8 +116,8 @@ namespace MAT_NS_BEGIN { std::string sslCaInfo; { - std::lock_guard lock(m_requestsMtx); - sslCaInfo = m_sslCaInfo; + std::lock_guard lock(state->requestsMtx); + sslCaInfo = state->sslCaInfo; } // Copy the request body into the operation instead of moving it out. The @@ -115,7 +131,7 @@ namespace MAT_NS_BEGIN { auto curlOperation = std::make_shared(curlRequest->m_method, curlRequest->m_url, callback, requestHeaders, curlRequest->m_body, false, HTTP_CONN_TIMEOUT, m_sslVerify, sslCaInfo); // Count this operation before the async send starts so ~HttpClient_Curl waits // for its curl_easy_cleanup to complete before curl_global_cleanup. - curlOperation->trackWith(m_activeOps); + curlOperation->trackWith(state->activeOps); curlRequest->SetOperation(curlOperation); // The async Send() runs on a detached worker that holds its own shared_ptr @@ -126,8 +142,17 @@ namespace MAT_NS_BEGIN { // that request being destroyed on the worker thread (OnHttpResponse -> // EventsUploadContext::clear()), the operation is simply destroyed there // once the worker returns; there is no future to join. - curlOperation->SendAsync([this, callback, requestId](CurlHttpOperation& operation) { - this->EraseRequest(requestId); + curlOperation->SendAsync([state, callback, requestId](CurlHttpOperation& operation) { + const bool abandonCallback = state->activeOps->abandonCallbacks.load(std::memory_order_acquire); + { + std::lock_guard lock(state->requestsMtx); + state->requests.erase(requestId); + } + if (abandonCallback) + { + TRACE("HttpClient_Curl shutdown abandoned response callback for %s\n", requestId.c_str()); + return; + } auto response = std::unique_ptr(new SimpleHttpResponse(requestId)); response->m_result = HttpResult_OK; @@ -157,14 +182,16 @@ namespace MAT_NS_BEGIN { void HttpClient_Curl::CancelRequestAsync(std::string const& id) { + auto state = m_state; CurlHttpRequest* request = nullptr; { // Hold the lock only while iterating over the list of requests - std::lock_guard lock(m_requestsMtx); - if (m_requests.find(id) != m_requests.cend()) { - request = static_cast(m_requests[id]); + std::lock_guard lock(state->requestsMtx); + auto requestIt = state->requests.find(id); + if (requestIt != state->requests.cend()) { + request = static_cast(requestIt->second); LOG_TRACE("HTTP request=%p id=%s being aborted...", request, id.c_str()); - m_requests.erase(id); + state->requests.erase(requestIt); } } @@ -183,23 +210,18 @@ namespace MAT_NS_BEGIN { void HttpClient_Curl::SetSslVerification(bool sslVerify, const std::string& caInfo) { m_sslVerify = sslVerify; - std::lock_guard lock(m_requestsMtx); - m_sslCaInfo = caInfo; - } - - void HttpClient_Curl::EraseRequest(std::string const& id) - { - std::lock_guard lock(m_requestsMtx); - m_requests.erase(id); + auto state = m_state; + std::lock_guard lock(state->requestsMtx); + state->sslCaInfo = caInfo; } void HttpClient_Curl::AddRequest(IHttpRequest* request) { - std::lock_guard lock(m_requestsMtx); - m_requests[request->GetId()] = request; + auto state = m_state; + std::lock_guard lock(state->requestsMtx); + state->requests[request->GetId()] = request; } } MAT_NS_END #endif - diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index cfdde4e90..beada6ed2 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -53,11 +54,28 @@ namespace MAT_NS_BEGIN { // Tracks the number of in-flight CurlHttpOperations so ~HttpClient_Curl can wait for // their detached-worker curl_easy_cleanup to finish before it runs -// curl_global_cleanup (the two must not run concurrently). +// curl_global_cleanup (the two must not run concurrently). If shutdown times +// out, abandonCallbacks tells late workers to skip callback/log dispatch. struct CurlOperationTracker { std::mutex mtx; std::condition_variable cv; int inFlight = 0; + std::atomic abandonCallbacks { false }; +}; + +// State shared with detached curl worker callbacks. A worker can outlive +// HttpClient_Curl if shutdown's bounded drain times out, so callbacks must only +// touch this shared state and never capture/dereference the parent client. +struct CurlClientSharedState { + CurlClientSharedState() : + activeOps(std::make_shared()) + { + } + + std::mutex requestsMtx; + std::map requests; + std::string sslCaInfo; + std::shared_ptr activeOps; }; /** @@ -76,17 +94,10 @@ class HttpClient_Curl : public IHttpClient { void SetSslVerification(bool sslVerify, const std::string& caInfo = ""); private: - void EraseRequest(std::string const& id); void AddRequest(IHttpRequest* request); - std::mutex m_requestsMtx; - std::map m_requests; + std::shared_ptr m_state { std::make_shared() }; std::atomic m_sslVerify { true }; - std::string m_sslCaInfo; - - // Tracks in-flight CurlHttpOperations so the destructor can wait for their - // curl_easy_cleanup to complete before curl_global_cleanup. - std::shared_ptr m_activeOps { std::make_shared() }; }; class CurlHttpOperation : public std::enable_shared_from_this { @@ -94,6 +105,10 @@ class CurlHttpOperation : public std::enable_shared_from_this void DispatchEvent(HttpStateEvent type) { + if (m_tracker && m_tracker->abandonCallbacks.load(std::memory_order_acquire)) + { + return; + } if (m_callback != nullptr) { m_callback->OnHttpStateEvent(type, static_cast(curl), 0);