Fix WinHttpRequest destructor hang when the request context is never bound - #7353
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). 8 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| ### Bugs Fixed | ||
|
|
||
| - 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)_) |
There was a problem hiding this comment.
We should not really touchthe changel;og as part of the PR methinks.
There was a problem hiding this comment.
Good call - removed. The CHANGELOG entry is dropped from this PR (commit 8623ac0), so the change is now limited to the fix, the new test, and its CMakeLists registration.
Happy to add the entry back in whatever form you prefer at merge time, or leave it entirely to your release tooling.
There was a problem hiding this comment.
Why? The pattern for the C++ repository has typically been to update the changelog with the change, that way changes don't get missed.
What is your alternate proposal to ensure that this content gets into the changelog?
There was a problem hiding this comment.
Larry Osterman (@LarryOsterman) I see, my bad. I thought that my bot went completely overboard with adding it to the changelog, so I asked it to remove it. Let me re-add it to the changelog. Thanks!
There was a problem hiding this comment.
Bot reply:
"Re-added in f677594 - on a closer look this does appear to be the established pattern for azure-core: 15 of the last 25 commits touching sdk/core/azure-core/src also updated CHANGELOG.md, and #6637 (the closely related WinHTTP FailFast fix) added one too.
I matched the current format used by #7196 rather than my original entry, i.e. prefixed with the PR link and inserted at the top of the ### Bugs Fixed section:
- [[#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 ...
Happy to drop it again if you'd rather the entry be written at release time."
There was a problem hiding this comment.
Not sure what the "reply2.txt" files are.
There was a problem hiding this comment.
Larry Osterman (@LarryOsterman) Bot got stupid and pasted the link to the local file as PR comment instead of the actual comment it generated.
There was a problem hiding this comment.
I pasted the actual contents, where it just said it did what I told it to do.
d9e7d0c to
8623ac0
Compare
|
Azure Pipelines: Successfully started running 2 pipeline(s). 8 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a WinHTTP adapter teardown deadlock in azure-core where WinHttpRequest::~WinHttpRequest() could block forever if the request is destroyed before WinHttpSendRequest() associates the callback context with the request handle. The change ensures the destructor’s existing “wait for WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING” barrier is always satisfiable, and adds a unit test to prevent regressions.
Changes:
- Bind the WinHTTP context value in
WinHttpRequestconstruction viaWinHttpSetOption(WINHTTP_OPTION_CONTEXT_VALUE, ...)before registering the status callback. - Add a WinHTTP-focused unit test that deterministically throws during request setup (before
WinHttpSendRequest) and asserts the call returns (does not hang). - Wire the new test file into the azure-core unit test target.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp | Sets WINHTTP_OPTION_CONTEXT_VALUE during request construction so HANDLE_CLOSING callbacks always have a valid context and the destructor barrier can complete. |
| sdk/core/azure-core/test/ut/win_http_transport_test.cpp | Adds WinHttpTransport.RequestThatFailsDuringSetupDoesNotHang regression test covering the pre-WinHttpSendRequest failure window. |
| sdk/core/azure-core/test/ut/CMakeLists.txt | Includes the new WinHTTP transport unit test source in the test executable build. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…bound ~WinHttpRequest closes the request handle and then waits for WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING so that no WinHTTP worker thread can dereference the WinHttpAction after it is freed. WinHttpAction::StatusCallback discards every notification that arrives with dwContext == 0, and the context is associated with the handle only by WinHttpSendRequest, while the status callback is registered at the end of the constructor. A request destroyed between those two points therefore waits for a notification that is discarded, on a default-constructed Azure::Core::Context that is never cancelled, and the calling thread is lost for the lifetime of the process. Bind the context with WinHttpSetOption(WINHTTP_OPTION_CONTEXT_VALUE) in the constructor so HANDLE_CLOSING is always delivered and the existing barrier always completes. WinHttpSendRequest continues to pass the same value, which is idempotent. Adding a timeout to the destructor's wait would not be a valid fix: abandoning the barrier lets a WinHTTP worker thread invoke the callback after the objects are destroyed, turning a hang into a use-after-free. Adds a regression test that constructs a request whose body stream throws from Length(), which enters the window without requiring any network traffic. Fixes Azure#7352
8623ac0 to
f677594
Compare
Fixes #7352
Problem
WinHttpRequest::~WinHttpRequest()closes the request handle and then waits forWINHTTP_CALLBACK_STATUS_HANDLE_CLOSINGbefore returning. That barrier is deliberate andnecessary: WinHTTP is used in async mode (
WINHTTP_FLAG_ASYNC), the callback context is a rawpointer to the
WinHttpActionowned by the request, and the callback also dereferencesm_httpRequest. Returning beforeHANDLE_CLOSINGwould let a WinHTTP worker thread touch freedmemory.
However,
WinHttpAction::StatusCallback()discards every notification that arrives withdwContext == 0, and the context is bound to the handle only byWinHttpSendRequest()— whilethe status callback is registered much earlier, at the end of the constructor. Any request
destroyed in between (for example when an exception is thrown while preparing headers or querying
the body stream length) therefore closes its handle, receives
HANDLE_CLOSINGwith no context, hasit dropped, and blocks in
WaitForActionforever. The wait uses a default-constructedAzure::Core::Context, soThrowIfCancelled()never fires and the poll loop spins indefinitely —the thread is lost for the lifetime of the process.
CompleteActionWithError()intentionally does not signal while waiting forHANDLE_CLOSING, sothe
REQUEST_ERROR/ERROR_WINHTTP_OPERATION_CANCELLEDnotification that WinHTTP raises whenclosing a handle with a pending operation does not release the waiter either.
This is the hang that remains in the window #6637 addressed for the FailFast case; that PR's own
comment already observes that "the
initiateActioncall is a call toWinHttpSendRequestwhichestablishes the SendContext".
Fix
Bind the context to the request handle in the constructor with
WinHttpSetOption(WINHTTP_OPTION_CONTEXT_VALUE, ...), immediately before registering the statuscallback.
HANDLE_CLOSINGis then always delivered with a valid context, so the destructor'sbarrier always completes.
WinHttpSendRequest()continues to pass the same value, which isidempotent.
The barrier itself is unchanged — the fix makes it satisfiable rather than removing it.
Alternatives considered
reason the bug exists (with no context every callback is already dropped, so there is nothing to
synchronize against), but smaller in scope and leaves
HANDLE_CLOSINGunobservable.thread to invoke the callback after
WinHttpAction/WinHttpRequestare destroyed, converting ahang into a use-after-free.
CompleteActionWithError()during close. Rejected for the same reason:it would release the waiter before
HANDLE_CLOSING, which is precisely what the barrier prevents.Test
sdk/core/azure-core/test/ut/win_http_transport_test.cppaddsWinHttpTransport.RequestThatFailsDuringSetupDoesNotHang.SendRequest()callsrequest.GetBodyStream()->Length()beforeWinHttpSendRequest(), andBodyStream::Length()is notnoexcept, so a body stream that throws fromLength()enters thewindow deterministically. The test needs no network:
WinHttpOpen(),WinHttpConnect()andWinHttpOpenRequest()only allocate handles, and the request fails before anything is sent.The work runs on a detached thread guarded by a 30 s future wait, so a regression fails the test
instead of hanging the CI run (a blocked destructor cannot be joined).
Verified against the vendored 1.16.3 sources in a downstream consumer: an equivalent test hangs
and fails on the 30 s timeout without the product change, and passes in 0.76 s with it. A leak
checker also reported the abandoned
WinHttpRequestfromWinHttpTransportImpl::CreateRequestHandle()before the fix, and no leaks after.The full CI matrix is green on this PR, including the
Win2022_Win32Api_debug_tests_winhttp_x64and
x86legs that build and run the new test.Pull Request Checklist
Authored with GitHub Copilot CLI — session ID
6afdad25-6ece-4e65-991c-aa8d88d138fa