diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 9b931ba8b7..87f2067010 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -10,6 +10,7 @@ ### Bugs Fixed +- [[#7353]](https://github.com/Azure/azure-sdk-for-cpp/pull/7353) Fixed a hang in the WinHTTP transport where `WinHttpRequest`'s destructor could block forever when the request was destroyed before `WinHttpSendRequest` associated the request context with the handle, causing `WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING` to be discarded. - [[#7200]](https://github.com/Azure/azure-sdk-for-cpp/pull/7200) Fix global-buffer-overflow and undefined shift in `Base64Decode()`. (A community contribution, courtesy of _[groeneai](https://github.com/groeneai)_) ### Other Changes diff --git a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp index 37be746fcb..6dc1edcdfc 100644 --- a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp +++ b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp @@ -1432,6 +1432,30 @@ namespace Azure { namespace Core { namespace Http { namespace _detail { // Set the callback function to be called whenever the state of the request handle changes. m_httpAction = std::make_unique<_detail::WinHttpAction>(this); + // Associate the action with the request handle before the status callback is registered. + // + // WinHttpSendRequest() also passes this value as its context parameter, but it is not reached + // if the request is destroyed first - for example when an exception is thrown while preparing + // the request. In that case WinHttpAction::StatusCallback() is invoked with dwContext == 0 and + // discards every notification, including the WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING that + // ~WinHttpRequest() blocks on, so the destructor would wait forever. + // + // Binding the context here guarantees HANDLE_CLOSING is always delivered, so the destructor's + // close barrier - which exists to keep WinHTTP worker threads from dereferencing a freed + // WinHttpAction - always completes. + // + // NB: DO NOT CHANGE THE TYPE OF THE CONTEXT VALUE WITHOUT UPDATING + // WinHttpAction::StatusCallback. + DWORD_PTR contextValue = reinterpret_cast(m_httpAction.get()); + if (!WinHttpSetOption( + m_requestHandle.get(), + WINHTTP_OPTION_CONTEXT_VALUE, + &contextValue, + sizeof(contextValue))) + { + GetErrorAndThrow("Error while setting the request context value."); + } + if (!m_httpAction->RegisterWinHttpStatusCallback(m_requestHandle)) { GetErrorAndThrow("Error while setting up the status callback."); diff --git a/sdk/core/azure-core/test/ut/CMakeLists.txt b/sdk/core/azure-core/test/ut/CMakeLists.txt index b284c0cc8c..234a175106 100644 --- a/sdk/core/azure-core/test/ut/CMakeLists.txt +++ b/sdk/core/azure-core/test/ut/CMakeLists.txt @@ -97,6 +97,7 @@ add_executable ( transport_policy_options.cpp url_test.cpp uuid_test.cpp + win_http_transport_test.cpp ) target_compile_definitions(azure-core-test PRIVATE _azure_BUILDING_TESTS) diff --git a/sdk/core/azure-core/test/ut/win_http_transport_test.cpp b/sdk/core/azure-core/test/ut/win_http_transport_test.cpp new file mode 100644 index 0000000000..2e02e06ab2 --- /dev/null +++ b/sdk/core/azure-core/test/ut/win_http_transport_test.cpp @@ -0,0 +1,109 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#include + +#if defined(AZ_PLATFORM_WINDOWS) && defined(BUILD_TRANSPORT_WINHTTP_ADAPTER) + +#include "azure/core/context.hpp" +#include "azure/core/http/http.hpp" +#include "azure/core/http/win_http_transport.hpp" +#include "azure/core/io/body_stream.hpp" +#include "azure/core/url.hpp" + +#include +#include +#include +#include +#include +#include + +#include + +namespace Azure { namespace Core { namespace Test { + + namespace { + + // How long to wait for the transport call to return before declaring the thread stuck. The + // call either succeeds or throws within milliseconds, so any wait beyond a few seconds means + // the destructor is blocked. + constexpr auto TransportCallTimeout = std::chrono::seconds(30); + + // The URL is never contacted: WinHttpOpen(), WinHttpConnect() and WinHttpOpenRequest() only + // allocate handles, and the request fails during setup before anything is sent on the wire. + constexpr const char* TestUrl = "https://localhost/"; + + // A body stream whose Length() throws. + // + // WinHttpRequest::SendRequest() calls request.GetBodyStream()->Length() before it calls + // WinHttpSendRequest(), so throwing from Length() aborts the request after the WinHttpRequest + // has been constructed (its status callback is registered) but before WinHttpSendRequest() + // associates the request context with the handle. + class ThrowingLengthBodyStream final : public Azure::Core::IO::BodyStream { + public: + int64_t Length() const override + { + throw std::runtime_error("Injected failure from BodyStream::Length"); + } + + private: + size_t OnRead(uint8_t*, size_t, Azure::Core::Context const&) override { return 0; } + }; + + void SendRequestThatFailsDuringSetup() + { + Azure::Core::Http::WinHttpTransport transport; + ThrowingLengthBodyStream bodyStream; + Azure::Core::Http::Request request( + Azure::Core::Http::HttpMethod::Put, Azure::Core::Url(TestUrl), &bodyStream); + + Azure::Core::Context context; + static_cast(transport.Send(request, context)); + } + + } // namespace + + // A request that fails during setup, before WinHttpSendRequest() associates the request context + // with the handle, must not block the calling thread. + // + // ~WinHttpRequest() closes the request handle and waits for + // WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING so that no WinHTTP worker thread can dereference the + // WinHttpAction after it has been freed. WinHttpAction::StatusCallback() drops every + // notification that arrives with dwContext == 0, and the context is bound to the handle only by + // WinHttpSendRequest(). Without the context being bound in the constructor, the destructor waits + // for a notification that is discarded, on a default-constructed Context that is never + // cancelled, and the thread is lost for the lifetime of the process. + // + // The work runs on a detached thread on purpose: when the bug is present the thread cannot be + // joined, so the test must still be able to report the failure. On Windows, process exit + // terminates the leaked thread. + TEST(WinHttpTransport, RequestThatFailsDuringSetupDoesNotHang) + { + auto callCompleted = std::make_shared>(); + std::future callCompletedFuture = callCompleted->get_future(); + + std::thread([callCompleted]() { + try + { + SendRequestThatFailsDuringSetup(); + } + catch (...) + { + // Expected: the injected Length() failure propagates out of Send(). All this test cares + // about is that the call returns rather than blocking forever. + } + + callCompleted->set_value(); + }).detach(); + + ASSERT_EQ(std::future_status::ready, callCompletedFuture.wait_for(TransportCallTimeout)) + << "WinHttpTransport::Send did not return within " + << std::chrono::duration_cast(TransportCallTimeout).count() + << "s. ~WinHttpRequest is blocked waiting for WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, " + "which is discarded because the request context was never associated with the request " + "handle."; + } + +}}} // namespace Azure::Core::Test + +#endif // AZ_PLATFORM_WINDOWS && BUILD_TRANSPORT_WINHTTP_ADAPTER