diff --git a/cmake/MatsdkOptions.cmake b/cmake/MatsdkOptions.cmake index a56e468c6..3cb213ef8 100644 --- a/cmake/MatsdkOptions.cmake +++ b/cmake/MatsdkOptions.cmake @@ -42,6 +42,8 @@ option(MATSDK_BUILD_AZMON "Build Azure Monitor / Application Insights support" ON) option(MATSDK_BUILD_APPLE_HTTP "Build the Apple-native HTTP client" "${APPLE}") +option(MATSDK_USE_WININET + "Use WinInet instead of WinHTTP as the Win32 desktop HTTP client" OFF) set(_matsdk_android_http_client_predefined OFF) if(DEFINED MATSDK_ANDROID_HTTP_CLIENT) diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index b08fd4537..b48b04d81 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -299,9 +299,22 @@ target_compile_definitions(matsdk_internal_config INTERFACE _USRDLL WINVER=_WIN32_WINNT_WIN7) target_compile_options(matsdk_internal_config INTERFACE /U_MBCS) +if(MATSDK_USE_WININET) + target_compile_definitions(matsdk_internal_config INTERFACE HAVE_MAT_WININET_HTTP_CLIENT) +endif() + if(MATSDK_USE_WININET) + list(APPEND SRCS + http/HttpClient_WinInet.cpp + http/HttpClient_WinInet.hpp + ) + else() + list(APPEND SRCS + http/HttpClient_WinHttp.cpp + http/HttpClient_WinHttp.hpp + http/IBoundedHttpClientCancel.hpp + ) + endif() list(APPEND SRCS - http/HttpClient_WinInet.cpp - http/HttpClient_WinInet.hpp pal/desktop/WindowsDesktopDeviceInformationImpl.cpp pal/desktop/WindowsDesktopNetworkInformationImpl.cpp pal/desktop/WindowsDesktopSystemInformationImpl.cpp @@ -666,7 +679,12 @@ 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) + if(MATSDK_USE_WININET) + target_link_libraries(mat PRIVATE wininet) + else() + target_link_libraries(mat PRIVATE winhttp) + endif() + target_link_libraries(mat PRIVATE crypt32) 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_Curl.cpp b/lib/http/HttpClient_Curl.cpp index b910cdf28..3db7f3127 100644 --- a/lib/http/HttpClient_Curl.cpp +++ b/lib/http/HttpClient_Curl.cpp @@ -83,16 +83,14 @@ 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. - curlOperation->SendAsync([this, callback, requestId](CurlHttpOperation& operation) { - this->EraseRequest(requestId); + curlOperation->SendAsync([this, callback, requestId](CurlHttpOperation& operation) { + EraseRequest(requestId); auto response = std::unique_ptr(new SimpleHttpResponse(requestId)); response->m_result = HttpResult_OK; response->m_statusCode = operation.GetResponseCode(); - if (response->m_statusCode == CURLE_FAILED_INIT) { + if (operation.HasOptionFailure() || response->m_statusCode == CURLE_FAILED_INIT) { // There was an error in CURL stack while trying to create request response->m_result = HttpResult_LocalFailure; } else if ((CURLE_OK < response->m_statusCode) && (response->m_statusCode <= CURL_LAST)) { @@ -161,4 +159,3 @@ namespace MAT_NS_BEGIN { } MAT_NS_END #endif - diff --git a/lib/http/HttpClient_Curl.hpp b/lib/http/HttpClient_Curl.hpp index 7d599dec9..d6fd22a36 100644 --- a/lib/http/HttpClient_Curl.hpp +++ b/lib/http/HttpClient_Curl.hpp @@ -17,11 +17,15 @@ #include #include #include +#include #include #include -#include +#include #include +#include +#include +#include #include #include @@ -71,13 +75,6 @@ class HttpClient_Curl : public IHttpClient { class CurlHttpOperation { public: - static long GetPreferredHttpVersion() - { - const curl_version_info_data* versionInfo = curl_version_info(CURLVERSION_NOW); - return (versionInfo != nullptr && (versionInfo->features & CURL_VERSION_HTTP2) != 0) - ? CURL_HTTP_VERSION_2_0 - : CURL_HTTP_VERSION_1_1; - } void DispatchEvent(HttpStateEvent type) { @@ -88,6 +85,7 @@ class CurlHttpOperation { } std::atomic isAborted { false }; // Set to 'true' when async callback is aborted + bool m_optionFailure { false }; /** * Create local CURL instance for url and body @@ -97,13 +95,29 @@ 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, 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 stored by reference; + // CurlHttpRequest destroys this operation (which joins the worker) before + // destroying its inherited request-body storage. const std::map& requestHeaders, const std::vector& requestBody, // Default connectivity and response size options @@ -139,33 +153,13 @@ class CurlHttpOperation { return; } -#if 0 - // Be verbose - if (!SetOption(CURLOPT_VERBOSE, 1L)) -#else - if (!SetOption(CURLOPT_VERBOSE, 0L)) -#endif - { - DispatchEvent(OnCreateFailed); - return; - } - - // Specify target URL - if (!SetOption(CURLOPT_URL, m_url.c_str()) - || !SetOption(CURLOPT_SSL_VERIFYPEER, sslVerify ? 1L : 0L) - || !SetOption(CURLOPT_SSL_VERIFYHOST, sslVerify ? 2L : 0L)) - { - DispatchEvent(OnCreateFailed); - return; - } - - if (!m_sslCaInfo.empty() && !SetOption(CURLOPT_CAINFO, m_sslCaInfo.c_str())) - { - DispatchEvent(OnCreateFailed); - return; - } - - if (!SetOption(CURLOPT_HTTP_VERSION, GetPreferredHttpVersion())) + if (!SetOption(CURLOPT_VERBOSE, 0L) || + !SetOption(CURLOPT_URL, m_url.c_str()) || + !SetOption(CURLOPT_SSL_VERIFYPEER, sslVerify ? 1L : 0L) || + !SetOption(CURLOPT_SSL_VERIFYHOST, sslVerify ? 2L : 0L) || + (!m_sslCaInfo.empty() && !SetOption(CURLOPT_CAINFO, m_sslCaInfo.c_str())) || + // HTTP/2 when the linked libcurl supports it, otherwise HTTP/1.1 + !SetOption(CURLOPT_HTTP_VERSION, GetPreferredHttpVersion())) { DispatchEvent(OnCreateFailed); return; @@ -177,24 +171,24 @@ class CurlHttpOperation { for (const auto& kv : requestHeaders) { std::string header = kv.first + ": " + kv.second; - curl_slist* appended = curl_slist_append(m_headersChunk, header.c_str()); - if (appended == nullptr) + curl_slist* appendedHeaders = curl_slist_append(m_headersChunk, header.c_str()); + if (appendedHeaders == nullptr) { res = CURLE_OUT_OF_MEMORY; + m_optionFailure = true; DispatchEvent(OnCreateFailed); return; } - m_headersChunk = appended; + m_headersChunk = appendedHeaders; } - if(m_headersChunk != nullptr && !SetOption(CURLOPT_HTTPHEADER, m_headersChunk)) + if (m_headersChunk != nullptr && !SetOption(CURLOPT_HTTPHEADER, m_headersChunk)) { DispatchEvent(OnCreateFailed); return; } TRACE("method=%s, url=%s\n", this->m_method.c_str(), this->m_url.c_str()); - m_isConfigured = true; DispatchEvent(OnCreated); } @@ -203,19 +197,31 @@ class CurlHttpOperation { */ virtual ~CurlHttpOperation() { - // Given the request has not been aborted we should wait for completion here - // This guarantees the lifetime of this request. - if (result.valid()) + if (m_worker.joinable()) { - result.wait(); + 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(); + } } - DispatchEvent(OnDestroy); + + DispatchDestroyEvent(); res = CURLE_OK; if (curl != nullptr) { curl_easy_cleanup(curl); } - curl_slist_free_all(m_headersChunk); + if (m_headersChunk != nullptr) + { + curl_slist_free_all(m_headersChunk); + } ReleaseResponse(); } @@ -230,14 +236,17 @@ class CurlHttpOperation { // Request buffer const void *request = requestBody.empty() ? nullptr : requestBody.data(); const size_t reqSize = requestBody.size(); - int socketWaitResult = 0; + long httpStatusCode = 0; + CURLcode infoResult = CURLE_OK; - if(!curl || !m_isConfigured) + if(!curl) + { + res = CURLE_FAILED_INIT; + DispatchEvent(OnSendFailed); + goto cleanup; + } + if (m_optionFailure) { - if (res == CURLE_OK) - { - res = CURLE_FAILED_INIT; - } DispatchEvent(OnSendFailed); goto cleanup; } @@ -252,43 +261,35 @@ class CurlHttpOperation { goto cleanup; } DispatchEvent(OnConnecting); + res = curl_easy_perform(curl); + if(CURLE_OK != res) { - const CURLcode curlResult = curl_easy_perform(curl); - res = static_cast(curlResult); - if(CURLE_OK != curlResult) - { - DispatchEvent(OnConnectFailed); // couldn't connect - stage 1 - TRACE("Error #1: %s\n", curl_easy_strerror(curlResult)); - goto cleanup; - } + DispatchEvent(OnConnectFailed); // couldn't connect - stage 1 + TRACE("Error #1: %s\n", curl_easy_strerror(res)); + goto cleanup; } - { - CURLcode infoResult; + /* Extract the socket from the curl handle - we'll need it for waiting. + * Note that this API takes a pointer to a 'long' while we use + * curl_socket_t for sockets otherwise. + */ + #if LIBCURL_VERSION_NUM >= 0x072D00 // Version 7.45.00 - infoResult = curl_easy_getinfo(curl, CURLINFO_ACTIVESOCKET, &sockextr); + res = curl_easy_getinfo(curl, CURLINFO_ACTIVESOCKET, &sockextr); #else - long lastSocket = -1; - infoResult = curl_easy_getinfo(curl, CURLINFO_LASTSOCKET, &lastSocket); - if (infoResult == CURLE_OK) - { - sockextr = static_cast(lastSocket); - } + res = curl_easy_getinfo(curl, CURLINFO_LASTSOCKET, &sockextr); #endif - if(CURLE_OK != infoResult || sockextr == CURL_SOCKET_BAD) - { - res = static_cast( - infoResult != CURLE_OK ? infoResult : CURLE_COULDNT_CONNECT); - DispatchEvent(OnConnectFailed); // couldn't connect - stage 2 - TRACE("Error #2: %s\n", curl_easy_strerror(static_cast(res))); - goto cleanup; - } + + if(CURLE_OK != res) + { + DispatchEvent(OnConnectFailed); // couldn't connect - stage 2 + TRACE("Error #2: %s\n", curl_easy_strerror(res)); + goto cleanup; } /* wait for the socket to become ready for sending */ sockfd = sockextr; - socketWaitResult = WaitOnSocket(sockfd, 0, HTTP_CONN_TIMEOUT * 1000L); - if(socketWaitResult <= 0 || isAborted) + if( !WaitOnSocket(sockfd, 0, HTTP_CONN_TIMEOUT * 1000L) || isAborted) { TRACE("Error #3: timeout, aborted=%u\n", isAborted.load() ); res = CURLE_OPERATION_TIMEDOUT; @@ -306,33 +307,31 @@ class CurlHttpOperation { // send all data to our callback function if (rawResponse) { - if (!SetOption(CURLOPT_HEADER, 1L) - || !SetOption(CURLOPT_WRITEFUNCTION, - static_cast(&WriteMemoryCallback)) - || !SetOption(CURLOPT_WRITEDATA, static_cast(&response))) + if (!SetOption(CURLOPT_HEADER, 1L) || + !SetOption(CURLOPT_WRITEFUNCTION, &WriteMemoryCallback) || + !SetOption(CURLOPT_WRITEDATA, static_cast(&response))) + { + DispatchEvent(OnSendFailed); + goto cleanup; + } + } else { + if (!SetOption(CURLOPT_HEADERFUNCTION, &WriteVectorCallback) || + !SetOption(CURLOPT_HEADERDATA, static_cast(&respHeaders)) || + !SetOption(CURLOPT_WRITEFUNCTION, &WriteVectorCallback) || + !SetOption(CURLOPT_WRITEDATA, static_cast(&respBody))) { DispatchEvent(OnSendFailed); goto cleanup; } - } - else if (!SetOption(CURLOPT_WRITEFUNCTION, - static_cast(&WriteVectorCallback)) - || !SetOption(CURLOPT_HEADERFUNCTION, - static_cast(&WriteVectorCallback)) - || !SetOption(CURLOPT_HEADERDATA, static_cast(&respHeaders)) - || !SetOption(CURLOPT_WRITEDATA, static_cast(&respBody))) - { - DispatchEvent(OnSendFailed); - goto cleanup; } // TODO: only two methods supported for now - POST and GET if (m_method.compare("POST") == 0) { // POST - if (!SetOption(CURLOPT_POST, 1L) - || !SetOption(CURLOPT_POSTFIELDS, static_cast(request)) - || !SetOption(CURLOPT_POSTFIELDSIZE_LARGE, static_cast(reqSize))) + if (!SetOption(CURLOPT_POST, 1L) || + !SetOption(CURLOPT_POSTFIELDS, static_cast(request)) || + !SetOption(CURLOPT_POSTFIELDSIZE_LARGE, static_cast(reqSize))) { DispatchEvent(OnSendFailed); goto cleanup; @@ -348,22 +347,19 @@ class CurlHttpOperation { goto cleanup; } - if (!SetOption(CURLOPT_LOW_SPEED_TIME, 30L) - || !SetOption(CURLOPT_LOW_SPEED_LIMIT, 4096L)) + if (!SetOption(CURLOPT_LOW_SPEED_TIME, 30L) || + !SetOption(CURLOPT_LOW_SPEED_LIMIT, 4096L)) { DispatchEvent(OnSendFailed); goto cleanup; } DispatchEvent(OnSending); + res = curl_easy_perform(curl); + if(CURLE_OK != res) { - const CURLcode curlResult = curl_easy_perform(curl); - res = static_cast(curlResult); - if(CURLE_OK != curlResult) - { - DispatchEvent(OnSendFailed); - TRACE("Error: %s\n", curl_easy_strerror(curlResult)); - goto cleanup; - } + DispatchEvent(OnSendFailed); + TRACE("Error: %s\n", curl_easy_strerror(res)); + goto cleanup; } /* Code snippet to parse raw HTTP response. This might come in handy @@ -378,19 +374,17 @@ class CurlHttpOperation { */ /* libcurl is nice enough to parse the response code itself: */ + infoResult = curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &httpStatusCode); + if (infoResult != CURLE_OK) { - long responseCode = 0; - const CURLcode infoResult = curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &responseCode); - if (infoResult != CURLE_OK) - { - res = static_cast(infoResult); - DispatchEvent(OnSendFailed); - goto cleanup; - } - res = responseCode; + res = infoResult; + DispatchEvent(OnSendFailed); + TRACE("Error getting HTTP response code: %s\n", curl_easy_strerror(res)); + goto cleanup; } + res = static_cast(httpStatusCode); // We got some response from server. Dump the contents. - TRACE("HTTP response code %d\n", res); + TRACE("HTTP response code %ld\n", httpStatusCode); DispatchEvent(OnResponse); cleanup: @@ -402,14 +396,46 @@ class CurlHttpOperation { return res; } - std::future & SendAsync(std::function callback = nullptr) { - result = std::async(std::launch::async, [this, callback] { - long result = Send(); - if (callback!=nullptr) - callback(*this); - return result; - }); - return result; + 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_sendAttempted) + { + throw std::logic_error("CurlHttpOperation is single-use"); + } + m_sendAttempted = true; + + try + { + 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 (...) + { + // Callable allocation/copy or std::thread creation failed. + } + } + + res = CURLE_FAILED_INIT; + Complete(callback); } /** @@ -428,6 +454,11 @@ class CurlHttpOperation { return isAborted.load(); } + bool HasOptionFailure() const + { + return m_optionFailure; + } + /** * Return a copy of response headers * @@ -521,19 +552,16 @@ class CurlHttpOperation { const size_t httpConnTimeout; // Timeout for connect. Default: 5s CURL *curl; // Local curl instance - long res = CURLE_OK; // Curl result OR HTTP status code if successful - + CURLcode res = CURLE_OK; // Curl result OR HTTP status code if successful + IHttpResponseCallback* m_callback = nullptr; // Request values std::string m_method; std::string m_url; std::string m_sslCaInfo; - bool m_isConfigured = false; - // 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. + // 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; @@ -544,26 +572,73 @@ class CurlHttpOperation { // Socket parameters curl_socket_t sockfd = 0; - curl_socket_t sockextr = CURL_SOCKET_BAD; + long sockextr = 0; curl_off_t nread = 0; size_t sendlen = 0; // # bytes sent by client size_t acklen = 0; // # bytes ack by server - std::future result; + std::mutex m_workerStartMtx; + bool m_sendAttempted = false; + std::thread m_worker; + std::atomic m_destroyEventDispatched { false }; - template - bool SetOption(CURLoption option, TValue value) + void DispatchDestroyEvent() noexcept { - const CURLcode optionResult = curl_easy_setopt(curl, option, value); - if (optionResult != CURLE_OK) + 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. + } + } + + template + bool SetOption(CURLoption option, T value) + { + if (curl == nullptr) { - res = static_cast(optionResult); - TRACE("curl_easy_setopt(%d) failed: %s\n", - static_cast(option), curl_easy_strerror(optionResult)); + res = CURLE_FAILED_INIT; + m_optionFailure = true; return false; } - return true; + + const CURLcode optionResult = curl_easy_setopt(curl, option, value); + if (optionResult == CURLE_OK) + { + return true; + } + + LOG_WARN("curl_easy_setopt(%d) failed: %s", static_cast(option), curl_easy_strerror(optionResult)); + res = optionResult; + m_optionFailure = true; + return false; } /** @@ -607,7 +682,7 @@ class CurlHttpOperation { * @param userp * @return */ - static size_t WriteMemoryCallback(char *contents, size_t size, size_t nmemb, void *userp) + static size_t WriteMemoryCallback(void *contents, size_t size, size_t nmemb, void *userp) { // Guard the size * nmemb product against size_t overflow before using it. if (nmemb != 0 && size > static_cast(-1) / nmemb) { @@ -651,15 +726,14 @@ class CurlHttpOperation { * @param data * @return */ - static size_t WriteVectorCallback(char *ptr, size_t size, size_t nmemb, void* userp) + static size_t WriteVectorCallback(void *ptr, size_t size, size_t nmemb, std::vector* data) { // Guard the size * nmemb product against size_t overflow before using it. if (nmemb != 0 && size > static_cast(-1) / nmemb) { return 0; } - size_t realsize = size * nmemb; - auto* data = static_cast*>(userp); if (data != nullptr) { + size_t realsize = size * nmemb; // SECURITY: bound the buffered response (see kMaxResponseBytes). Compare // overflow-safely (data->size() is always <= kMaxResponseBytes here). // Returning a short count aborts the transfer with CURLE_WRITE_ERROR. @@ -667,11 +741,11 @@ class CurlHttpOperation { TRACE("Response exceeds max buffered size (%zu bytes); aborting transfer\n", kMaxResponseBytes); return 0; } - const auto* begin = reinterpret_cast(ptr); + const auto* begin = static_cast(ptr); const auto* end = begin + realsize; data->insert( data->end(), begin, end); } - return realsize; + return size * nmemb; } }; diff --git a/lib/http/HttpClient_WinHttp.cpp b/lib/http/HttpClient_WinHttp.cpp new file mode 100644 index 000000000..8de676d65 --- /dev/null +++ b/lib/http/HttpClient_WinHttp.cpp @@ -0,0 +1,817 @@ +// +// 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 +#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: + 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; + std::atomic isCallbackCalled {false}; + bool isAborted {false}; + WinHttpCallbackContext* m_callbackContext {nullptr}; + + 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); + if (isCallbackCalled) + { + return; + } + isAborted = true; + hRequestToClose = m_hRequest; + m_hRequest = nullptr; + } + if (hRequestToClose != nullptr) + { + ::WinHttpCloseHandle(hRequestToClose); + // WinHttpCloseHandle waits for any callback to finish. Some + // cancellation paths report only HANDLE_CLOSING, so complete the + // request here if no callback delivered the terminal result. + if (!isCallbackCalled) + { + m_hRequest = nullptr; + onRequestComplete(ERROR_WINHTTP_OPERATION_CANCELLED); + } + } + } + + /// + /// 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(HINTERNET hRequest) + { + PCCERT_CONTEXT pCertContext = nullptr; + DWORD dwSize = sizeof(pCertContext); + if (!::WinHttpQueryOption(hRequest, WINHTTP_OPTION_SERVER_CERT_CONTEXT, &pCertContext, &dwSize)) + { + LOG_WARN("WinHttpQueryOption(SERVER_CERT_CONTEXT) failed: %d", ::GetLastError()); + return false; + } + + 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 + { + LOG_WARN("CertGetCertificateChain() failed: %d", ::GetLastError()); + result = false; + } + ::CertFreeCertificateContext(pCertContext); + return result; + } + + HINTERNET getRequestHandle() + { + std::lock_guard lock(m_parent.m_requestsMutex); + return m_hRequest; + } + + 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. + // + // Handle setup runs under m_parent.m_requestsMutex (a recursive_mutex, + // matching HttpClient_WinInet's model), exactly like cancel(): that + // serializes send() and cancel() completely, so cancel() can never + // interleave mid-way through handle creation and be silently lost. + // + // DEADLOCK NOTE: the lock must NOT still be held when a synchronous + // failure completes the request. onRequestComplete() invokes the + // application callback, which is documented (below) to be able to tear the + // client down synchronously -- that reaches CancelAllRequests(), which + // waits on m_requestsCv. condition_variable_any::wait() releases only ONE + // level of a recursive_mutex, so waiting with the mutex held twice leaves + // it locked: erase() on the WinHTTP callback thread can then never acquire + // it to notify, and the wait never wakes. So sendLocked() only reports the + // failure, and send() completes it after the lock is released. + void send(IHttpResponseCallback* callback) + { + bool failed = false; + DWORD dwError = ERROR_SUCCESS; + { + std::lock_guard lock(m_parent.m_requestsMutex); + failed = !sendLocked(callback, dwError); + } + if (failed) + { + onRequestComplete(dwError); + } + } + + // Returns true if the request was handed off to WinHTTP asynchronously. + // Returns false on synchronous failure, setting dwError to the result the + // caller must complete the request with (once the lock has been dropped). + bool sendLocked(IHttpResponseCallback* callback, DWORD& dwErrorOut) + { + m_appCallback = callback; + m_parent.m_requests[m_id] = shared_from_this(); + + if (isAborted) + { + // Request force-aborted before creating a WinHTTP handle. + DispatchEvent(OnConnectFailed); + dwErrorOut = ERROR_WINHTTP_OPERATION_CANCELLED; + return false; + } + + 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); + dwErrorOut = ERROR_WINHTTP_OPERATION_CANCELLED; + return false; + } + + // 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); + dwErrorOut = ERROR_WINHTTP_OPERATION_CANCELLED; + return false; + } + + 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); + dwErrorOut = ERROR_WINHTTP_OPERATION_CANCELLED; + return false; + } + + // 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. + + // 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 | WINHTTP_CALLBACK_FLAG_HANDLES, 0) == WINHTTP_INVALID_STATUS_CALLBACK) + { + DWORD dwError = ::GetLastError(); + LOG_WARN("WinHttpSetStatusCallback() failed: %d", dwError); + DispatchEvent(OnConnectFailed); + dwErrorOut = ERROR_WINHTTP_OPERATION_CANCELLED; + return false; + } + + 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() && + wHeaders.size() > static_cast(std::numeric_limits::max())) + { + LOG_WARN("Request headers exceed WinHTTP's maximum size"); + DispatchEvent(OnConnectFailed); + dwErrorOut = ERROR_INVALID_PARAMETER; + return false; + } + 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); + dwErrorOut = ERROR_WINHTTP_OPERATION_CANCELLED; + return false; + } + + // Try to send headers and request body to server + DispatchEvent(OnSending); + if (m_request->m_body.size() > static_cast(std::numeric_limits::max())) + { + LOG_WARN("Request body exceeds WinHTTP's maximum size"); + DispatchEvent(OnSendFailed); + dwErrorOut = ERROR_INVALID_PARAMETER; + return false; + } + void* data = m_request->m_body.empty() ? nullptr : static_cast(m_request->m_body.data()); + DWORD size = static_cast(m_request->m_body.size()); + 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(); + // WinHTTP retains the context on the request handle and can deliver + // HANDLE_CLOSING after this failure. Keep it alive until that callback. + LOG_WARN("WinHttpSendRequest() failed: %d", dwError); + // Unable to send request + DispatchEvent(OnSendFailed); + dwErrorOut = dwError; + return false; + } + // Async request has been queued; completion arrives via winHttpCallback. + return true; + } + + // 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); + + 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.get(), dwInternetStatus); + + switch (dwInternetStatus) + { + case WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE: + { + HINTERNET request = self->getRequestHandle(); + if (request != nullptr && !::WinHttpReceiveResponse(request, NULL)) + { + self->onRequestComplete(::GetLastError()); + } + return; + } + + case WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE: + { + HINTERNET request = self->getRequestHandle(); + if (request == nullptr) + { + return; + } + // TLS negotiation and response-header receipt are both complete here, + // so WINHTTP_OPTION_SERVER_CERT_CONTEXT is available for the + // configured Microsoft-root enforcement. + if (self->m_parent.IsMsRootCheckRequired() && !self->isMsRootCert(request)) + { + self->onRequestComplete(ERROR_WINHTTP_SECURE_INVALID_CERT); + return; + } + if (!::WinHttpQueryDataAvailable(request, 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; + } + HINTERNET request = self->getRequestHandle(); + if (request == nullptr) + { + return; + } + self->m_readBuffer.resize(bytesAvailable); + if (!::WinHttpReadData(request, 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). + if (dwStatusInformationLength > self->m_readBuffer.size()) + { + self->onRequestComplete(ERROR_WINHTTP_INVALID_SERVER_RESPONSE); + return; + } + self->m_bodyBuffer.insert(self->m_bodyBuffer.end(), + self->m_readBuffer.begin(), self->m_readBuffer.begin() + dwStatusInformationLength); + { + HINTERNET request = self->getRequestHandle(); + if (request == nullptr) + { + return; + } + if (!::WinHttpQueryDataAvailable(request, 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) + { + if (isCallbackCalled.exchange(true)) + { + return; + } + + std::unique_ptr response(new SimpleHttpResponse(m_id)); + HINTERNET request = getRequestHandle(); + if (dwError == ERROR_SUCCESS && request == nullptr) + { + dwError = ERROR_WINHTTP_OPERATION_CANCELLED; + } + + if (dwError == ERROR_SUCCESS) { + response->m_body = m_bodyBuffer; + response->m_result = HttpResult_OK; + + DWORD statusCode = 0; + DWORD dwSize = sizeof(statusCode); + if (!::WinHttpQueryHeaders(request, 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_result = HttpResult_NetworkFailure; + } + 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(request, 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(request, 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()); + response->m_result = HttpResult_NetworkFailure; + } + } + else + { + LOG_WARN("WinHttpQueryHeaders(RAW_HEADERS_CRLF) failed: %d", headerErr); + response->m_result = HttpResult_NetworkFailure; + } + // 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; + } + } + + { + 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(); + } + } + + 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) + { + size_t lineStart = 0; + while (lineStart < raw.size()) { + size_t lineEnd = raw.find("\r\n", lineStart); + if (lineEnd == std::string::npos) { + lineEnd = raw.size(); + } + + const std::string line = raw.substr(lineStart, lineEnd - lineStart); + const size_t colon = line.find(':'); + if (colon != std::string::npos) { + size_t valueStart = colon + 1; + while (valueStart < line.size() && line[valueStart] == ' ') { + ++valueStart; + } + response.m_headers.add(line.substr(0, colon), line.substr(valueStart)); + } + + if (lineEnd == raw.size()) { + break; + } + lineStart = lineEnd + 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 the machine-wide WinHTTP proxy configuration. This is + // the documented pre-Windows-8.1 behavior and avoids bypassing enterprise + // proxies entirely. + 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 default proxy", ::GetLastError()); + m_hSession = ::WinHttpOpen( + NULL, WINHTTP_ACCESS_TYPE_DEFAULT_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() +{ + CancelAllRequests(std::chrono::milliseconds::zero()); +} + +void HttpClient_WinHttp::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) +{ + if (bestEffortTimeout > std::chrono::milliseconds::zero()) + { + 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 without holding the lock. + for (const auto& id : ids) + CancelRequestAsync(id); + + std::unique_lock lock(m_requestsMutex); + m_requestsCv.wait_for(lock, bestEffortTimeout, [this]() noexcept -> bool { + return m_requests.empty(); + }); + } + else + { + // A request can be inserted after the initial cancellation snapshot + // while the producer side is still shutting down. Repeatedly take a + // snapshot and cancel until the map is empty; waiting only on the + // original snapshot can leave a late request uncancelled forever. + for (;;) + { + std::vector ids; + { + std::lock_guard lock(m_requestsMutex); + if (m_requests.empty()) + { + return; + } + for (auto const& item : m_requests) { + ids.push_back(item.first); + } + } + + for (const auto& id : ids) + CancelRequestAsync(id); + + std::unique_lock lock(m_requestsMutex); + m_requestsCv.wait_for(lock, std::chrono::milliseconds(100), [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.store(enforceMsRoot, std::memory_order_release); +} + +/// +/// Determines whether MS-Rooted server cert check required. +/// +/// +/// true if [MS-Rooted server cert check required]; otherwise, false. +/// +bool HttpClient_WinHttp::IsMsRootCheckRequired() +{ + return m_msRootCheck.load(std::memory_order_acquire); +} + +} 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..7a79e0e2e --- /dev/null +++ b/lib/http/HttpClient_WinHttp.hpp @@ -0,0 +1,70 @@ +// +// 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 "IBoundedHttpClientCancel.hpp" +#include "pal/PAL.hpp" + +#include "ILogManager.hpp" + +#include +#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 IBoundedHttpClientCancel { + 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 CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) 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; + std::atomic m_msRootCheck; + friend class WinHttpRequestWrapper; +}; + +} MAT_NS_END + +#endif // HAVE_MAT_DEFAULT_HTTP_CLIENT + +#endif // HTTPCLIENT_WINHTTP_HPP diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index 2ec8be9b0..64a3de2aa 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/lib/http/HttpClient_WinInet.cpp @@ -14,7 +14,6 @@ #include #include -#include #include #include #include @@ -55,6 +54,9 @@ class WinInetRequestWrapper if (m_hWinInetRequest != nullptr) { ::InternetCloseHandle(m_hWinInetRequest); + } + if (m_hWinInetSession != nullptr) + { ::InternetCloseHandle(m_hWinInetSession); } } @@ -593,7 +595,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 12ac6aa00..062c90318 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/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(); }; 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 - diff --git a/lib/pal/desktop/desktop.vcxitems b/lib/pal/desktop/desktop.vcxitems index 0d8ae8def..e827b6299 100644 --- a/lib/pal/desktop/desktop.vcxitems +++ b/lib/pal/desktop/desktop.vcxitems @@ -14,9 +14,12 @@ + + + ..\..;..\..\include;$(ProjectDir);$(GeneratedFilesDir);$(IntDir);%(AdditionalIncludeDirectories);$(WindowsSDK_IncludePath) diff --git a/lib/utils/annex_k.hpp b/lib/utils/annex_k.hpp index 5aa4b73af..98df5ebf2 100644 --- a/lib/utils/annex_k.hpp +++ b/lib/utils/annex_k.hpp @@ -7,9 +7,8 @@ #include #include #include -#ifndef _MSC_VER #include -#else +#ifdef _MSC_VER #include #endif @@ -47,21 +46,24 @@ 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) + // Compare half-open address ranges without pointer arithmetic: the + // arguments may refer to different objects, and invalid lengths must not + // wrap an end address before the overlap check. + if (buffer1_len == 0 || buffer2_len == 0) { - if (buffer1 + buffer1_len - 1 > buffer2 ) - { - return true; - } + return false; } - else + + uintptr_t begin1 = reinterpret_cast(buffer1); + uintptr_t begin2 = reinterpret_cast(buffer2); + if (buffer1_len > UINTPTR_MAX - begin1 || buffer2_len > UINTPTR_MAX - begin2) { - if (buffer2 + buffer2_len - 1 > buffer1) - { - return true; - } + return true; } - return false; + + uintptr_t end1 = begin1 + buffer1_len; + uintptr_t end2 = begin2 + buffer2_len; + return begin1 < end2 && begin2 < end1; } public: @@ -147,12 +149,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 +182,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 } }; } diff --git a/tests/functests/APITest.cpp b/tests/functests/APITest.cpp index baea0112e..50adf18a4 100644 --- a/tests/functests/APITest.cpp +++ b/tests/functests/APITest.cpp @@ -16,6 +16,8 @@ #include #include #include +#include +#include #include "PayloadDecoder.hpp" @@ -673,37 +675,31 @@ 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); + for (auto& uploadThread : uploadThreads) + { + uploadThread.join(); + } LogManager::FlushAndTeardown(); } removeAllListeners(debugListener); @@ -1252,8 +1248,8 @@ TEST(APITest, LogManager_BadStoragePath_Test) } -#ifdef HAVE_MAT_WININET_HTTP_CLIENT -/* This test requires WinInet HTTP client */ +#if defined(_WIN32) && defined(HAVE_MAT_DEFAULT_HTTP_CLIENT) +/* This test verifies the certificate policy used by either Windows HTTP transport. */ TEST(APITest, LogConfiguration_MsRoot_Check) { TestDebugEventListener debugListener; diff --git a/tests/functests/BasicFuncTests.cpp b/tests/functests/BasicFuncTests.cpp index bc879d3e6..f54bebd80 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; + // 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; diff --git a/tests/unittests/AnnexKTests.cpp b/tests/unittests/AnnexKTests.cpp index fa74e23f5..0df63787c 100644 --- a/tests/unittests/AnnexKTests.cpp +++ b/tests/unittests/AnnexKTests.cpp @@ -30,3 +30,10 @@ TEST(AnnexKTests, memcpy_s) EXPECT_EQ(BoundCheckFunctions::oneds_memcpy_s( dest, dest_len, src, dest_len + 1 ), EINVAL); EXPECT_EQ(BoundCheckFunctions::oneds_memcpy_s( dest, dest_len, (void *)((char *)dest + 1), src_len + 1 ), EINVAL); } + +TEST(AnnexKTests, memcpy_sAllowsAdjacentBuffers) +{ + char buffers[8] = {}; + + EXPECT_EQ(BoundCheckFunctions::oneds_memcpy_s(buffers, 4, buffers + 4, 4), 0); +} diff --git a/tests/unittests/HttpClientCurlTests.cpp b/tests/unittests/HttpClientCurlTests.cpp index 50a82a874..7f07a5b7c 100644 --- a/tests/unittests/HttpClientCurlTests.cpp +++ b/tests/unittests/HttpClientCurlTests.cpp @@ -13,6 +13,15 @@ #include "http/HttpClient_Curl.hpp" #include "config/RuntimeConfig_Default.hpp" +#include +#include +#include +#include +#include +#include +#include +#include + using namespace testing; using namespace MAT; @@ -184,6 +193,68 @@ TEST_F(HttpClientCurlTests, SetSslVerification_ConcurrentCallsNoRace) SUCCEED(); } +// --- Regression: EDEADLK self-join in ~CurlHttpOperation --- + +TEST_F(HttpClientCurlTests, SendAsync_DestroyOnWorkerThread_NoSelfJoin) +{ + struct TrackingCallback : public IHttpResponseCallback + { + std::atomic destroyEvents { 0 }; + void OnHttpResponse(IHttpResponse* response) override { delete response; } + void OnHttpStateEvent(HttpStateEvent state, void*, size_t) override + { + if (state == OnDestroy) + { + ++destroyEvents; + } + } + }; + + auto callback = std::make_shared(); + auto callbackDone = std::make_shared>(); + auto done = callbackDone->get_future(); + + auto op = std::make_shared( + "GET", "://malformed", callback.get(), m_headers, m_body, + false, 1 /*connTimeout*/, false /*sslVerify*/, ""); + + auto box = std::make_shared>(std::move(op)); + (*box)->SendAsync([box, callback, callbackDone](CurlHttpOperation&) { + box->reset(); + callbackDone->set_value(); + }); + + 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->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); +} + // --- Response-size cap (memory-amplification DoS hardening) --- class HttpClientCurlResponseCapTests : public ::testing::Test, @@ -195,9 +266,7 @@ class HttpClientCurlResponseCapTests : public ::testing::Test, HttpClient_Curl m_client; // The client never takes ownership of the request (it only stores a raw pointer // and erases it); the fixture owns it and frees it in TearDown -- on the main - // thread, after the transfer has completed. Freeing it inside OnHttpResponse - // would destroy the CurlHttpOperation from within its own async task, whose - // destructor waits on that task (a self-join deadlock). + // thread, after the transfer has completed. std::unique_ptr m_request; std::string m_hostname; size_t m_responseBodySize {0}; 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 -