From 6a1c0500284edee4f37f450c334e9dc94cee0c64 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Sat, 1 Aug 2026 02:45:03 -0500 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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(); } }