From 7b43dd4504e1a3764cda2cca23ab77fa0f50b9b9 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 10 Jun 2026 13:44:38 -0500 Subject: [PATCH 01/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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/50] 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); From 6b793adc0652fedceb7a91b9d7f6783105a5d485 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 30 Jul 2026 00:26:07 -0500 Subject: [PATCH 30/50] Simplify curl worker lifetime handling Replace the detached self-keepalive and shutdown-tracker design with an owned worker thread. Normal destruction joins the worker; callback-thread destruction detaches it to avoid EDEADLK, while completion is published before callbacks can release the operation. Files: - lib/http/HttpClient_Curl.hpp: own, publish, join, and self-detach the worker safely - lib/http/HttpClient_Curl.cpp: restore the direct client lifetime model - tests/unittests/HttpClientCurlTests.cpp: cover self-destruction and late OnDestroy suppression Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 --- lib/http/HttpClient_Curl.cpp | 104 ++-------- lib/http/HttpClient_Curl.hpp | 245 +++++++----------------- tests/unittests/HttpClientCurlTests.cpp | 143 ++------------ 3 files changed, 100 insertions(+), 392 deletions(-) diff --git a/lib/http/HttpClient_Curl.cpp b/lib/http/HttpClient_Curl.cpp index e69b7b31e..4633b2fc3 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -54,44 +54,7 @@ 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) - // 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(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", 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 - // 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(); - } + curl_global_cleanup(); TRACE("Destroyed HttpClient_Curl.\n"); }; @@ -102,8 +65,6 @@ 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); @@ -116,44 +77,15 @@ namespace MAT_NS_BEGIN { std::string sslCaInfo; { - std::lock_guard lock(state->requestsMtx); - sslCaInfo = state->sslCaInfo; + std::lock_guard lock(m_requestsMtx); + sslCaInfo = m_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(state->activeOps); curlRequest->SetOperation(curlOperation); - // 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 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([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; - } - + curlOperation->SendAsync([this, callback, requestId](CurlHttpOperation& operation) { + EraseRequest(requestId); auto response = std::unique_ptr(new SimpleHttpResponse(requestId)); response->m_result = HttpResult_OK; @@ -182,16 +114,14 @@ 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(state->requestsMtx); - auto requestIt = state->requests.find(id); - if (requestIt != state->requests.cend()) { - request = static_cast(requestIt->second); + std::lock_guard lock(m_requestsMtx); + if (m_requests.find(id) != m_requests.cend()) { + request = static_cast(m_requests[id]); LOG_TRACE("HTTP request=%p id=%s being aborted...", request, id.c_str()); - state->requests.erase(requestIt); + m_requests.erase(id); } } @@ -210,16 +140,20 @@ namespace MAT_NS_BEGIN { void HttpClient_Curl::SetSslVerification(bool sslVerify, const std::string& caInfo) { m_sslVerify = sslVerify; - auto state = m_state; - std::lock_guard lock(state->requestsMtx); - state->sslCaInfo = caInfo; + 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); } void HttpClient_Curl::AddRequest(IHttpRequest* request) { - auto state = m_state; - std::lock_guard lock(state->requestsMtx); - state->requests[request->GetId()] = request; + std::lock_guard lock(m_requestsMtx); + m_requests[request->GetId()] = request; } } MAT_NS_END diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index beada6ed2..076bec4a3 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -24,12 +24,8 @@ #include #include #include -#include -#include -#include #include -#include -#include +#include #include #include @@ -52,32 +48,6 @@ 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). 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; -}; - /** * Curl-based HTTP client */ @@ -94,21 +64,20 @@ 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::shared_ptr m_state { std::make_shared() }; + std::mutex m_requestsMtx; + std::map m_requests; std::atomic m_sslVerify { true }; + std::string m_sslCaInfo; }; -class CurlHttpOperation : public std::enable_shared_from_this { +class CurlHttpOperation { public: 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); @@ -117,11 +86,6 @@ 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 * @@ -135,12 +99,11 @@ class CurlHttpOperation : public std::enable_shared_from_this std::string url, IHttpResponseCallback* callback, // 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(). + // need not outlive this operation. requestBody is stored by reference; + // CurlHttpRequest destroys this operation (which joins the worker) before + // destroying its inherited request-body storage. const std::map& requestHeaders, - std::vector requestBody, + const std::vector& requestBody, // Default connectivity and response size options bool rawResponse = false, size_t httpConnTimeout = HTTP_CONN_TIMEOUT, @@ -158,7 +121,7 @@ class CurlHttpOperation : public std::enable_shared_from_this m_sslCaInfo(sslCaInfo), // Local vars - requestBody(std::move(requestBody)) + requestBody(requestBody) { TRACE("--------------------------------------------------------------------------------------------------\n"); response.memory = nullptr; @@ -215,24 +178,23 @@ class CurlHttpOperation : public std::enable_shared_from_this */ virtual ~CurlHttpOperation() { - // 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 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_worker.joinable()) + { + if (m_worker.get_id() == std::this_thread::get_id()) + { + // The completion callback can release the owning request on this + // worker. Detach rather than joining the current thread; Send() has + // finished and the worker does not touch this operation afterward. + m_worker.detach(); + } + else + { + m_worker.join(); + } + } + + // The completion callback may destroy m_callback. SendAsync marks completion + // before invoking it, so do not dispatch through that pointer afterward. if (!m_completed.load(std::memory_order_acquire)) { DispatchEvent(OnDestroy); @@ -241,30 +203,6 @@ 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; - } } /** @@ -398,97 +336,46 @@ class CurlHttpOperation : public std::enable_shared_from_this return res; } - // 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(); - } - catch (const std::exception& e) - { - TRACE("CurlHttpOperation Send() failed by exception: %s\n", e.what()); - res = CURLE_FAILED_INIT; // report a failure result to the callback - } - catch (...) + void SendAsync(std::function callback = nullptr) { + // A newly created std::thread may run before it is assigned to m_worker. + // Hold this gate until the assignment completes so a fast failure cannot + // destroy the operation from its callback while SendAsync still uses it. + std::lock_guard startGuard(m_workerStartMtx); + if (m_worker.joinable()) { - TRACE("CurlHttpOperation Send() failed by unknown exception\n"); - res = CURLE_FAILED_INIT; + throw std::logic_error("CurlHttpOperation is single-use"); } - // 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 + m_completed.store(false, std::memory_order_release); + m_worker = std::thread([this, callback]() { { - callback(*this); + std::lock_guard startGuard(m_workerStartMtx); } - catch (const std::exception& e) + try { - TRACE("CurlHttpOperation callback threw: %s\n", e.what()); + Send(); } catch (...) { - TRACE("CurlHttpOperation callback threw unknown exception\n"); + // std::async stored worker exceptions in its unobserved future. + // A raw thread must contain them to avoid std::terminate. + res = CURLE_FAILED_INIT; } - } - // 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) { - // 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. 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. - 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; - } - try - { - // 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) - { - // 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()); - RunSendAndCallback(callback); - } + // The callback can release the last owner and run this destructor on + // the worker, so this is the worker's final access to operation state. + m_completed.store(true, std::memory_order_release); + try + { + if (callback != nullptr) + { + callback(*this); + } + } + catch (...) + { + // Match the old unobserved-future behavior at the thread boundary. + } + }); } /** @@ -604,17 +491,13 @@ 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; 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. - std::vector requestBody; + // The owning CurlHttpRequest destroys this operation before its inherited + // request-body storage, and cross-thread destruction joins the worker. + const std::vector& requestBody; struct curl_slist *m_headersChunk = nullptr; // Processed response headers and body @@ -630,6 +513,12 @@ class CurlHttpOperation : public std::enable_shared_from_this size_t sendlen = 0; // # bytes sent by client size_t acklen = 0; // # bytes ack by server + std::mutex m_workerStartMtx; + std::thread m_worker; + // Set before the completion callback, which may destroy m_callback and this + // operation. The destructor uses it to suppress a late OnDestroy dispatch. + std::atomic m_completed { false }; + /** * Helper routine to wait for data on socket * diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 383c5f565..9236791fa 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -16,7 +16,6 @@ #include #include #include -#include #include using namespace testing; @@ -31,32 +30,6 @@ class HttpClientCurlTests : public ::testing::Test const std::vector m_body; }; -// 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)); - 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)); - } - 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 --- TEST_F(HttpClientCurlTests, SslVerification_DefaultsToTrue) @@ -161,84 +134,7 @@ TEST_F(HttpClientCurlTests, SetSslVerification_ConcurrentCallsNoRace) // --- 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 -// 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) -{ - // 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 -- - // 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*/, ""); - - // 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 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 - // 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(); - }); - - const bool completed = (done.wait_for(std::chrono::seconds(15)) == std::future_status::ready); - - // 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). - DrainOperationOrDie(weakOp); - EXPECT_TRUE(completed) << "SendAsync did not complete within 15s"; -} - -// 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. -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); -} - -// 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 { @@ -248,45 +144,34 @@ TEST_F(HttpClientCurlTests, SendAsync_NoOnDestroyDispatchAfterCompletion) void OnHttpStateEvent(HttpStateEvent state, void*, size_t) override { if (state == OnDestroy && completed.load()) - onDestroyAfterComplete++; + { + ++onDestroyAfterComplete; + } } }; - // 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 callback = std::make_shared(); auto callbackDone = std::make_shared>(); auto done = callbackDone->get_future(); auto op = std::make_shared( - "GET", "http://selfjoin.regression.invalid/", cb.get(), m_headers, m_body, + "GET", "://malformed", callback.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&) { - // 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)->SendAsync([box, callback, callbackDone](CurlHttpOperation&) { + callback->completed.store(true); box->reset(); callbackDone->set_value(); }); - const bool completed = (done.wait_for(std::chrono::seconds(15)) == std::future_status::ready); - - // 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. - 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. - std::this_thread::sleep_for(std::chrono::milliseconds(50)); - - EXPECT_EQ(cb->onDestroyAfterComplete.load(), 0); + if (done.wait_for(std::chrono::seconds(5)) != std::future_status::ready) + { + ADD_FAILURE() << "curl worker did not finish before fixture teardown"; + std::abort(); + } + EXPECT_EQ(callback->onDestroyAfterComplete.load(), 0); } #endif // MATSDK_PAL_CPP11 && !_MSC_VER && HAVE_MAT_DEFAULT_HTTP_CLIENT From 88f5c8fc4e4fb5afaade9b9805610b3d028948b6 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 30 Jul 2026 02:41:54 -0500 Subject: [PATCH 31/50] Preserve curl completion semantics on worker failures Keep OnDestroy delivery exactly once while the response callback is still valid, and complete requests synchronously when callable copying or thread creation fails. Track send attempts independently of thread joinability so failed construction cannot make an operation reusable. Files: - lib/http/HttpClient_Curl.hpp: centralize terminal event/callback delivery and harden thread startup - tests/unittests/HttpClientCurlTests.cpp: verify self-destruction, terminal event delivery, construction failure, and single-use behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 --- lib/http/HttpClient_Curl.hpp | 100 +++++++++++++++--------- tests/unittests/HttpClientCurlTests.cpp | 36 +++++++-- 2 files changed, 94 insertions(+), 42 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 076bec4a3..83e1de320 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -193,12 +193,7 @@ class CurlHttpOperation { } } - // The completion callback may destroy m_callback. SendAsync marks completion - // before invoking it, so do not dispatch through that pointer afterward. - if (!m_completed.load(std::memory_order_acquire)) - { - DispatchEvent(OnDestroy); - } + DispatchDestroyEvent(); res = CURLE_OK; curl_easy_cleanup(curl); curl_slist_free_all(m_headersChunk); @@ -340,42 +335,42 @@ class CurlHttpOperation { // A newly created std::thread may run before it is assigned to m_worker. // Hold this gate until the assignment completes so a fast failure cannot // destroy the operation from its callback while SendAsync still uses it. - std::lock_guard startGuard(m_workerStartMtx); - if (m_worker.joinable()) { - throw std::logic_error("CurlHttpOperation is single-use"); - } - m_completed.store(false, std::memory_order_release); - m_worker = std::thread([this, callback]() { - { - std::lock_guard startGuard(m_workerStartMtx); - } - try - { - Send(); - } - catch (...) + std::lock_guard startGuard(m_workerStartMtx); + if (m_sendAttempted) { - // std::async stored worker exceptions in its unobserved future. - // A raw thread must contain them to avoid std::terminate. - res = CURLE_FAILED_INIT; + throw std::logic_error("CurlHttpOperation is single-use"); } + m_sendAttempted = true; - // The callback can release the last owner and run this destructor on - // the worker, so this is the worker's final access to operation state. - m_completed.store(true, std::memory_order_release); try { - if (callback != nullptr) - { - callback(*this); - } + m_worker = std::thread([this, callback]() { + { + std::lock_guard startGuard(m_workerStartMtx); + } + try + { + Send(); + } + catch (...) + { + // std::async stored worker exceptions in its unobserved + // future. A raw thread must contain them. + res = CURLE_FAILED_INIT; + } + Complete(callback); + }); + return; } catch (...) { - // Match the old unobserved-future behavior at the thread boundary. + // Callable allocation/copy or std::thread creation failed. } - }); + } + + res = CURLE_FAILED_INIT; + Complete(callback); } /** @@ -514,10 +509,45 @@ class CurlHttpOperation { size_t acklen = 0; // # bytes ack by server std::mutex m_workerStartMtx; + bool m_sendAttempted = false; std::thread m_worker; - // Set before the completion callback, which may destroy m_callback and this - // operation. The destructor uses it to suppress a late OnDestroy dispatch. - std::atomic m_completed { false }; + std::atomic m_destroyEventDispatched { false }; + + void DispatchDestroyEvent() noexcept + { + bool expected = false; + if (m_destroyEventDispatched.compare_exchange_strong( + expected, true, std::memory_order_acq_rel)) + { + try + { + DispatchEvent(OnDestroy); + } + catch (...) + { + // State observers must not terminate the worker or destructor. + } + } + } + + void Complete(const std::function& callback) noexcept + { + // Preserve the documented state event while m_callback is still valid. + // The completion callback can release the last owner, so this must remain + // the worker's final access to the operation. + DispatchDestroyEvent(); + try + { + if (callback != nullptr) + { + callback(*this); + } + } + catch (...) + { + // Match the old unobserved-future behavior at the thread boundary. + } + } /** * Helper routine to wait for data on socket diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 9236791fa..abe13e71d 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -17,6 +17,8 @@ #include #include #include +#include +#include using namespace testing; using namespace MAT; @@ -138,14 +140,13 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) { struct TrackingCallback : public IHttpResponseCallback { - std::atomic completed { false }; - std::atomic onDestroyAfterComplete { 0 }; + std::atomic destroyEvents { 0 }; void OnHttpResponse(IHttpResponse* response) override { delete response; } void OnHttpStateEvent(HttpStateEvent state, void*, size_t) override { - if (state == OnDestroy && completed.load()) + if (state == OnDestroy) { - ++onDestroyAfterComplete; + ++destroyEvents; } } }; @@ -159,9 +160,7 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) false, 1 /*connTimeout*/, false /*sslVerify*/, ""); auto box = std::make_shared>(std::move(op)); - (*box)->SendAsync([box, callback, callbackDone](CurlHttpOperation&) { - callback->completed.store(true); box->reset(); callbackDone->set_value(); }); @@ -171,7 +170,30 @@ TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) ADD_FAILURE() << "curl worker did not finish before fixture teardown"; std::abort(); } - EXPECT_EQ(callback->onDestroyAfterComplete.load(), 0); + EXPECT_EQ(callback->destroyEvents.load(), 1); +} + +TEST_F(HttpClientCurlTests, SendAsync_CallbackCopyFailureStillCompletes) +{ + struct ThrowOnCopy + { + explicit ThrowOnCopy(bool& invoked) : invoked(&invoked) {} + ThrowOnCopy(ThrowOnCopy&&) = default; + ThrowOnCopy(const ThrowOnCopy&) { throw std::logic_error("copy failed"); } + void operator()(CurlHttpOperation&) const { *invoked = true; } + bool* invoked; + }; + + CurlHttpOperation op( + "GET", "://malformed", nullptr, m_headers, m_body, + false, 1 /*connTimeout*/, false /*sslVerify*/, ""); + bool callbackInvoked = false; + std::function callback { ThrowOnCopy(callbackInvoked) }; + + EXPECT_NO_THROW(op.SendAsync(std::move(callback))); + EXPECT_TRUE(callbackInvoked); + EXPECT_EQ(op.GetResponseCode(), CURLE_FAILED_INIT); + EXPECT_THROW(op.SendAsync(), std::logic_error); } #endif // MATSDK_PAL_CPP11 && !_MSC_VER && HAVE_MAT_DEFAULT_HTTP_CLIENT From 6a1c0500284edee4f37f450c334e9dc94cee0c64 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 1 Aug 2026 02:45:03 -0500 Subject: [PATCH 32/50] Default Win32 desktop transport to WinHTTP instead of WinInet WinInet is designed for interactive desktop apps: it depends on a logged-on user and that user's Internet Explorer settings, and Microsoft documents it as unsupported for services and other non-interactive processes. WinHTTP is Microsoft's own recommended replacement for exactly that scenario, and 1DS's dominant embedding scenario (background/service telemetry) is the one WinInet is not designed for. Add lib/http/HttpClient_WinHttp.hpp/.cpp implementing the same IHttpClient/IHttpRequest contract as HttpClient_WinInet using WinHTTP's async API instead. Key differences from a direct port of the WinInet implementation: - WinHttpOpen uses WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY (falling back to WINHTTP_ACCESS_TYPE_NO_PROXY on an older OS that rejects it) instead of WinInet's INTERNET_OPEN_TYPE_PRECONFIG, so proxy resolution does not require a logged-on user. - WinHTTP's async model has one distinct callback status per stage (SENDREQUEST_COMPLETE -> HEADERS_AVAILABLE -> DATA_AVAILABLE/READ_COMPLETE loop -> REQUEST_ERROR) rather than WinInet's single INTERNET_STATUS_REQUEST_COMPLETE, and a FALSE return from an async-handle call is always a genuine synchronous failure (never ERROR_IO_PENDING as with WinInet). - The response-size cap (MAX_HTTP_RESPONSE_SIZE, see #1508) is enforced the same way, before every read. - The MS-root certificate check rebuilds the chain via CertGetCertificateChain, since WinHttpQueryOption only hands back the leaf certificate rather than WinInet's ready-made chain context. - Request lifetime uses std::enable_shared_from_this / shared_ptr rather than raw-pointer self-ownership: WinHttpCloseHandle on a request with a pending operation blocks the calling thread until that operation's completion callback (which runs on a different WinHTTP-internal thread) finishes running. Holding the shared requests-map mutex across that call -- WinInet's pattern, safe there because its callback runs synchronously on the calling thread -- deadlocks here, since the callback thread needs that same mutex to erase() the completed request. shared_ptr lets cancellation release the map lock before the blocking close, while still safely keeping the wrapper alive against a concurrent natural completion. - CancelAllRequests() waits on a condition variable signaled from erase() instead of polling in a sleep loop. HttpClientFactory now selects WinHTTP by default on Win32 desktop (non-WinRT) builds. Set MATSDK_USE_WININET=ON (CMake) or define HAVE_MAT_WININET_HTTP_CLIENT (legacy MSBuild) to opt back into WinInet, e.g. for IE-integrated proxy/cookie behavior. Both cpp files are always compiled; the choice is made at the factory's #include/#ifdef site, matching the existing pattern for WinRt vs. WinInet. Wired into both build systems: lib/CMakeLists.txt (new source files, winhttp link library, MATSDK_USE_WININET option) and lib/pal/desktop/desktop.vcxitems (new source files; linking uses #pragma comment(lib, "winhttp.lib") in the new .cpp so no individual .vcxproj's AdditionalDependencies needs updating). Validation (Windows x64 Debug, both CMake and the Solutions\MSTelemetrySDK.sln MSBuild path actually used by CI): - UnitTests: 496/496 passed. - FuncTests: 43/43 passed, excluding sendManyRequestsAndCancel, which hits the real production collector over the internet. That specific test hangs identically with the original, unmodified WinInet client under the same back-to-back test sequence, confirming it is pre-existing network/infrastructure flakiness unrelated to this change, not a regression. - Found and fixed two real bugs during validation: (1) WinHttpSetStatusCallback's return value was checked as a boolean, when it actually returns the previous callback function pointer (typically null on first registration) -- this rejected every request immediately after registering the callback; (2) the deadlock described above, reproduced live via a hung sendManyRequestsAndCancel run and confirmed fixed by comparing CPU-active vs. CPU-static process state before and after the shared_ptr change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 --- lib/CMakeLists.txt | 13 +- lib/http/HttpClientFactory.cpp | 9 + lib/http/HttpClientFactory.hpp | 13 +- lib/http/HttpClient_WinHttp.cpp | 666 +++++++++++++++++++++++++++++++ lib/http/HttpClient_WinHttp.hpp | 67 ++++ lib/pal/desktop/desktop.vcxitems | 2 + 6 files changed, 767 insertions(+), 3 deletions(-) create mode 100644 lib/http/HttpClient_WinHttp.cpp create mode 100644 lib/http/HttpClient_WinHttp.hpp diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index 13b4d46d4..fc0475c43 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -276,9 +276,20 @@ if(NOT MATSDK_USE_VCPKG_DEPS) endif() add_definitions(-D_UNICODE -DUNICODE -DWIN32 -DMATSDK_PLATFORM_WINDOWS=1 -D_UTC_SDK -DUSE_BOND -D_WINDOWS -D_USRDLL -DWINVER=_WIN32_WINNT_WIN7) remove_definitions(-D_MBCS) + # WinHTTP is the default Win32 desktop HTTP transport (see + # HttpClientFactory.hpp): unlike WinInet it does not require a logged-on + # interactive user, so it works in services and other non-interactive + # processes. Set MATSDK_USE_WININET=ON to opt back into WinInet, e.g. for + # IE-integrated proxy/cookie behavior. + option(MATSDK_USE_WININET "Use WinInet instead of WinHTTP as the Win32 desktop HTTP client" OFF) + if(MATSDK_USE_WININET) + add_definitions(-DHAVE_MAT_WININET_HTTP_CLIENT) + endif() list(APPEND SRCS http/HttpClient_WinInet.cpp http/HttpClient_WinInet.hpp + http/HttpClient_WinHttp.cpp + http/HttpClient_WinHttp.hpp pal/desktop/WindowsDesktopDeviceInformationImpl.cpp pal/desktop/WindowsDesktopNetworkInformationImpl.cpp pal/desktop/WindowsDesktopSystemInformationImpl.cpp @@ -606,7 +617,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux" OR CMAKE_SYSTEM_NAME STREQUAL "Android") target_link_libraries(mat PUBLIC log) endif() elseif(PAL_IMPLEMENTATION STREQUAL "WIN32") - target_link_libraries(mat PUBLIC wininet crypt32 ws2_32) + target_link_libraries(mat PUBLIC wininet winhttp crypt32 ws2_32) elseif(APPLE) target_link_libraries(mat PUBLIC "-framework CoreFoundation" diff --git a/lib/http/HttpClientFactory.cpp b/lib/http/HttpClientFactory.cpp index 5419f161d..b58175e1a 100644 --- a/lib/http/HttpClientFactory.cpp +++ b/lib/http/HttpClientFactory.cpp @@ -18,6 +18,8 @@ #include "http/HttpClient_WinRt.hpp" #elif defined(HAVE_MAT_WININET_HTTP_CLIENT) #include "http/HttpClient_WinInet.hpp" + #elif defined(HAVE_MAT_WINHTTP_HTTP_CLIENT) + #include "http/HttpClient_WinHttp.hpp" #endif #elif defined(MATSDK_PAL_CPP11) #if TARGET_OS_IPHONE || (defined(__APPLE__) && defined(APPLE_HTTP)) @@ -49,6 +51,13 @@ namespace MAT_NS_BEGIN { return std::make_shared(); } +#elif defined(HAVE_MAT_WINHTTP_HTTP_CLIENT) + /* Win32 WinHTTP client (default) */ + std::shared_ptr HttpClientFactory::Create() { + LOG_TRACE("Creating HttpClient_WinHttp"); + return std::make_shared(); + } + #endif #elif defined(HAVE_MAT_CURL_HTTP_CLIENT) std::shared_ptr HttpClientFactory::Create() { diff --git a/lib/http/HttpClientFactory.hpp b/lib/http/HttpClientFactory.hpp index c96bc2ab0..08cbe2cc0 100644 --- a/lib/http/HttpClientFactory.hpp +++ b/lib/http/HttpClientFactory.hpp @@ -25,8 +25,17 @@ class HttpClientFactory // TODO: [maxgolov] - remove this once there is a better way to pass HTTP client configuration #if defined(MATSDK_PAL_WIN32) && !defined(_WINRT_DLL) -#define HAVE_MAT_WININET_HTTP_CLIENT -#include "http/HttpClient_WinInet.hpp" + #if defined(HAVE_MAT_WININET_HTTP_CLIENT) + #include "http/HttpClient_WinInet.hpp" + #else + // WinHTTP is the default Win32 desktop transport: unlike WinInet, it does + // not depend on a logged-on interactive user or that user's Internet + // Explorer settings, so it works in services and other non-interactive + // processes without extra configuration. Define HAVE_MAT_WININET_HTTP_CLIENT + // to opt back into WinInet (e.g. for IE-integrated proxy/cookie behavior). + #define HAVE_MAT_WINHTTP_HTTP_CLIENT + #include "http/HttpClient_WinHttp.hpp" + #endif #endif #endif // HAVE_MAT_DEFAULT_HTTP_CLIENT diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp new file mode 100644 index 000000000..3aa3b2212 --- /dev/null +++ b/lib/http/HttpClient_WinHttp.cpp @@ -0,0 +1,666 @@ +// clang-format off +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#include "mat/config.h" + +#ifdef HAVE_MAT_DEFAULT_HTTP_CLIENT +#include "HttpClient_WinHttp.hpp" +#include "utils/StringConversion.hpp" +#include "utils/StringUtils.hpp" + +#include +#include + +#include +#include +#include +#include + +#pragma comment(lib, "winhttp.lib") + +namespace MAT_NS_BEGIN { + +class WinHttpRequestWrapper : public std::enable_shared_from_this +{ + protected: + HttpClient_WinHttp& m_parent; + std::string m_id; + IHttpResponseCallback* m_appCallback {nullptr}; + HINTERNET m_hConnect {nullptr}; + HINTERNET m_hRequest {nullptr}; + SimpleHttpRequest* m_request; + std::vector m_bodyBuffer; + std::vector m_readBuffer; + bool isCallbackCalled {false}; + bool isAborted {false}; + + public: + WinHttpRequestWrapper(HttpClient_WinHttp& parent, SimpleHttpRequest* request) + : m_parent(parent), + m_id(request->GetId()), + m_request(request) + { + LOG_TRACE("%p WinHttpRequestWrapper()", this); + } + + WinHttpRequestWrapper(WinHttpRequestWrapper const&) = delete; + WinHttpRequestWrapper& operator=(WinHttpRequestWrapper const&) = delete; + + ~WinHttpRequestWrapper() noexcept + { + LOG_TRACE("%p ~WinHttpRequestWrapper()", this); + if (m_hRequest != nullptr) + { + ::WinHttpCloseHandle(m_hRequest); + } + if (m_hConnect != nullptr) + { + ::WinHttpCloseHandle(m_hConnect); + } + } + + /// + /// Asynchronously cancel pending request. + /// + /// Unlike WinInet's InternetCloseHandle, WinHttpCloseHandle on a request + /// with a pending async operation blocks the calling thread until that + /// operation's completion callback has finished running -- and that + /// callback runs on a *different* WinHTTP-internal thread. Holding + /// m_parent.m_requestsMutex across the call (WinInet's pattern, safe there + /// because its callback runs synchronously on the calling thread) would + /// deadlock here: this thread would block inside WinHttpCloseHandle holding + /// the lock, while the completion callback blocks on the same thread's + /// erase() needing that same lock. So the handle is captured and closed + /// without holding the lock. This wrapper is only reachable through a + /// shared_ptr (see HttpClient_WinHttp::m_requests / CancelRequestAsync), so + /// releasing the lock here cannot race with the object being freed -- + /// the caller already holds its own shared_ptr keeping *this* alive. + /// + void cancel() + { + HINTERNET hRequestToClose = nullptr; + { + std::lock_guard lock(m_parent.m_requestsMutex); + isAborted = true; + hRequestToClose = m_hRequest; + } + if (hRequestToClose != nullptr) + { + ::WinHttpCloseHandle(hRequestToClose); + // async request callback destroys the object + } + } + + /// + /// Verify that the server end-point certificate is MS-Rooted. + /// Unlike WinInet's INTERNET_OPTION_SERVER_CERT_CHAIN_CONTEXT (which hands + /// back a ready-made chain), WinHttpQueryOption only returns the leaf server + /// certificate context, so the chain must be built explicitly before running + /// the same CERT_CHAIN_POLICY_MICROSOFT_ROOT policy check WinInet performs. + /// + bool isMsRootCert() + { + PCCERT_CONTEXT pCertContext = nullptr; + DWORD dwSize = sizeof(pCertContext); + if (!::WinHttpQueryOption(m_hRequest, WINHTTP_OPTION_SERVER_CERT_CONTEXT, &pCertContext, &dwSize)) + { + // Downlevel/unsupported: proceed without cert validation. This behavior + // is identical to WinInet's fallback when its cert-chain option is + // unavailable, to avoid regressions for downlevel OS. + LOG_TRACE("WinHttpQueryOption(SERVER_CERT_CONTEXT) failed to obtain cert"); + return true; + } + + bool result = true; + PCCERT_CHAIN_CONTEXT pChainCtx = nullptr; + CERT_CHAIN_PARA chainPara = { sizeof(chainPara) }; + if (::CertGetCertificateChain(NULL, pCertContext, NULL, pCertContext->hCertStore, &chainPara, 0, NULL, &pChainCtx)) + { + CERT_CHAIN_POLICY_STATUS pps = { 0, 0, 0, 0, nullptr }; + pps.cbSize = sizeof(pps); + // Verify that the cert chain roots up to the Microsoft application root at top level + CERT_CHAIN_POLICY_PARA policyPara = { 0, 0, nullptr }; + policyPara.cbSize = sizeof(policyPara); + policyPara.dwFlags = MICROSOFT_ROOT_CERT_CHAIN_POLICY_CHECK_APPLICATION_ROOT_FLAG; + policyPara.pvExtraPolicyPara = nullptr; + + BOOL policyChecked = ::CertVerifyCertificateChainPolicy(CERT_CHAIN_POLICY_MICROSOFT_ROOT, pChainCtx, &policyPara, &pps); + if (!policyChecked) + { + LOG_WARN("CertVerifyCertificateChainPolicy() failed: unable to verify"); + result = false; + } + else if (pps.dwError != ERROR_SUCCESS) + { + LOG_WARN("CertVerifyCertificateChainPolicy() failed: invalid root CA - %d", pps.dwError); + result = false; + } + ::CertFreeCertificateChain(pChainCtx); + } + else + { + // Unable to build the chain -- proceed without cert validation, same + // fallback philosophy as the "downlevel OS" case above. + LOG_TRACE("CertGetCertificateChain() failed to build cert chain"); + } + ::CertFreeCertificateContext(pCertContext); + return result; + } + + void DispatchEvent(HttpStateEvent type) + { + if (m_appCallback != nullptr) + { + m_appCallback->OnHttpStateEvent(type, static_cast(m_hRequest), 0); + } + } + + // Asynchronously send HTTP request and invoke response callback. + // Ownership semantics: send(...) method self-destroys *this* upon + // reaching the terminal WinHTTP callback. There must be absolutely no + // methods that attempt to use the object after triggering send on it. + // Send operation on request may be issued no more than once. + // + // Held under m_parent.m_requestsMutex (a recursive_mutex, matching + // HttpClient_WinInet's model) for the whole method, exactly like cancel(): + // that serializes send() and cancel() completely, so cancel() can never + // interleave mid-way through handle creation and be silently lost, and a + // synchronous/reentrant completion on this same thread can safely re-enter + // the lock rather than deadlock. + void send(IHttpResponseCallback* callback) + { + std::lock_guard lock(m_parent.m_requestsMutex); + m_appCallback = callback; + m_parent.m_requests[m_id] = shared_from_this(); + + if (isAborted) + { + // Request force-aborted before creating a WinHTTP handle. + DispatchEvent(OnConnectFailed); + onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); + return; + } + + DispatchEvent(OnConnecting); + + std::wstring wUrl = to_utf16_string(m_request->m_url); + URL_COMPONENTS urlc; + memset(&urlc, 0, sizeof(urlc)); + urlc.dwStructSize = sizeof(urlc); + wchar_t hostname[256] = { 0 }; + urlc.lpszHostName = hostname; + urlc.dwHostNameLength = ARRAYSIZE(hostname); + wchar_t path[1024] = { 0 }; + urlc.lpszUrlPath = path; + urlc.dwUrlPathLength = ARRAYSIZE(path); + if (!::WinHttpCrackUrl(wUrl.c_str(), static_cast(wUrl.size()), 0, &urlc)) + { + DWORD dwError = ::GetLastError(); + LOG_WARN("WinHttpCrackUrl() failed: dwError=%d url=%s", dwError, m_request->m_url.c_str()); + // Invalid URL passed to WinHTTP API + DispatchEvent(OnConnectFailed); + onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); + return; + } + + // TODO: connect handle for the same target should be cached across + // requests to enable keep-alive (same pre-existing opportunity noted + // in HttpClient_WinInet.cpp; out of scope for this transport swap). + m_hConnect = ::WinHttpConnect(m_parent.m_hSession, hostname, urlc.nPort, 0); + if (m_hConnect == nullptr) + { + DWORD dwError = ::GetLastError(); + LOG_WARN("WinHttpConnect() failed: %d", dwError); + // Cannot connect to host + DispatchEvent(OnConnectFailed); + onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); + return; + } + + std::wstring wMethod = to_utf16_string(m_request->m_method); + bool isHttps = (urlc.nScheme == INTERNET_SCHEME_HTTPS); + m_hRequest = ::WinHttpOpenRequest( + m_hConnect, wMethod.c_str(), path, NULL, WINHTTP_NO_REFERER, + WINHTTP_DEFAULT_ACCEPT_TYPES, + WINHTTP_FLAG_REFRESH | (isHttps ? WINHTTP_FLAG_SECURE : 0)); + if (m_hRequest == nullptr) + { + DWORD dwError = ::GetLastError(); + LOG_WARN("WinHttpOpenRequest() failed: %d", dwError); + // Request cannot be opened to given URL because of some connectivity issue + DispatchEvent(OnConnectFailed); + onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); + return; + } + + // Unlike WinInet, WinHTTP has no automatic cookie jar to suppress (it + // never manages cookies on the caller's behalf) and never shows UI, so + // neither INTERNET_FLAG_NO_COOKIES nor INTERNET_FLAG_NO_UI has a WinHTTP + // equivalent to set here. + + /* Perform optional MS Root certificate check for certain end-point URLs */ + if (m_parent.IsMsRootCheckRequired()) + { + if (!isMsRootCert()) + { + // Request cannot be completed: end-point certificate is not MS-Rooted + DispatchEvent(OnConnectFailed); + onRequestComplete(ERROR_WINHTTP_SECURE_INVALID_CERT); + return; + } + } + + // WinHttpSetStatusCallback returns the PREVIOUS callback function + // pointer (typically NULL here, since this is the first registration + // on a freshly opened request handle) -- not a BOOL -- and signals + // failure only via the distinct WINHTTP_INVALID_STATUS_CALLBACK + // sentinel. Treating a null "previous callback" as failure would + // reject every request immediately after this call. + if (::WinHttpSetStatusCallback(m_hRequest, &WinHttpRequestWrapper::winHttpCallback, + WINHTTP_CALLBACK_FLAG_ALL_COMPLETIONS, 0) == WINHTTP_INVALID_STATUS_CALLBACK) + { + DWORD dwError = ::GetLastError(); + LOG_WARN("WinHttpSetStatusCallback() failed: %d", dwError); + DispatchEvent(OnConnectFailed); + onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); + return; + } + + std::ostringstream os; + for (auto const& header : m_request->m_headers) { + os << header.first << ": " << header.second << "\r\n"; + } + std::wstring wHeaders = to_utf16_string(os.str()); + + if (!wHeaders.empty() && + !::WinHttpAddRequestHeaders(m_hRequest, wHeaders.c_str(), static_cast(wHeaders.size()), + WINHTTP_ADDREQ_FLAG_ADD | WINHTTP_ADDREQ_FLAG_REPLACE)) + { + DWORD dwError = ::GetLastError(); + LOG_WARN("WinHttpAddRequestHeaders() failed: %d", dwError); + // Unable to add request headers. There's no point in proceeding with upload because + // our server is expecting those custom request headers to always be there. + DispatchEvent(OnConnectFailed); + onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); + return; + } + + // Try to send headers and request body to server + DispatchEvent(OnSending); + void* data = m_request->m_body.empty() ? nullptr : static_cast(m_request->m_body.data()); + DWORD size = static_cast(m_request->m_body.size()); + DWORD_PTR context = reinterpret_cast(this); + BOOL bResult = ::WinHttpSendRequest( + m_hRequest, WINHTTP_NO_ADDITIONAL_HEADERS, 0, data, size, size, context); + if (!bResult) + { + DWORD dwError = ::GetLastError(); + LOG_WARN("WinHttpSendRequest() failed: %d", dwError); + // Unable to send request + DispatchEvent(OnSendFailed); + onRequestComplete(dwError); + return; + } + // Async request has been queued; completion arrives via winHttpCallback. + } + + // Drives the WinHTTP async state machine: SendRequest -> ReceiveResponse -> + // (QueryDataAvailable -> ReadData)* -> onRequestComplete. Unlike WinInet + // (whose async completions all report through the single + // INTERNET_STATUS_REQUEST_COMPLETE code, and whose synchronous API calls + // signal a pending async op via a FALSE return + GetLastError()== + // ERROR_IO_PENDING), WinHTTP has one distinct callback status per stage, + // and a FALSE return from any of these calls on an async handle is always a + // genuine synchronous failure -- never "pending" -- so every failure path + // here reports immediately instead of waiting for a further callback. + static void CALLBACK winHttpCallback(HINTERNET hInternet, DWORD_PTR dwContext, DWORD dwInternetStatus, LPVOID lpvStatusInformation, DWORD dwStatusInformationLength) + { + UNREFERENCED_PARAMETER(hInternet); + + WinHttpRequestWrapper* self = reinterpret_cast(dwContext); + if (self == nullptr) + { + return; + } + + LOG_TRACE("winHttpCallback: hInternet %p, self %p, dwInternetStatus %u", hInternet, self, dwInternetStatus); + + switch (dwInternetStatus) + { + case WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING: + // HANDLE_CLOSING should always come after the terminal completion + // (REQUEST_ERROR or the zero-byte DATA_AVAILABLE). When (and if) it + // (ever) happens, self may point to an object that has already been + // destroyed. We do not perform any actions on it. + return; + + case WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE: + if (!::WinHttpReceiveResponse(self->m_hRequest, NULL)) + { + self->onRequestComplete(::GetLastError()); + } + return; + + case WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE: + if (!::WinHttpQueryDataAvailable(self->m_hRequest, NULL)) + { + self->onRequestComplete(::GetLastError()); + } + return; + + case WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE: + { + DWORD bytesAvailable = (lpvStatusInformation != nullptr) + ? *static_cast(lpvStatusInformation) : 0; + if (bytesAvailable == 0) + { + // No more data: response is complete. + self->onRequestComplete(ERROR_SUCCESS); + return; + } + // SECURITY: refuse an over-large response instead of buffering it + // (see MAX_HTTP_RESPONSE_SIZE) so a hostile/MITM'd collector cannot + // exhaust process memory. Checked before every read so the buffer + // never exceeds the cap; reported as an invalid server response -> + // NetworkFailure (retried). + if (self->m_bodyBuffer.size() + bytesAvailable > MAX_HTTP_RESPONSE_SIZE) + { + LOG_WARN("HTTP response exceeds max buffered size (%zu bytes); aborting", MAX_HTTP_RESPONSE_SIZE); + self->onRequestComplete(ERROR_WINHTTP_INVALID_SERVER_RESPONSE); + return; + } + self->m_readBuffer.resize(bytesAvailable); + if (!::WinHttpReadData(self->m_hRequest, self->m_readBuffer.data(), bytesAvailable, NULL)) + { + self->onRequestComplete(::GetLastError()); + } + return; + } + + case WINHTTP_CALLBACK_STATUS_READ_COMPLETE: + // dwStatusInformationLength is the number of bytes actually placed + // into the buffer passed to WinHttpReadData (may be less than the + // bytesAvailable that was requested). + self->m_bodyBuffer.insert(self->m_bodyBuffer.end(), + self->m_readBuffer.begin(), self->m_readBuffer.begin() + dwStatusInformationLength); + if (!::WinHttpQueryDataAvailable(self->m_hRequest, NULL)) + { + self->onRequestComplete(::GetLastError()); + } + return; + + case WINHTTP_CALLBACK_STATUS_REQUEST_ERROR: + { + WINHTTP_ASYNC_RESULT* result = static_cast(lpvStatusInformation); + DWORD dwError = (result != nullptr) ? result->dwError : ERROR_WINHTTP_INTERNAL_ERROR; + self->onRequestComplete(dwError); + return; + } + + default: + return; + } + } + + void onRequestComplete(DWORD dwError) + { + std::unique_ptr response(new SimpleHttpResponse(m_id)); + + if (dwError == ERROR_SUCCESS) { + response->m_body = m_bodyBuffer; + response->m_result = HttpResult_OK; + + DWORD statusCode = 0; + DWORD dwSize = sizeof(statusCode); + if (!::WinHttpQueryHeaders(m_hRequest, WINHTTP_QUERY_STATUS_CODE | WINHTTP_QUERY_FLAG_NUMBER, + WINHTTP_HEADER_NAME_BY_INDEX, &statusCode, &dwSize, WINHTTP_NO_HEADER_INDEX)) + { + LOG_WARN("WinHttpQueryHeaders(STATUS_CODE) failed: %d", ::GetLastError()); + } + response->m_statusCode = statusCode; + + // Raw headers, as "Name: Value\r\n..." pairs -- the same shape WinInet + // hands back via HTTP_QUERY_RAW_HEADERS_CRLF. + DWORD headerBytes = 0; + ::WinHttpQueryHeaders(m_hRequest, WINHTTP_QUERY_RAW_HEADERS_CRLF, + WINHTTP_HEADER_NAME_BY_INDEX, WINHTTP_NO_OUTPUT_BUFFER, &headerBytes, WINHTTP_NO_HEADER_INDEX); + DWORD headerErr = ::GetLastError(); + if (headerBytes > 0 && headerErr == ERROR_INSUFFICIENT_BUFFER) + { + std::wstring wHeaders(headerBytes / sizeof(wchar_t), L'\0'); + if (::WinHttpQueryHeaders(m_hRequest, WINHTTP_QUERY_RAW_HEADERS_CRLF, + WINHTTP_HEADER_NAME_BY_INDEX, &wHeaders[0], &headerBytes, WINHTTP_NO_HEADER_INDEX)) + { + // WinHttpQueryHeaders includes the buffer's trailing NUL(s) in + // the byte count; trim at the first one before converting. + size_t nul = wHeaders.find(L'\0'); + if (nul != std::wstring::npos) + { + wHeaders.resize(nul); + } + parseHeaders(to_utf8_string(wHeaders), *response); + } + else + { + LOG_WARN("WinHttpQueryHeaders(RAW_HEADERS_CRLF) failed twice: %d", ::GetLastError()); + } + } + // This event handler covers the only positive case when we actually got some server response. + // We may still invoke OnHttpResponse(...) below for this positive as well as other negative + // cases where there was a short-read, connection failure or timeout on reading the response. + DispatchEvent(OnResponse); + + } else { + switch (dwError) { + case ERROR_WINHTTP_OPERATION_CANCELLED: + response->m_result = HttpResult_Aborted; + break; + + case ERROR_WINHTTP_TIMEOUT: + case ERROR_WINHTTP_NAME_NOT_RESOLVED: + case ERROR_WINHTTP_CANNOT_CONNECT: + case ERROR_WINHTTP_CONNECTION_ERROR: + case ERROR_WINHTTP_RESEND_REQUEST: + case ERROR_WINHTTP_SECURE_CERT_DATE_INVALID: + case ERROR_WINHTTP_SECURE_CERT_CN_INVALID: + case ERROR_WINHTTP_CLIENT_AUTH_CERT_NEEDED: + case ERROR_WINHTTP_SECURE_INVALID_CA: + case ERROR_WINHTTP_SECURE_CERT_REV_FAILED: + case ERROR_WINHTTP_SECURE_CHANNEL_ERROR: + case ERROR_WINHTTP_SECURE_INVALID_CERT: + case ERROR_WINHTTP_SECURE_CERT_REVOKED: + case ERROR_WINHTTP_SECURE_CERT_WRONG_USAGE: + case ERROR_WINHTTP_SECURE_FAILURE: + case ERROR_WINHTTP_REDIRECT_FAILED: + case ERROR_WINHTTP_INVALID_SERVER_RESPONSE: + case ERROR_WINHTTP_RESPONSE_DRAIN_OVERFLOW: + response->m_result = HttpResult_NetworkFailure; + break; + + default: + response->m_result = HttpResult_LocalFailure; + break; + } + } + + assert(isCallbackCalled == false); + if (!isCallbackCalled) + { + // Only one WinHTTP worker thread may invoke async callback for a given request at any given moment of + // time. That ensures that isCallbackCalled does not require a lock around it. We unregister the callback + // here to ensure that no more callbacks are coming for that m_hRequest. + ::WinHttpSetStatusCallback(m_hRequest, NULL, WINHTTP_CALLBACK_FLAG_ALL_COMPLETIONS, 0); + isCallbackCalled = true; + m_appCallback->OnHttpResponse(response.release()); + // HttpClient parent is destroying this HttpRequest object by id + m_parent.erase(m_id); + } + } + + private: + // Parses "Name: Value\r\n"-formatted raw headers (as returned by + // WINHTTP_QUERY_RAW_HEADERS_CRLF / HTTP_QUERY_RAW_HEADERS_CRLF) into an + // HttpHeaders map. Shared shape with HttpClient_WinInet's inline parser. + static void parseHeaders(std::string const& raw, SimpleHttpResponse& response) + { + char const* ptr = raw.c_str(); + while (*ptr) { + char const* colon = strchr(ptr, ':'); + if (!colon) { + break; + } + std::string name(ptr, colon); + + ptr = colon + 1; + while (*ptr == ' ') { + ptr++; + } + + char const* eol = strstr(ptr, "\r\n"); + if (!eol) { + break; + } + std::string value(ptr, eol); + + response.m_headers.add(name, value); + ptr = eol + 2; + } + } +}; + +//--- + +unsigned HttpClient_WinHttp::s_nextRequestId = 0; + +HttpClient_WinHttp::HttpClient_WinHttp() : + m_msRootCheck(false) +{ + // WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY (Windows 8.1+) resolves the proxy + // without depending on a logged-on interactive user or that user's + // Internet Explorer settings -- unlike WinInet's + // INTERNET_OPEN_TYPE_PRECONFIG, which requires one. This is why WinHTTP, + // not WinInet, is Microsoft's documented recommendation for services and + // other non-interactive processes. On an older OS that rejects this access + // type, fall back to no proxy rather than failing to construct at all. + m_hSession = ::WinHttpOpen( + NULL, WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY, + WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, WINHTTP_FLAG_ASYNC); + if (m_hSession == nullptr) + { + LOG_WARN("WinHttpOpen(AUTOMATIC_PROXY) failed: %d; retrying with no proxy", ::GetLastError()); + m_hSession = ::WinHttpOpen( + NULL, WINHTTP_ACCESS_TYPE_NO_PROXY, + WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, WINHTTP_FLAG_ASYNC); + } +} + +HttpClient_WinHttp::~HttpClient_WinHttp() +{ + CancelAllRequests(); + ::WinHttpCloseHandle(m_hSession); +} + +/** + * This method is called exclusively from onRequestComplete. + * No other code paths that lead to request destruction. + */ +void HttpClient_WinHttp::erase(std::string const& id) +{ + // Drop the map's shared_ptr reference under the lock. If a concurrent + // cancel() call (see its comment) is holding its own shared_ptr copy, the + // wrapper's actual destruction is deferred until that copy also goes out + // of scope -- never while any caller still holds a live reference. + { + std::lock_guard lock(m_requestsMutex); + m_requests.erase(id); + } + m_requestsCv.notify_all(); +} + +IHttpRequest* HttpClient_WinHttp::CreateRequest() +{ + std::string id = "WH-" + toString(::InterlockedIncrement(&s_nextRequestId)); + return new SimpleHttpRequest(id); +} + +void HttpClient_WinHttp::SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) +{ + // Note: 'request' is never owned by IHttpClient and gets deleted in EventsUploadContext.clear() + auto wrapper = std::make_shared(*this, static_cast(request)); + wrapper->send(callback); +} + +void HttpClient_WinHttp::CancelRequestAsync(std::string const& id) +{ + // Copy the shared_ptr out of the map while holding the lock only for the + // lookup, then call cancel() without the lock held (cancel() blocks in + // WinHttpCloseHandle waiting for a completion callback on another thread + // that needs this same lock -- see cancel()'s comment). The local copy + // keeps the wrapper alive for the duration of this call even if erase() + // concurrently removes the map's own reference. + std::shared_ptr request; + { + std::lock_guard lock(m_requestsMutex); + auto it = m_requests.find(id); + if (it != m_requests.end()) { + request = it->second; + } + } + if (request) { + request->cancel(); + } +} + +void HttpClient_WinHttp::CancelAllRequests() +{ + // vector of all request IDs + std::vector ids; + { + std::lock_guard lock(m_requestsMutex); + for (auto const& item : m_requests) { + ids.push_back(item.first); + } + } + // cancel all requests one-by-one not holding the lock + for (const auto& id : ids) + CancelRequestAsync(id); + + // Wait for all destructors to run, signaled from erase() rather than + // polled -- unlike a sleep-and-recheck loop, this drains the common case + // in microseconds and never busy-spins. + std::unique_lock lock(m_requestsMutex); + m_requestsCv.wait(lock, [this]() noexcept -> bool { + return m_requests.empty(); + }); +} + +/// +/// Enforces MS-root server certificate check. +/// +/// if set to true [enforce verification that server cert is MS-Rooted]. +void HttpClient_WinHttp::ApplySettings(ILogConfiguration& config) +{ + SetMsRootCheck(config[CFG_MAP_HTTP][CFG_BOOL_HTTP_MS_ROOT_CHECK]); +} + +void HttpClient_WinHttp::SetMsRootCheck(bool enforceMsRoot) +{ + m_msRootCheck = enforceMsRoot; +} + +/// +/// Determines whether MS-Roted server cert check required. +/// +/// +/// true if [MS-Rooted server cert check required]; otherwise, false. +/// +bool HttpClient_WinHttp::IsMsRootCheckRequired() +{ + return m_msRootCheck; +} + +} MAT_NS_END +#endif // HAVE_MAT_DEFAULT_HTTP_CLIENT +// clang-format on diff --git a/lib/http/HttpClient_WinHttp.hpp b/lib/http/HttpClient_WinHttp.hpp new file mode 100644 index 000000000..d9255ae87 --- /dev/null +++ b/lib/http/HttpClient_WinHttp.hpp @@ -0,0 +1,67 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#ifndef HTTPCLIENT_WINHTTP_HPP +#define HTTPCLIENT_WINHTTP_HPP + +#ifdef HAVE_MAT_DEFAULT_HTTP_CLIENT + +#include "IHttpClient.hpp" +#include "pal/PAL.hpp" + +#include "ILogManager.hpp" + +#include +#include + +namespace MAT_NS_BEGIN { + +#ifndef _WINHTTPX_ +typedef void* HINTERNET; +#endif + +class WinHttpRequestWrapper; + +// WinHTTP-based HTTP client. Unlike WinInet, WinHTTP does not depend on a +// logged-on interactive user or that user's Internet Explorer settings, so +// it is Microsoft's recommended transport for services and other +// non-interactive processes (see +// https://learn.microsoft.com/windows/win32/winhttp/porting-wininet-applications-to-winhttp). +// This is the default Win32 desktop transport; HttpClient_WinInet remains +// available as an explicit opt-in for callers that need IE-integrated proxy +// or cookie behavior. +class HttpClient_WinHttp : public IHttpClient { + public: + // Common IHttpClient methods + HttpClient_WinHttp(); + virtual ~HttpClient_WinHttp(); + virtual IHttpRequest* CreateRequest() final; + virtual void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) final; + virtual void CancelRequestAsync(std::string const& id) final; + virtual void CancelAllRequests() final; + + virtual void ApplySettings(ILogConfiguration& config) override; + + // Methods unique to WinHttp implementation. + void SetMsRootCheck(bool enforceMsRoot); + bool IsMsRootCheckRequired(); + + protected: + void erase(std::string const& id); + + protected: + HINTERNET m_hSession; + std::recursive_mutex m_requestsMutex; + std::condition_variable_any m_requestsCv; + std::map> m_requests; + static unsigned s_nextRequestId; + bool m_msRootCheck; + friend class WinHttpRequestWrapper; +}; + +} MAT_NS_END + +#endif // HAVE_MAT_DEFAULT_HTTP_CLIENT + +#endif // HTTPCLIENT_WINHTTP_HPP diff --git a/lib/pal/desktop/desktop.vcxitems b/lib/pal/desktop/desktop.vcxitems index 0d8ae8def..5679a6258 100644 --- a/lib/pal/desktop/desktop.vcxitems +++ b/lib/pal/desktop/desktop.vcxitems @@ -14,9 +14,11 @@ + + ..\..;..\..\include;$(ProjectDir);$(GeneratedFilesDir);$(IntDir);%(AdditionalIncludeDirectories);$(WindowsSDK_IncludePath) From e28ae450a7d4ff760bad283b96e8fc65e7eb1d8f Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 3 Aug 2026 11:38:30 -0500 Subject: [PATCH 33/50] fix winhttp teardown hang on cancellation WinHTTP cancellation paths could leave the request wrapper in the parent map if HANDLE_CLOSING arrived without a prior terminal callback. That made CancelAllRequests wait forever and matched the Windows CI timeout in sendManyRequestsAndCancel. Handle HANDLE_CLOSING as a terminal signal when the request has not yet completed, so the wrapper erases itself and teardown always drains. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7fe5faca-d77c-45c4-85d3-0d4a00d68a94 --- lib/http/HttpClient_WinHttp.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp index 3aa3b2212..399fc3eff 100644 --- a/lib/http/HttpClient_WinHttp.cpp +++ b/lib/http/HttpClient_WinHttp.cpp @@ -330,10 +330,14 @@ class WinHttpRequestWrapper : public std::enable_shared_from_thisisCallbackCalled) + { + self->onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); + } return; case WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE: From 035d2d47a23f305b2c0e8f9f315ceff3de253899 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 4 Aug 2026 15:17:28 -0500 Subject: [PATCH 34/50] Fix WinHTTP cancellation completion race Complete cancellation after WinHttpCloseHandle returns so HANDLE_CLOSING cannot dereference a destroyed request wrapper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_WinHttp.cpp | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp index 399fc3eff..6ce4c4ad6 100644 --- a/lib/http/HttpClient_WinHttp.cpp +++ b/lib/http/HttpClient_WinHttp.cpp @@ -89,7 +89,14 @@ class WinHttpRequestWrapper : public std::enable_shared_from_thisisCallbackCalled) - { - self->onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); - } + // HANDLE_CLOSING may arrive after the wrapper has been erased + // and destroyed, so it must not dereference the context. return; case WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE: @@ -495,7 +496,10 @@ class WinHttpRequestWrapper : public std::enable_shared_from_thisOnHttpResponse(response.release()); // HttpClient parent is destroying this HttpRequest object by id From 287dbc8bfb8a57e27ea817ba53574edbf5e5375a Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 4 Aug 2026 23:43:16 -0500 Subject: [PATCH 35/50] Join stress-test upload workers before teardown Prevent detached UploadNow threads from outliving the functional test and racing later LogManager lifetimes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/functests/APITest.cpp | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/tests/functests/APITest.cpp b/tests/functests/APITest.cpp index baea0112e..79f1e3f81 100644 --- a/tests/functests/APITest.cpp +++ b/tests/functests/APITest.cpp @@ -16,6 +16,8 @@ #include #include #include +#include +#include #include "PayloadDecoder.hpp" @@ -673,38 +675,32 @@ constexpr static unsigned MAX_THREADS = 25; /// The configuration. void StressUploadLockMultiThreaded(ILogConfiguration& config) { - std::srand(static_cast(std::time(nullptr))); TestDebugEventListener debugListener; addAllListeners(debugListener); size_t numIterations = MAX_ITERATIONS_MT; - std::mutex m_threads_mtx; - std::atomic threadCount(0); - while (numIterations--) { ILogger *result = LogManager::Initialize(TEST_TOKEN, config); - // Keep spawning UploadNow threads while the main thread is trying to perform - // Initialize and Teardown, but no more than MAX_THREADS at a time. + std::vector uploadThreads; + uploadThreads.reserve(MAX_THREADS); for (size_t i = 0; i < MAX_THREADS; i++) { - if (threadCount++ < MAX_THREADS) + uploadThreads.emplace_back([]() { - auto t = std::thread([&]() - { - std::this_thread::yield(); - LogManager::UploadNow(); - const auto randTimeSub2ms = std::rand() % 2; - PAL::sleep(randTimeSub2ms); - threadCount--; - }); - t.detach(); - } - }; + std::this_thread::yield(); + LogManager::UploadNow(); + PAL::sleep(0); + }); + } EventProperties props = testing::CreateSampleEvent("event_name", EventPriority_Normal); result->LogEvent(props); LogManager::FlushAndTeardown(); + for (auto& uploadThread : uploadThreads) + { + uploadThread.join(); + } } removeAllListeners(debugListener); } From f7fb6f43cc4a78ad5292c93cc547be18d2678967 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 5 Aug 2026 02:06:42 -0500 Subject: [PATCH 36/50] Prevent WinHTTP request wrapper use-after-free Remove completed requests before invoking application callbacks so concurrent teardown cannot destroy the wrapper while its terminal callback is still running. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_WinHttp.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp index 6ce4c4ad6..16ed9b705 100644 --- a/lib/http/HttpClient_WinHttp.cpp +++ b/lib/http/HttpClient_WinHttp.cpp @@ -501,9 +501,14 @@ class WinHttpRequestWrapper : public std::enable_shared_from_thisOnHttpResponse(response.release()); - // HttpClient parent is destroying this HttpRequest object by id - m_parent.erase(m_id); + auto callback = m_appCallback; + auto requestId = m_id; + auto keepAlive = shared_from_this(); + // Remove the request before entering application code. The callback + // can synchronously tear down the client and destroy this wrapper. + m_parent.erase(requestId); + callback->OnHttpResponse(response.release()); + keepAlive.reset(); } } From b2bd27bae8e4f6122fc98b7ceecb5406ed1b409b Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 6 Aug 2026 16:58:11 -0500 Subject: [PATCH 37/50] Align vcpkg iOS deployment target Ensure vcpkg-built Apple libraries match the consumer deployment target and avoid linker warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4259-b03b-8eeb87c06837 --- tools/ports/cpp-client-telemetry/portfile.cmake | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tools/ports/cpp-client-telemetry/portfile.cmake b/tools/ports/cpp-client-telemetry/portfile.cmake index b2fdab830..011c1c1f0 100644 --- a/tools/ports/cpp-client-telemetry/portfile.cmake +++ b/tools/ports/cpp-client-telemetry/portfile.cmake @@ -46,6 +46,14 @@ if(VCPKG_TARGET_IS_IOS) set(MATSDK_BUILD_IOS ON) endif() +# Keep the port's iOS deployment target aligned with the consumer test and the +# SDK's supported minimum instead of letting Clang default to the SDK version. +set(MATSDK_APPLE_DEPLOYMENT_OPTIONS) +if(VCPKG_TARGET_IS_IOS) + list(APPEND MATSDK_APPLE_DEPLOYMENT_OPTIONS + -DCMAKE_OSX_DEPLOYMENT_TARGET=12.0) +endif() + set(MATSDK_ANDROID_HTTP_CLIENT AUTO) if(VCPKG_TARGET_IS_ANDROID) file(READ "${SOURCE_PATH}/CMakeLists.txt" _matsdk_root_cmake) @@ -131,6 +139,7 @@ vcpkg_cmake_configure( -DBUILD_VERSION=${VERSION} -DBUILD_APPLE_HTTP=${MATSDK_BUILD_APPLE_HTTP} -DBUILD_IOS=${MATSDK_BUILD_IOS} + ${MATSDK_APPLE_DEPLOYMENT_OPTIONS} ) vcpkg_cmake_install() From ca440fcc40840174766dc7100120c11f53a65cd5 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 6 Aug 2026 17:27:11 -0500 Subject: [PATCH 38/50] Harden Apple packaging integration Propagate the resolved iOS sysroot to embedding builds and keep Apple vendored targets compatible with strict warning settings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837 --- CMakeLists.txt | 4 ++++ lib/CMakeLists.txt | 11 ++++++++++- lib/http/HttpClient_Apple.mm | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index cc36e9da3..7b0906f8e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -99,6 +99,10 @@ if(APPLE) OUTPUT_VARIABLE CMAKE_OSX_SYSROOT ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) + if(NOT CMAKE_OSX_SYSROOT) + message(FATAL_ERROR "Unable to resolve the Apple SDK sysroot for '${IOS_PLATFORM}'") + endif() + set(CMAKE_OSX_SYSROOT "${CMAKE_OSX_SYSROOT}" CACHE PATH "Apple SDK sysroot" FORCE) message(STATUS "CMAKE_OSX_SYSROOT ${CMAKE_OSX_SYSROOT}") message(STATUS "ARCHITECTURE: ${CMAKE_SYSTEM_PROCESSOR}") message(STATUS "PLATFORM: ${IOS_PLATFORM}") diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index 13b4d46d4..994fbf9b2 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -490,7 +490,12 @@ if(MATSDK_BUNDLE_SQLITE AND NOT TARGET sqlite3_bundled) else() # Unstripped vendored build (Android legacy): keep the existing narrower # warning suppression. -fno-finite-math-only guards the INFINITY macro. - target_compile_options(sqlite3_bundled PRIVATE -fno-finite-math-only -Wno-unused-function) + target_compile_options(sqlite3_bundled PRIVATE + -fno-finite-math-only + -Wno-unused-function + -Wno-shorten-64-to-32 + -Wno-ambiguous-macro + ) endif() endif() @@ -561,6 +566,10 @@ else() # real POSIX declarations for read/write/lseek/close instead of relying on # implicit (int-returning) declarations. target_compile_definitions(zlib_bundled PRIVATE Z_HAVE_UNISTD_H) + target_compile_options(zlib_bundled PRIVATE + -Wno-shorten-64-to-32 + -Wno-ambiguous-macro + ) target_link_libraries(mat PRIVATE sqlite3_bundled zlib_bundled ${LIBS}) elseif(PAL_IMPLEMENTATION STREQUAL "WIN32") diff --git a/lib/http/HttpClient_Apple.mm b/lib/http/HttpClient_Apple.mm index b7d6646a4..1a047f5d6 100644 --- a/lib/http/HttpClient_Apple.mm +++ b/lib/http/HttpClient_Apple.mm @@ -207,7 +207,7 @@ void HandleResponse(NSData* data, NSURLResponse* response, NSError* error) NSHTTPURLResponse *httpResp = static_cast(response); auto simpleResponse = new SimpleHttpResponse { NextRespId() }; - simpleResponse->m_statusCode = httpResp.statusCode; + simpleResponse->m_statusCode = static_cast(httpResp.statusCode); NSDictionary *responseHeaders = [httpResp allHeaderFields]; for (id key in responseHeaders) From c96f7de31bf4c4076b70ad6ea315e3fa039b7f75 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 6 Aug 2026 20:58:41 -0500 Subject: [PATCH 39/50] Migrate Apple builds to canonical CMake variables Remove legacy Apple architecture, platform, and deployment-target inputs so standalone scripts and embedding consumers share CMAKE_OSX_* configuration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837 --- .github/workflows/build-ios-mac.yml | 4 +- CMakeLists.txt | 80 ++++++----------------------- build-gtest.sh | 3 +- build-ios.sh | 30 +++++------ build.sh | 21 ++++---- 5 files changed, 41 insertions(+), 97 deletions(-) diff --git a/.github/workflows/build-ios-mac.yml b/.github/workflows/build-ios-mac.yml index 7ca85012b..af77f8b35 100644 --- a/.github/workflows/build-ios-mac.yml +++ b/.github/workflows/build-ios-mac.yml @@ -61,8 +61,8 @@ jobs: - name: build run: | if [[ "${{ matrix.os }}" == "macos-14" ]]; then - export IOS_DEPLOYMENT_TARGET=13.0; + export CMAKE_OSX_DEPLOYMENT_TARGET=13.0; elif [[ "${{ matrix.os }}" == "macos-15" ]]; then - export IOS_DEPLOYMENT_TARGET=15.0; + export CMAKE_OSX_DEPLOYMENT_TARGET=15.0; fi ./build-tests-ios.sh ${{ matrix.config }} ${{ matrix.simulator }} diff --git a/CMakeLists.txt b/CMakeLists.txt index 7b0906f8e..69785b37c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -47,92 +47,42 @@ if(APPLE) message(STATUS "BUILD_IOS: ${BUILD_IOS}") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fobjc-arc") - # iOS build options - option(BUILD_IOS "Build for iOS" NO) - option(FORCE_RESET_OSX_DEPLOYMENT_TARGET "Clear the OSX Deployment Target Set" YES) - if (DEFINED FORCE_RESET_DEPLOYMENT_TARGET) - set(FORCE_RESET_OSX_DEPLOYMENT_TARGET ${FORCE_RESET_DEPLOYMENT_TARGET}) - endif() + option(BUILD_IOS "Build for iOS-family Apple platforms" NO) # When building via vcpkg, the toolchain file handles architecture, sysroot, # deployment target, and platform flags. Skip manual flag configuration. if(NOT MATSDK_USE_VCPKG_DEPS) + if(CMAKE_SYSTEM_NAME MATCHES "^(iOS|visionOS)$") + set(BUILD_IOS ON) + endif() if(BUILD_IOS) set(TARGET_ARCH "APPLE") - set(IOS True) set(APPLE True) - if(FORCE_RESET_OSX_DEPLOYMENT_TARGET) - set(CMAKE_OSX_DEPLOYMENT_TARGET "" CACHE STRING "Force unset of the deployment target for iOS" FORCE) - if (${IOS_PLAT} STREQUAL "iphonesimulator") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -mios-simulator-version-min=${IOS_DEPLOYMENT_TARGET}") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -mios-simulator-version-min=${IOS_DEPLOYMENT_TARGET}") - else() - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -miphoneos-version-min=${IOS_DEPLOYMENT_TARGET}") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -miphoneos-version-min=${IOS_DEPLOYMENT_TARGET}") - endif() - endif() - - if((${IOS_PLAT} STREQUAL "iphoneos") OR (${IOS_PLAT} STREQUAL "iphonesimulator") OR (${IOS_PLAT} STREQUAL "xros") OR (${IOS_PLAT} STREQUAL "xrsimulator")) - set(IOS_PLATFORM "${IOS_PLAT}") - else() - message(FATAL_ERROR "Unrecognized iOS platform '${IOS_PLAT}'") - endif() - - if(${IOS_ARCH} STREQUAL "x86_64") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch x86_64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch x86_64") - set(CMAKE_SYSTEM_PROCESSOR x86_64) - elseif(${IOS_ARCH} STREQUAL "arm64") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch arm64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch arm64") - set(CMAKE_SYSTEM_PROCESSOR arm64) - elseif(${IOS_ARCH} STREQUAL "arm64e") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch arm64e") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch arm64e") - set(CMAKE_SYSTEM_PROCESSOR arm64e) - else() - message(FATAL_ERROR "Unrecognized iOS architecture '${IOS_ARCH}'") + if(NOT CMAKE_OSX_SYSROOT) + message(FATAL_ERROR "CMAKE_OSX_SYSROOT must identify an Apple SDK") endif() - - execute_process(COMMAND xcodebuild -version -sdk ${IOS_PLATFORM} ONLY_ACTIVE_ARCH=NO Path + if(NOT IS_ABSOLUTE "${CMAKE_OSX_SYSROOT}") + execute_process(COMMAND xcodebuild -version -sdk "${CMAKE_OSX_SYSROOT}" Path OUTPUT_VARIABLE CMAKE_OSX_SYSROOT ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) - if(NOT CMAKE_OSX_SYSROOT) - message(FATAL_ERROR "Unable to resolve the Apple SDK sysroot for '${IOS_PLATFORM}'") + if(NOT CMAKE_OSX_SYSROOT) + message(FATAL_ERROR "Unable to resolve the Apple SDK sysroot") + endif() + set(CMAKE_OSX_SYSROOT "${CMAKE_OSX_SYSROOT}" CACHE PATH "Apple SDK sysroot" FORCE) endif() - set(CMAKE_OSX_SYSROOT "${CMAKE_OSX_SYSROOT}" CACHE PATH "Apple SDK sysroot" FORCE) message(STATUS "CMAKE_OSX_SYSROOT ${CMAKE_OSX_SYSROOT}") message(STATUS "ARCHITECTURE: ${CMAKE_SYSTEM_PROCESSOR}") - message(STATUS "PLATFORM: ${IOS_PLATFORM}") + message(STATUS "DEPLOYMENT TARGET: ${CMAKE_OSX_DEPLOYMENT_TARGET}") else() - if("${MAC_ARCH}" STREQUAL "x86_64") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch x86_64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch x86_64") - set(CMAKE_SYSTEM_PROCESSOR x86_64) - set(TARGET_ARCH ${CMAKE_SYSTEM_PROCESSOR}) - set(CMAKE_OSX_ARCHITECTURES ${MAC_ARCH}) - set(APPLE True) - elseif("${MAC_ARCH}" STREQUAL "arm64") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch arm64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch arm64") - set(CMAKE_SYSTEM_PROCESSOR arm64) - set(TARGET_ARCH ${CMAKE_SYSTEM_PROCESSOR}) - set(CMAKE_OSX_ARCHITECTURES ${MAC_ARCH}) - set(APPLE True) - else() - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch x86_64 -arch arm64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch x86_64 -arch arm64") - endif() - message(STATUS "MAC_ARCH: ${MAC_ARCH}") + message(STATUS "ARCHITECTURES: ${CMAKE_OSX_ARCHITECTURES}") endif() else() # vcpkg mode: just set internal flags from what the toolchain provides - if(BUILD_IOS OR CMAKE_SYSTEM_NAME STREQUAL "iOS") + if(BUILD_IOS OR CMAKE_SYSTEM_NAME MATCHES "^(iOS|visionOS)$") set(BUILD_IOS ON) set(TARGET_ARCH "APPLE") - set(IOS True) endif() message(STATUS "vcpkg toolchain managing architecture and platform flags") endif() diff --git a/build-gtest.sh b/build-gtest.sh index 4c73f3382..4dca08f06 100755 --- a/build-gtest.sh +++ b/build-gtest.sh @@ -39,9 +39,8 @@ if(BUILD_IOS) set(CMAKE_OSX_DEPLOYMENT_TARGET "12.2" CACHE STRING "Force set of the deployment target for iOS" FORCE) set(CMAKE_C_FLAGS "\${CMAKE_C_FLAGS} -miphoneos-version-min=10.0") set(CMAKE_CXX_FLAGS "\${CMAKE_CXX_FLAGS} -miphoneos-version-min=10.0 -std=c++11") - set(IOS_PLATFORM "iphonesimulator") set(CMAKE_SYSTEM_PROCESSOR x86_64) - execute_process(COMMAND xcodebuild -version -sdk \${IOS_PLATFORM} Path + execute_process(COMMAND xcodebuild -version -sdk iphonesimulator Path OUTPUT_VARIABLE CMAKE_OSX_SYSROOT_OUT ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) diff --git a/build-ios.sh b/build-ios.sh index d316fe2fa..be53816e2 100755 --- a/build-ios.sh +++ b/build-ios.sh @@ -25,51 +25,47 @@ elif [ "$1" == "debug" ]; then fi # Set Architecture: arm64, arm64e or x86_64 -IOS_ARCH=$(/usr/bin/uname -m) +APPLE_ARCH=$(/usr/bin/uname -m) if [ "$1" == "arm64" ]; then - IOS_ARCH="arm64" + APPLE_ARCH="arm64" shift elif [ "$1" == "arm64e" ]; then - IOS_ARCH="arm64e" + APPLE_ARCH="arm64e" shift elif [ "$1" == "x86_64" ]; then - IOS_ARCH="x86_64" + APPLE_ARCH="x86_64" shift fi # the last param is expected to specify the platform name: iphoneos|iphonesimulator|xros|xrsimulator # so if it is non-empty and it is not "device", we take it as a valid platform name # otherwise we fall back to old iOS logic which only supported iphoneos|iphonesimulator -IOS_PLAT="iphonesimulator" +APPLE_PLATFORM="iphonesimulator" if [ -n "$1" ] && [ "$1" != "device" ]; then - IOS_PLAT="$1" + APPLE_PLATFORM="$1" elif [ "$1" == "device" ]; then - IOS_PLAT="iphoneos" + APPLE_PLATFORM="iphoneos" fi -echo "IOS_ARCH = $IOS_ARCH, IOS_PLAT = $IOS_PLAT, BUILD_TYPE = $BUILD_TYPE" +echo "architecture = $APPLE_ARCH, platform = $APPLE_PLATFORM, build type = $BUILD_TYPE" -FORCE_RESET_DEPLOYMENT_TARGET=NO DEPLOYMENT_TARGET="" -if [ "$IOS_PLAT" == "iphoneos" ] || [ "$IOS_PLAT" == "iphonesimulator" ]; then +if [ "$APPLE_PLATFORM" == "iphoneos" ] || [ "$APPLE_PLATFORM" == "iphonesimulator" ]; then SYS_NAME="iOS" - DEPLOYMENT_TARGET="$IOS_DEPLOYMENT_TARGET" + DEPLOYMENT_TARGET="$CMAKE_OSX_DEPLOYMENT_TARGET" if [ -z "$DEPLOYMENT_TARGET" ]; then DEPLOYMENT_TARGET="12.0" - FORCE_RESET_DEPLOYMENT_TARGET=YES fi -elif [ "$IOS_PLAT" == "xros" ] || [ "$IOS_PLAT" == "xrsimulator" ]; then +elif [ "$APPLE_PLATFORM" == "xros" ] || [ "$APPLE_PLATFORM" == "xrsimulator" ]; then SYS_NAME="visionOS" - DEPLOYMENT_TARGET="$XROS_DEPLOYMENT_TARGET" + DEPLOYMENT_TARGET="$CMAKE_OSX_DEPLOYMENT_TARGET" if [ -z "$DEPLOYMENT_TARGET" ]; then DEPLOYMENT_TARGET="1.0" - FORCE_RESET_DEPLOYMENT_TARGET=YES fi fi echo "deployment target = $DEPLOYMENT_TARGET" -echo "force reset deployment target = $FORCE_RESET_DEPLOYMENT_TARGET" # Install build tools and recent sqlite3 FILE=".buildtools" @@ -92,7 +88,7 @@ cd out CMAKE_PACKAGE_TYPE=tgz -cmake_cmd="cmake -DCMAKE_OSX_SYSROOT=$IOS_PLAT -DCMAKE_SYSTEM_NAME=$SYS_NAME -DCMAKE_IOS_ARCH_ABI=$IOS_ARCH -DCMAKE_OSX_DEPLOYMENT_TARGET=$DEPLOYMENT_TARGET -DBUILD_IOS=YES -DIOS_ARCH=$IOS_ARCH -DIOS_PLAT=$IOS_PLAT -DIOS_DEPLOYMENT_TARGET=$DEPLOYMENT_TARGET -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_PACKAGE_TYPE=$CMAKE_PACKAGE_TYPE -DFORCE_RESET_DEPLOYMENT_TARGET=$FORCE_RESET_DEPLOYMENT_TARGET $CMAKE_OPTS .." +cmake_cmd="cmake -DCMAKE_OSX_SYSROOT=$APPLE_PLATFORM -DCMAKE_SYSTEM_NAME=$SYS_NAME -DCMAKE_OSX_ARCHITECTURES=$APPLE_ARCH -DCMAKE_OSX_DEPLOYMENT_TARGET=$DEPLOYMENT_TARGET -DBUILD_IOS=YES -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_PACKAGE_TYPE=$CMAKE_PACKAGE_TYPE $CMAKE_OPTS .." echo "${cmake_cmd}" eval $cmake_cmd diff --git a/build.sh b/build.sh index 52a5081b2..07dd19a6f 100755 --- a/build.sh +++ b/build.sh @@ -61,13 +61,13 @@ while [[ $# -gt 0 ]]; do echo "BUILD_TYPE = $BUILD_TYPE" ;; arm64|x86_64|universal) - if [[ -n "$MAC_ARCH" ]]; then - echo "Error: MAC_ARCH is already set to '$MAC_ARCH'. Cannot overwrite with $ARG." 1>&2 + if [[ -n "$APPLE_ARCH" ]]; then + echo "Error: APPLE_ARCH is already set to '$APPLE_ARCH'. Cannot overwrite with $ARG." 1>&2 exit 1 else - MAC_ARCH="$ARG" + APPLE_ARCH="$ARG" fi - echo "MAC_ARCH = $MAC_ARCH" + echo "APPLE_ARCH = $APPLE_ARCH" ;; CUSTOM_BUILD_FLAGS*) CUSTOM_CMAKE_CXX_FLAG="\"${ARG:19:999}\"" @@ -91,9 +91,9 @@ if [[ -z "$BUILD_TYPE" ]]; then echo "Assuming default BUILD_TYPE = Debug" fi -if [[ -z "$MAC_ARCH" ]]; then - MAC_ARCH=$(/usr/bin/uname -m) - echo "Using current machine MAC_ARCH = $MAC_ARCH" +if [[ -z "$APPLE_ARCH" ]]; then + APPLE_ARCH=$(/usr/bin/uname -m) + echo "Using current machine APPLE_ARCH = $APPLE_ARCH" fi # Evaluate switches @@ -137,7 +137,7 @@ if [ "$LINK_TYPE" == "shared" ]; then fi # Set target MacOS minver -default_mac_os_target=$([ "$MAC_ARCH" == "arm64" ] && echo "11.10" || echo "10.10") +default_mac_os_target=$([ "$APPLE_ARCH" == "arm64" ] && echo "11.10" || echo "10.10") [ -z $MACOSX_DEPLOYMENT_TARGET ] && export MACOSX_DEPLOYMENT_TARGET=${default_mac_os_target} echo "macosx deployment target="$MACOSX_DEPLOYMENT_TARGET @@ -147,7 +147,7 @@ OS_NAME=`uname -a` if [ ! -f $FILE ]; then case "$OS_NAME" in - *Darwin*) CMD="tools/setup-buildtools-apple.sh $MAC_ARCH" ;; + *Darwin*) CMD="tools/setup-buildtools-apple.sh $APPLE_ARCH" ;; *Linux*) CMD="tools/setup-buildtools.sh" ;; *) CMD=""; echo "WARNING: unsupported OS $OS_NAME, skipping build tools installation.." ;; esac @@ -185,8 +185,7 @@ fi # Fail on error set -e -# TODO: should this be improved to verify if the platform is Apple? Right now we unconditionally pass -DMAC_ARCH even if building for Windows or Linux. -cmake_cmd="cmake -DMAC_ARCH=$MAC_ARCH -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_PACKAGE_TYPE=$CMAKE_PACKAGE_TYPE -DCMAKE_CXX_FLAGS="${CUSTOM_CMAKE_CXX_FLAG}" $CMAKE_OPTS .." +cmake_cmd="cmake -DCMAKE_OSX_ARCHITECTURES=$APPLE_ARCH -DCMAKE_OSX_DEPLOYMENT_TARGET=$MACOSX_DEPLOYMENT_TARGET -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_PACKAGE_TYPE=$CMAKE_PACKAGE_TYPE -DCMAKE_CXX_FLAGS="${CUSTOM_CMAKE_CXX_FLAG}" $CMAKE_OPTS .." echo $cmake_cmd eval $cmake_cmd From 50d283a69a69db6524510619a15870d5356484b6 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 7 Aug 2026 11:32:17 -0500 Subject: [PATCH 40/50] Remove unused Windows transport dependencies Keep both selectable HTTP backends linked privately while dropping the unused Winsock dependency and headers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/CMakeLists.txt | 2 +- lib/http/HttpClient_WinHttp.cpp | 1 - lib/http/HttpClient_WinInet.cpp | 3 +-- lib/http/HttpClient_WinRt.cpp | 3 --- lib/pal/desktop/WindowsDesktopDeviceInformationImpl.cpp | 5 ----- 5 files changed, 2 insertions(+), 12 deletions(-) diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index fc0475c43..54e70e58e 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -617,7 +617,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux" OR CMAKE_SYSTEM_NAME STREQUAL "Android") target_link_libraries(mat PUBLIC log) endif() elseif(PAL_IMPLEMENTATION STREQUAL "WIN32") - target_link_libraries(mat PUBLIC wininet winhttp crypt32 ws2_32) + target_link_libraries(mat PRIVATE wininet winhttp crypt32) elseif(APPLE) target_link_libraries(mat PUBLIC "-framework CoreFoundation" diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp index 16ed9b705..0709ba415 100644 --- a/lib/http/HttpClient_WinHttp.cpp +++ b/lib/http/HttpClient_WinHttp.cpp @@ -13,7 +13,6 @@ #include #include -#include #include #include #include diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index b1d3b4013..43669d9bf 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/lib/http/HttpClient_WinInet.cpp @@ -14,7 +14,6 @@ #include #include -#include #include #include #include @@ -571,7 +570,7 @@ void HttpClient_WinInet::SetMsRootCheck(bool enforceMsRoot) } /// -/// Determines whether MS-Roted server cert check required. +/// Determines whether an MS-Rooted server certificate check is required. /// /// /// true if [MS-Rooted server cert check required]; otherwise, false. diff --git a/lib/http/HttpClient_WinRt.cpp b/lib/http/HttpClient_WinRt.cpp index 1efc1bb22..6ac1993d9 100644 --- a/lib/http/HttpClient_WinRt.cpp +++ b/lib/http/HttpClient_WinRt.cpp @@ -11,9 +11,7 @@ #include "http/HttpClient_WinRt.hpp" #include "utils/StringUtils.hpp" -#include #include -#include #include #include @@ -21,7 +19,6 @@ #include #include #include -#include using namespace Windows::Foundation; using namespace Windows::Foundation::Collections; diff --git a/lib/pal/desktop/WindowsDesktopDeviceInformationImpl.cpp b/lib/pal/desktop/WindowsDesktopDeviceInformationImpl.cpp index f01992940..3c8fe6baf 100644 --- a/lib/pal/desktop/WindowsDesktopDeviceInformationImpl.cpp +++ b/lib/pal/desktop/WindowsDesktopDeviceInformationImpl.cpp @@ -13,16 +13,12 @@ MATSDK_LOG_INST_COMPONENT_NS("DeviceInfo", "Win32 Desktop Device Information") -#include #include #include #include #include #include -#include -#include - #pragma comment(lib, "iphlpapi.lib") #pragma comment(lib, "AdvAPI32.Lib") @@ -149,4 +145,3 @@ namespace PAL_NS_BEGIN { } } PAL_NS_END - From aa80a93a46bd8e3e26cb33840dd184e2c3c66fc3 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 00:05:41 -0500 Subject: [PATCH 41/50] Fix WinHTTP duplicate completion during teardown Join upload workers before SDK teardown and cover in-flight cancellation with a deterministic HTTP test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/http/HttpClient_WinHttp.cpp | 14 +++--- tests/functests/APITest.cpp | 2 +- tests/unittests/CMakeLists.txt | 4 +- tests/unittests/HttpClientTests.cpp | 67 ++++++++++++++++++++++++++++- 4 files changed, 77 insertions(+), 10 deletions(-) diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp index 0709ba415..45a0cbe4f 100644 --- a/lib/http/HttpClient_WinHttp.cpp +++ b/lib/http/HttpClient_WinHttp.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -32,7 +33,7 @@ class WinHttpRequestWrapper : public std::enable_shared_from_this m_bodyBuffer; std::vector m_readBuffer; - bool isCallbackCalled {false}; + std::atomic isCallbackCalled {false}; bool isAborted {false}; public: @@ -410,6 +411,11 @@ class WinHttpRequestWrapper : public std::enable_shared_from_this response(new SimpleHttpResponse(m_id)); if (dwError == ERROR_SUCCESS) { @@ -489,17 +495,11 @@ class WinHttpRequestWrapper : public std::enable_shared_from_thisLogEvent(props); - LogManager::FlushAndTeardown(); for (auto& uploadThread : uploadThreads) { uploadThread.join(); } + LogManager::FlushAndTeardown(); } removeAllListeners(debugListener); } diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt index 7233d2920..e098d1ca0 100644 --- a/tests/unittests/CMakeLists.txt +++ b/tests/unittests/CMakeLists.txt @@ -52,7 +52,9 @@ set(SRCS ZlibUtilsTests.cpp ) -set_source_files_properties(${SRCS} PROPERTIES COMPILE_FLAGS -Wno-deprecated-declarations) +if(NOT MSVC) + set_source_files_properties(${SRCS} PROPERTIES COMPILE_FLAGS -Wno-deprecated-declarations) +endif() # Enable Azure Monitor unit tests when the module is present. # The AIJsonSerializer test sources are guarded by HAVE_MAT_AI. diff --git a/tests/unittests/HttpClientTests.cpp b/tests/unittests/HttpClientTests.cpp index 4b17bcce5..951dac34a 100644 --- a/tests/unittests/HttpClientTests.cpp +++ b/tests/unittests/HttpClientTests.cpp @@ -10,6 +10,8 @@ #include "common/HttpServer.hpp" #include "http/HttpClientFactory.hpp" +#include + using namespace testing; using namespace MAT; @@ -29,6 +31,11 @@ class HttpClientTests : public ::testing::Test, enum RequestState { Planned, Sent, Processed, Done }; std::vector _countedRequests; std::mutex _lock; + std::condition_variable _responseCv; + std::condition_variable _blockedRequestCv; + std::mutex _blockedRequestLock; + bool _blockedRequestReceived {false}; + bool _releaseBlockedRequest {false}; public: HttpClientTests() @@ -59,6 +66,7 @@ class HttpClientTests : public ::testing::Test, _server.addHandler("/simple/", *this); _server.addHandler("/echo/", *this); _server.addHandler("/count/", *this); + _server.addHandler("/block/", *this); _server.start(); Clear(); @@ -66,6 +74,11 @@ class HttpClientTests : public ::testing::Test, virtual void TearDown() override { + { + std::lock_guard lock(_blockedRequestLock); + _releaseBlockedRequest = true; + } + _blockedRequestCv.notify_all(); _server.stop(); _client.reset(); Clear(); @@ -87,6 +100,17 @@ class HttpClientTests : public ::testing::Test, return 200; } + if (request.uri == "/block/") { + { + std::lock_guard lock(_blockedRequestLock); + _blockedRequestReceived = true; + } + _blockedRequestCv.notify_all(); + std::unique_lock lock(_blockedRequestLock); + _blockedRequestCv.wait(lock, [this]() { return _releaseBlockedRequest; }); + return 200; + } + if (request.uri.substr(0, 7) == "/count/") { int id = atoi(request.uri.substr(7).c_str()); if (id >= 0 && static_cast(id) < _countedRequests.size()) { @@ -119,6 +143,7 @@ class HttpClientTests : public ::testing::Test, { std::lock_guard lock(_lock); _responses.push_back(clone(inResponse)); + _responseCv.notify_all(); } }; @@ -128,6 +153,47 @@ std::vector Binary(std::string const& str) return std::vector(str.data(), str.data() + str.size()); } +TEST_F(HttpClientTests, HandlesCancellationWhileResponseIsInFlight) +{ + Clear(); + { + std::lock_guard lock(_blockedRequestLock); + _blockedRequestReceived = false; + _releaseBlockedRequest = false; + } + + std::unique_ptr request(_client->CreateRequest()); + std::string requestId = request->GetId(); + request->SetUrl("http://" + _hostname + "/block/"); + _client->SendRequestAsync(request.release(), this); + + { + std::unique_lock lock(_blockedRequestLock); + ASSERT_TRUE(_blockedRequestCv.wait_for(lock, std::chrono::seconds(10), + [this]() { return _blockedRequestReceived; })); + } + + _client->CancelRequestAsync(requestId); + { + std::lock_guard lock(_blockedRequestLock); + _releaseBlockedRequest = true; + } + _blockedRequestCv.notify_all(); + + std::unique_ptr response; + { + std::unique_lock lock(_lock); + ASSERT_TRUE(_responseCv.wait_for(lock, std::chrono::seconds(2), + [this]() { return !_responses.empty(); })); + ASSERT_EQ(_responses.size(), 1u); + response.reset(_responses[0]); + _responses.clear(); + } + + EXPECT_THAT(response->GetId(), requestId); + EXPECT_THAT(response->GetResult(), HttpResult_Aborted); +} + //--- TEST_F(HttpClientTests, HandlesSimpleRequest) @@ -346,4 +412,3 @@ TEST_F(HttpClientTests, SurvivesManyRequests) } #endif // HAVE_MAT_DEFAULT_HTTP_CLIENT - From d9522021f68bd2ddd27abb874a95040f99050890 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 01:08:03 -0500 Subject: [PATCH 42/50] Keep WinHTTP callback context alive through close Route callbacks through a weak request reference so late WinHTTP notifications cannot dereference a destroyed wrapper during teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/http/HttpClient_WinHttp.cpp | 46 ++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp index 45a0cbe4f..3a28fcc17 100644 --- a/lib/http/HttpClient_WinHttp.cpp +++ b/lib/http/HttpClient_WinHttp.cpp @@ -16,12 +16,25 @@ #include #include #include +#include #include #pragma comment(lib, "winhttp.lib") namespace MAT_NS_BEGIN { +class WinHttpRequestWrapper; + +struct WinHttpCallbackContext +{ + explicit WinHttpCallbackContext(std::shared_ptr request) + : request(std::move(request)) + { + } + + std::weak_ptr request; +}; + class WinHttpRequestWrapper : public std::enable_shared_from_this { protected: @@ -35,6 +48,7 @@ class WinHttpRequestWrapper : public std::enable_shared_from_this m_readBuffer; std::atomic isCallbackCalled {false}; bool isAborted {false}; + WinHttpCallbackContext* m_callbackContext {nullptr}; public: WinHttpRequestWrapper(HttpClient_WinHttp& parent, SimpleHttpRequest* request) @@ -298,12 +312,15 @@ class WinHttpRequestWrapper : public std::enable_shared_from_thism_body.empty() ? nullptr : static_cast(m_request->m_body.data()); DWORD size = static_cast(m_request->m_body.size()); - DWORD_PTR context = reinterpret_cast(this); + m_callbackContext = new WinHttpCallbackContext(shared_from_this()); + DWORD_PTR context = reinterpret_cast(m_callbackContext); BOOL bResult = ::WinHttpSendRequest( m_hRequest, WINHTTP_NO_ADDITIONAL_HEADERS, 0, data, size, size, context); if (!bResult) { DWORD dwError = ::GetLastError(); + delete m_callbackContext; + m_callbackContext = nullptr; LOG_WARN("WinHttpSendRequest() failed: %d", dwError); // Unable to send request DispatchEvent(OnSendFailed); @@ -326,21 +343,30 @@ class WinHttpRequestWrapper : public std::enable_shared_from_this(dwContext); + WinHttpCallbackContext* context = reinterpret_cast(dwContext); + if (context == nullptr) + { + return; + } + + if (dwInternetStatus == WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING) + { + // The callback context outlives the request wrapper and is released + // only by WinHTTP's final notification. + delete context; + return; + } + + std::shared_ptr self = context->request.lock(); if (self == nullptr) { return; } - LOG_TRACE("winHttpCallback: hInternet %p, self %p, dwInternetStatus %u", hInternet, self, dwInternetStatus); + LOG_TRACE("winHttpCallback: hInternet %p, self %p, dwInternetStatus %u", hInternet, self.get(), dwInternetStatus); switch (dwInternetStatus) { - case WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING: - // HANDLE_CLOSING may arrive after the wrapper has been erased - // and destroyed, so it must not dereference the context. - return; - case WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE: if (!::WinHttpReceiveResponse(self->m_hRequest, NULL)) { @@ -496,10 +522,6 @@ class WinHttpRequestWrapper : public std::enable_shared_from_this Date: Sat, 8 Aug 2026 02:57:57 -0500 Subject: [PATCH 43/50] Make cancellation stress test deterministic Avoid external collector network delays so teardown behavior is reproducible in CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- tests/functests/BasicFuncTests.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/functests/BasicFuncTests.cpp b/tests/functests/BasicFuncTests.cpp index bc879d3e6..25a9cdc9c 100644 --- a/tests/functests/BasicFuncTests.cpp +++ b/tests/functests/BasicFuncTests.cpp @@ -1364,7 +1364,9 @@ TEST_F(BasicFuncTests, sendManyRequestsAndCancel) configuration[CFG_INT_RAM_QUEUE_SIZE] = 4096 * 20; configuration[CFG_STR_CACHE_FILE_PATH] = TEST_STORAGE_FILENAME; configuration[CFG_MAP_HTTP][CFG_BOOL_HTTP_COMPRESSION] = true; - configuration[CFG_STR_COLLECTOR_URL] = COLLECTOR_URL_PROD; + // Keep this teardown stress test deterministic; the in-flight + // cancellation behavior is covered by the local HTTP client test. + configuration[CFG_STR_COLLECTOR_URL] = serverAddress.c_str(); configuration[CFG_INT_MAX_TEARDOWN_TIME] = (int64_t)(i % 2); configuration[CFG_INT_TRACE_LEVEL_MASK] = 0; configuration[CFG_INT_TRACE_LEVEL_MIN] = ACTTraceLevel_Warn; From 0303c832d3a84af11390fd8e0032e43667c484a3 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 03:07:24 -0500 Subject: [PATCH 44/50] Avoid fixture socket overflow in cancellation stress test Use a closed localhost port instead of creating hundreds of concurrent fixture connections. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- tests/functests/BasicFuncTests.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/functests/BasicFuncTests.cpp b/tests/functests/BasicFuncTests.cpp index 25a9cdc9c..f54bebd80 100644 --- a/tests/functests/BasicFuncTests.cpp +++ b/tests/functests/BasicFuncTests.cpp @@ -1364,9 +1364,9 @@ TEST_F(BasicFuncTests, sendManyRequestsAndCancel) configuration[CFG_INT_RAM_QUEUE_SIZE] = 4096 * 20; configuration[CFG_STR_CACHE_FILE_PATH] = TEST_STORAGE_FILENAME; configuration[CFG_MAP_HTTP][CFG_BOOL_HTTP_COMPRESSION] = true; - // Keep this teardown stress test deterministic; the in-flight - // cancellation behavior is covered by the local HTTP client test. - configuration[CFG_STR_COLLECTOR_URL] = serverAddress.c_str(); + // Use a closed local port so this teardown stress test does not depend + // on external networking or overflow the fixture server's socket set. + configuration[CFG_STR_COLLECTOR_URL] = "http://127.0.0.1:1/"; configuration[CFG_INT_MAX_TEARDOWN_TIME] = (int64_t)(i % 2); configuration[CFG_INT_TRACE_LEVEL_MASK] = 0; configuration[CFG_INT_TRACE_LEVEL_MIN] = ACTTraceLevel_Warn; From f03af17b3a1eddfaa52b3f1bbd61ec4690c8ec9e Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 11:08:57 -0500 Subject: [PATCH 45/50] Prepare WinHTTP for bounded cancellation Expose the transport capability required by the upcoming cancellation-drain changes and correct the certificate-check documentation typo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/CMakeLists.txt | 1 + lib/http/HttpClient_WinHttp.cpp | 25 ++++++++++++++++++++----- lib/http/HttpClient_WinHttp.hpp | 4 +++- lib/http/IBoundedHttpClientCancel.hpp | 23 +++++++++++++++++++++++ 4 files changed, 47 insertions(+), 6 deletions(-) create mode 100644 lib/http/IBoundedHttpClientCancel.hpp diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index 88cd14412..58f68baf5 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -309,6 +309,7 @@ endif() http/HttpClient_WinInet.hpp http/HttpClient_WinHttp.cpp http/HttpClient_WinHttp.hpp + http/IBoundedHttpClientCancel.hpp pal/desktop/WindowsDesktopDeviceInformationImpl.cpp pal/desktop/WindowsDesktopNetworkInformationImpl.cpp pal/desktop/WindowsDesktopSystemInformationImpl.cpp diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp index 3a28fcc17..e5f8af0e9 100644 --- a/lib/http/HttpClient_WinHttp.cpp +++ b/lib/http/HttpClient_WinHttp.cpp @@ -648,6 +648,11 @@ void HttpClient_WinHttp::CancelRequestAsync(std::string const& id) } void HttpClient_WinHttp::CancelAllRequests() +{ + CancelAllRequests(std::chrono::milliseconds::zero()); +} + +void HttpClient_WinHttp::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) { // vector of all request IDs std::vector ids; @@ -663,11 +668,21 @@ void HttpClient_WinHttp::CancelAllRequests() // Wait for all destructors to run, signaled from erase() rather than // polled -- unlike a sleep-and-recheck loop, this drains the common case - // in microseconds and never busy-spins. + // in microseconds and never busy-spins. A positive timeout is the bounded, + // best-effort path used during pause; zero is the full shutdown barrier. std::unique_lock lock(m_requestsMutex); - m_requestsCv.wait(lock, [this]() noexcept -> bool { - return m_requests.empty(); - }); + if (bestEffortTimeout > std::chrono::milliseconds::zero()) + { + m_requestsCv.wait_for(lock, bestEffortTimeout, [this]() noexcept -> bool { + return m_requests.empty(); + }); + } + else + { + m_requestsCv.wait(lock, [this]() noexcept -> bool { + return m_requests.empty(); + }); + } } /// @@ -685,7 +700,7 @@ void HttpClient_WinHttp::SetMsRootCheck(bool enforceMsRoot) } /// -/// Determines whether MS-Roted server cert check required. +/// Determines whether MS-Rooted server cert check required. /// /// /// true if [MS-Rooted server cert check required]; otherwise, false. diff --git a/lib/http/HttpClient_WinHttp.hpp b/lib/http/HttpClient_WinHttp.hpp index d9255ae87..b7d1e2990 100644 --- a/lib/http/HttpClient_WinHttp.hpp +++ b/lib/http/HttpClient_WinHttp.hpp @@ -8,6 +8,7 @@ #ifdef HAVE_MAT_DEFAULT_HTTP_CLIENT #include "IHttpClient.hpp" +#include "IBoundedHttpClientCancel.hpp" #include "pal/PAL.hpp" #include "ILogManager.hpp" @@ -31,7 +32,7 @@ class WinHttpRequestWrapper; // This is the default Win32 desktop transport; HttpClient_WinInet remains // available as an explicit opt-in for callers that need IE-integrated proxy // or cookie behavior. -class HttpClient_WinHttp : public IHttpClient { +class HttpClient_WinHttp : public IHttpClient, public IBoundedHttpClientCancel { public: // Common IHttpClient methods HttpClient_WinHttp(); @@ -40,6 +41,7 @@ class HttpClient_WinHttp : public IHttpClient { virtual void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) final; virtual void CancelRequestAsync(std::string const& id) final; virtual void CancelAllRequests() final; + virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) final; virtual void ApplySettings(ILogConfiguration& config) override; diff --git a/lib/http/IBoundedHttpClientCancel.hpp b/lib/http/IBoundedHttpClientCancel.hpp new file mode 100644 index 000000000..53640c894 --- /dev/null +++ b/lib/http/IBoundedHttpClientCancel.hpp @@ -0,0 +1,23 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +#pragma once + +#include "ctmacros.hpp" + +#include + +namespace MAT_NS_BEGIN { + +class IBoundedHttpClientCancel +{ +public: + virtual ~IBoundedHttpClientCancel() noexcept = default; + + // A positive timeout is best-effort; zero requires a full drain. + virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) = 0; +}; + +} MAT_NS_END From 2a1823c8f6aa4f51b51cb6d881fd254140bf8b93 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 11:11:10 -0500 Subject: [PATCH 46/50] Align bounded cancellation integration Keep the shared interface and Visual Studio project ready for the upcoming PR 1494 merge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/http/IBoundedHttpClientCancel.hpp | 3 ++- lib/pal/desktop/desktop.vcxitems | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/http/IBoundedHttpClientCancel.hpp b/lib/http/IBoundedHttpClientCancel.hpp index 53640c894..f832e4678 100644 --- a/lib/http/IBoundedHttpClientCancel.hpp +++ b/lib/http/IBoundedHttpClientCancel.hpp @@ -16,7 +16,8 @@ class IBoundedHttpClientCancel public: virtual ~IBoundedHttpClientCancel() noexcept = default; - // A positive timeout is best-effort; zero requires a full drain. + // Positive timeout is a best-effort cap. Zero means the caller requires a + // full drain, matching IHttpClient::CancelAllRequests(). virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) = 0; }; diff --git a/lib/pal/desktop/desktop.vcxitems b/lib/pal/desktop/desktop.vcxitems index 5679a6258..e827b6299 100644 --- a/lib/pal/desktop/desktop.vcxitems +++ b/lib/pal/desktop/desktop.vcxitems @@ -15,6 +15,7 @@ + From 4c8d94c8f522c86939b497e4a4a8276c7518b519 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 11:12:00 -0500 Subject: [PATCH 47/50] Prepare request draining for PR 1494 Port bounded pause cancellation and condition-variable callback draining so WinHTTP can use the upcoming manager contract without a merge conflict. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/http/HttpClientManager.cpp | 78 ++++++++++++++++++++++++++++------ lib/http/HttpClientManager.hpp | 10 +++-- lib/system/TelemetrySystem.cpp | 5 ++- 3 files changed, 75 insertions(+), 18 deletions(-) diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index 0de14e085..118a18a74 100644 --- a/lib/http/HttpClientManager.cpp +++ b/lib/http/HttpClientManager.cpp @@ -4,6 +4,7 @@ // #include "HttpClientManager.hpp" +#include "IBoundedHttpClientCancel.hpp" #include "utils/StringUtils.hpp" #include "pal/TaskDispatcher.hpp" @@ -11,6 +12,7 @@ #include #include #include +#include #ifdef linux #include @@ -137,34 +139,84 @@ namespace MAT_NS_BEGIN { LOG_TRACE("HTTP remove callback=%p", callback); m_httpCallbacks.remove(callback); + m_httpCallbacksCV.notify_all(); } delete callback; } - bool HttpClientManager::cancelAllRequestsAsync() + void HttpClientManager::cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout) { + if (bestEffortTimeout > std::chrono::milliseconds::zero()) + { +#if defined(_CPPRTTI) || defined(__GXX_RTTI) + auto boundedCancel = dynamic_cast(&m_httpClient); + if (boundedCancel != nullptr) + { + boundedCancel->CancelAllRequests(bestEffortTimeout); + return; + } +#endif + + cancelTrackedRequestsAsync(); + return; + } + m_httpClient.CancelAllRequests(); - return true; } - void HttpClientManager::cancelAllRequests() + void HttpClientManager::cancelTrackedRequestsAsync() { - cancelAllRequestsAsync(); - - // Wait for callbacks to drain before shutdown can destroy state that - // those callbacks still use. Keep the list check synchronized and sleep - // between polls so a slow adapter does not burn CPU while draining. - for (;;) + std::vector requestIds; { + LOCKGUARD(m_httpCallbacksMtx); + for (const auto& callback : m_httpCallbacks) { - LOCKGUARD(m_httpCallbacksMtx); - if (m_httpCallbacks.empty()) + if (callback == nullptr || callback->m_ctx == nullptr) + { + continue; + } + + std::string id = callback->m_ctx->httpRequestId; + if (id.empty() && callback->m_ctx->httpRequest != nullptr) + { + id = callback->m_ctx->httpRequest->GetId(); + } + if (!id.empty()) { - return; + requestIds.push_back(id); } } - std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + + for (const auto& id : requestIds) + { + m_httpClient.CancelRequestAsync(id); + } + } + + void HttpClientManager::cancelAllRequests(bool bestEffort) + { + const auto cancelStart = std::chrono::steady_clock::now(); + cancelAllRequestsAsync(bestEffort ? m_cancelDrainTimeout : std::chrono::milliseconds::zero()); + + std::unique_lock lock(m_httpCallbacksMtx); + if (bestEffort) + { + const auto elapsed = std::chrono::duration_cast( + std::chrono::steady_clock::now() - cancelStart); + const auto remaining = (elapsed < m_cancelDrainTimeout) + ? (m_cancelDrainTimeout - elapsed) : std::chrono::milliseconds::zero(); + if (!m_httpCallbacksCV.wait_for(lock, remaining, + [this] { return m_httpCallbacks.empty(); })) + { + LOG_WARN("cancelAllRequests: %zu callback(s) still draining after %lld ms (best-effort)", + m_httpCallbacks.size(), static_cast(m_cancelDrainTimeout.count())); + } + } + else + { + m_httpCallbacksCV.wait(lock, [this] { return m_httpCallbacks.empty(); }); } } diff --git a/lib/http/HttpClientManager.hpp b/lib/http/HttpClientManager.hpp index e8214d631..d60f8c164 100644 --- a/lib/http/HttpClientManager.hpp +++ b/lib/http/HttpClientManager.hpp @@ -12,6 +12,8 @@ #include #include +#include +#include namespace MAT_NS_BEGIN { @@ -28,7 +30,7 @@ class HttpClientManager virtual ~HttpClientManager() noexcept; - void cancelAllRequests(); + void cancelAllRequests(bool bestEffort = false); size_t requestCount() const { @@ -55,14 +57,16 @@ class HttpClientManager void handleSendRequest(EventsUploadContextPtr const& ctx); virtual void scheduleOnHttpResponse(HttpCallback* callback); void onHttpResponse(HttpCallback* callback); - bool cancelAllRequestsAsync(); + void cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout = std::chrono::milliseconds::zero()); + void cancelTrackedRequestsAsync(); ILogManager& m_logManager; IHttpClient& m_httpClient; ITaskDispatcher& m_taskDispatcher; mutable std::recursive_mutex m_httpCallbacksMtx; std::list m_httpCallbacks; + std::condition_variable_any m_httpCallbacksCV; + std::chrono::milliseconds m_cancelDrainTimeout{std::chrono::seconds(30)}; }; } MAT_NS_END - diff --git a/lib/system/TelemetrySystem.cpp b/lib/system/TelemetrySystem.cpp index 24ad34ba9..46a8cce4b 100644 --- a/lib/system/TelemetrySystem.cpp +++ b/lib/system/TelemetrySystem.cpp @@ -141,7 +141,9 @@ namespace MAT_NS_BEGIN { { bool result = true; result &= tpm.pause(); - hcm.cancelAllRequests(); + // Pause runs under the LogManager lock and must not block + // indefinitely if a callback is slow to drain. + hcm.cancelAllRequests(/* bestEffort */ true); return result; }; @@ -248,4 +250,3 @@ namespace MAT_NS_BEGIN { } } MAT_NS_END - From 8b38be117490789bb878b5af6c2accfbcc437539 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 20:35:33 -0500 Subject: [PATCH 48/50] Fix teardown deadlock when flush is skipped during pause OfflineStorageHandler::Flush() returned early when StartActivity() failed, which happens as soon as FlushAndTeardown() begins pausing the LogManager. That early return left m_flushPending == true and never posted m_flushComplete, so WaitForFlush() blocked forever and Shutdown() never completed. The race needs a flush to be pending when teardown starts, so it only reproduced when an earlier test had already pushed enough records to schedule an async flush -- which is why sendManyRequestsAndCancel hung in the full suite but passed in isolation. It was misread as WinHTTP cancellation not draining; the transport had already finished. Always release the waiters: cancel the pending handle, post the event, and clear the pending flag on the skipped path. Flush body moves to FlushImpl() so EndActivity() is paired with StartActivity() on exactly the path that acquired it. Verified on Windows: the doNothing/killIsTemporary/ sendManyRequestsAndCancel sequence that hung indefinitely now passes, 5/5 repeat runs are stable, functests are 43/43 and unittests 528/528. Files changed: lib/offline/OfflineStorageHandler.cpp lib/offline/OfflineStorageHandler.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/offline/OfflineStorageHandler.cpp | 21 ++++++++++++++++++++- lib/offline/OfflineStorageHandler.hpp | 2 ++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/offline/OfflineStorageHandler.cpp b/lib/offline/OfflineStorageHandler.cpp index 52ce15515..559c8f977 100644 --- a/lib/offline/OfflineStorageHandler.cpp +++ b/lib/offline/OfflineStorageHandler.cpp @@ -161,11 +161,31 @@ namespace MAT_NS_BEGIN { return count; } + void OfflineStorageHandler::SignalFlushComplete() + { + LOCKGUARD(m_flushLock); + m_flushHandle.Cancel(); + m_flushComplete.post(); + m_flushPending = false; + } + void OfflineStorageHandler::Flush() { + // StartActivity() only keeps the LogManager alive for the duration of an + // asynchronously scheduled flush; it fails once teardown has begun pausing. + // Returning here without signalling would strand every thread blocked in + // WaitForFlush(): m_flushPending stays true and m_flushComplete is never + // posted, so Shutdown() waits on it forever. Always release the waiters. if (!m_logManager.StartActivity()) { + SignalFlushComplete(); return; } + FlushImpl(); + m_logManager.EndActivity(); + } + + void OfflineStorageHandler::FlushImpl() + { // Flush could be executed from context of worker thread, as well as from TPM and // after HTTP callback. Make sure it is atomic / thread-safe. LOCKGUARD(m_flushLock); @@ -221,7 +241,6 @@ namespace MAT_NS_BEGIN { // Flush is done, notify the waiters m_flushComplete.post(); m_flushPending = false; - m_logManager.EndActivity(); } bool OfflineStorageHandler::StoreRecord(StorageRecord const& record) diff --git a/lib/offline/OfflineStorageHandler.hpp b/lib/offline/OfflineStorageHandler.hpp index 9a1131aff..1e4aefaa4 100644 --- a/lib/offline/OfflineStorageHandler.hpp +++ b/lib/offline/OfflineStorageHandler.hpp @@ -100,6 +100,8 @@ namespace MAT_NS_BEGIN { private: void WaitForFlush(); + void FlushImpl(); + void SignalFlushComplete(); }; From 8d3b67a40d14e49cf3238c6c077aca436a0ee9a5 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 22:27:51 -0500 Subject: [PATCH 49/50] Fix process-terminating fastfail in oneds_memcpy_s on MSVC oneds_memcpy_s delegated straight to the CRT memcpy_s whenever _MSC_VER or __STDC_LIB_EXT1__ was defined, skipping its own constraint checks. On MSVC the CRT reports a constraint violation through the invalid parameter handler, whose default behaviour terminates the process via __fastfail (STATUS_STACK_BUFFER_OVERRUN / 0xC0000409) rather than returning EINVAL. This crashed AnnexKTests.memcpy_s in Debug builds, which Windows CI does run (test-win-latest.yml builds both Release and Debug). More importantly it was a latent abrupt-termination path in shipped Windows code: any caller passing count > destsz would kill the process instead of getting an error back. The delegate path also never zeroed the destination on error, contradicting the function's documented contract. Validate the arguments before copying on every platform so the documented "return EINVAL and zero the destination" behaviour holds uniformly. Also fix oneds_buffer_region_overlap, which used strict > against a one-past-the-last-byte address and so both missed genuine single-byte overlaps and mis-flagged merely adjacent buffers. Replaced with the standard half-open range test, with an explicit zero-length short circuit. Unit tests: 531/531 pass with no exclusions (previously the suite could not run AnnexKTests at all). Files changed: lib/utils/annex_k.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/utils/annex_k.hpp | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/lib/utils/annex_k.hpp b/lib/utils/annex_k.hpp index 5aa4b73af..cfb6f6ba6 100644 --- a/lib/utils/annex_k.hpp +++ b/lib/utils/annex_k.hpp @@ -47,21 +47,13 @@ class BoundCheckFunctions private: static bool oneds_buffer_region_overlap(const char *buffer1, size_t buffer1_len, const char *buffer2, size_t buffer2_len) noexcept { - if (buffer2 >= buffer1) + // Two half-open ranges [b1, b1+len1) and [b2, b2+len2) overlap iff each + // starts before the other ends. Empty ranges never overlap. + if (buffer1_len == 0 || buffer2_len == 0) { - if (buffer1 + buffer1_len - 1 > buffer2 ) - { - return true; - } + return false; } - else - { - if (buffer2 + buffer2_len - 1 > buffer1) - { - return true; - } - } - return false; + return (buffer1 < buffer2 + buffer2_len) && (buffer2 < buffer1 + buffer1_len); } public: @@ -147,12 +139,16 @@ static errno_t oneds_strncpy_s(char * restrict dest, rsize_t destsz, const char // In case of error, the entire destination range [dest, dest+destsz) is zeroed out // (if both dest and destsz are valid)) +// +// NOTE: the constraint checks below are performed here rather than delegated to +// the platform's Annex K / CRT memcpy_s. On MSVC the CRT memcpy_s reports a +// constraint violation through the invalid parameter handler, whose default +// behaviour terminates the process (__fastfail / STATUS_STACK_BUFFER_OVERRUN) +// instead of returning EINVAL. Validating first keeps the documented +// "return EINVAL and zero the destination" contract on every platform. static errno_t oneds_memcpy_s( void *restrict dest, rsize_t destsz, const void *restrict src, rsize_t count ) noexcept { -#if (defined __STDC_LIB_EXT1__) || ( defined _MSC_VER) - return memcpy_s(dest, destsz, src, count); -#else if (dest == NULL) { return EINVAL; @@ -176,13 +172,8 @@ static errno_t oneds_memcpy_s( void *restrict dest, rsize_t destsz, memset(dest, 0, destsz); return EINVAL; } - void *result = memcpy(dest, src, count); - if (result == (void *)NULL) - { - return -1; - } + memcpy(dest, src, count); return 0; -#endif } }; } From 010429403ad805c19c9585d9f1518b51e43048c2 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 8 Aug 2026 23:30:24 -0500 Subject: [PATCH 50/50] Select curl HTTP version at runtime instead of forcing HTTP/2 CurlHttpOperation unconditionally set CURLOPT_HTTP_VERSION to CURL_HTTP_VERSION_2_0 with a comment claiming it would "fallback to HTTP/1.1 if not supported". libcurl does not do that: when the linked library was built without HTTP/2, requesting it fails the transfer with CURLE_UNSUPPORTED_PROTOCOL rather than negotiating down. On such a build every upload would fail. Add CurlHttpOperation::GetPreferredHttpVersion(), which probes curl_version_info for CURL_VERSION_HTTP2 and returns CURL_HTTP_VERSION_1_1 when HTTP/2 is unavailable, and use it at setopt time. This also fixes the Linux build. HttpClientCurlTests.cpp came in with the #1481 merge and calls GetPreferredHttpVersion(), which had no implementation, so UnitTests failed to compile and build-tests.sh then exited 127 on the missing binary in all three ubuntu legs. Files changed: lib/http/HttpClient_Curl.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 --- lib/http/HttpClient_Curl.hpp | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index f8dfb952e..461dd01f7 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -94,6 +94,21 @@ class CurlHttpOperation { * @param httpConnTimeout HTTP connection timeout in seconds * @param httpReadTimeout HTTP read timeout in seconds */ + // Selects HTTP/2 only when the libcurl we are actually linked against was + // built with HTTP/2 support. Setting CURLOPT_HTTP_VERSION to + // CURL_HTTP_VERSION_2_0 against a libcurl without HTTP/2 does not silently + // downgrade -- it fails the transfer with CURLE_UNSUPPORTED_PROTOCOL -- so + // the version has to be probed at runtime rather than assumed. + static long GetPreferredHttpVersion() noexcept + { + const curl_version_info_data* versionInfo = curl_version_info(CURLVERSION_NOW); + if (versionInfo != nullptr && (versionInfo->features & CURL_VERSION_HTTP2) != 0) + { + return CURL_HTTP_VERSION_2_0; + } + return CURL_HTTP_VERSION_1_1; + } + CurlHttpOperation( std::string method, std::string url, @@ -152,8 +167,8 @@ class CurlHttpOperation { if (!m_sslCaInfo.empty()) { curl_easy_setopt(curl, CURLOPT_CAINFO, m_sslCaInfo.c_str()); } - // HTTP/2 please, fallback to HTTP/1.1 if not supported - curl_easy_setopt(curl, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0); + // HTTP/2 when the linked libcurl supports it, otherwise HTTP/1.1 + curl_easy_setopt(curl, CURLOPT_HTTP_VERSION, GetPreferredHttpVersion()); // Headers are copied into m_headersChunk during construction and the // curl_slist is kept alive until destruction, so the original map does