Fix cleanup/request-kickoff races (#1018, #1019)

[Jason Sandlin (jasonsandlin)]

Summary

Fixes two races between HCCleanupAsync and HTTP request kickoff, reported in #1018 and #1019. Both are crashes in the field: a null-handle dereference during cancellation, and a use-after-free on NetworkState. They're related to #999 but distinct, and both persist after #1000.

Each fix is accompanied by a deterministic unit test that reproduces the exact reported interleaving. Both tests were confirmed to fail before the corresponding fix and pass after.

Issue #1018 — cancel races perform startup

NetworkState::HttpCallPerformAsyncProvider inserts a request into m_activeHttpRequests before calling HC_CALL::PerformAsync. Cleanup can therefore snapshot and cancel a request whose HC_CALL::PerfomAsyncProvider::Begin is still running:

1. Thread A: HttpCallPerformAsyncProvider::Begin inserts the request
2. Thread A: enters HC_CALL::PerfomAsyncProvider::Begin
3. Thread B: CleanupAsyncProvider::Begin snapshots and calls XAsyncCancel
4. Thread B: PerfomAsyncProvider::Cancel calls XTaskQueueTerminate(context->workQueue, ...)

If step 4 lands before Begin creates workQueue, the handle is nullptr. XTaskQueueTerminate passes it to GetQueue, which dereferences handle->m_signature — an access violation.

Fix: a startupMutex on PerformContext orders publication of the work queues against the cancel. If a cancel arrives first it records cancelRequested and returns instead of terminating a null handle; Begin observes the flag once it publishes the queues and completes the perform as canceled (E_ABORT) — the same outcome a later cancel produces.

Issue #1019 — refused request touches freed NetworkState

The m_cleanupStarted guard added in #1000 correctly refuses a perform that arrives after cleanup began, but the refused request's XAsyncOp::Cleanup still dereferenced NetworkState to take m_mutex and call erase().

That op runs asynchronously (completion port, after the client callback), and a refused request was never tracked — so nothing holds a singleton reference on its behalf. By the time it runs, cleanup may already have destroyed the singleton and the NetworkState it owns, so the deref is a use-after-free.

The existing erase() != 0 check prevented a spurious reschedule, but it could not prevent the dereference: it already required touching freed memory.

Fix: an admitted flag is set under m_mutex when the request is actually inserted. A request that was never admitted has no bookkeeping to undo and now touches NetworkState not at all.

The WebSocket path is already safe here — WebSocketConnectAsyncProvider's Cleanup op only reclaims its context, and WebSocketConnectComplete never runs for a refused connect. No change needed.

Tests

New Tests/UnitTests/Tests/CleanupRaceTests.cpp with one test per issue. Both force the reported interleaving deterministically rather than relying on stress, and both convert the crash into a normal assertion failure so a regression reports as a failed test rather than tearing down the test host.

• NetworkState: HTTP submission races cleanup, causing crashes in HC_CALL::PerfomAsyncProvider #1018 uses a test-only hook (HC_UNITTEST_API only) that parks a perform inside the startup window, before the work queues exist, then runs cleanup. It asserts no fault and that cleanup and the canceled perform both actually complete — so the test can't pass by simply never reaching the window.
• NetworkState: HTTP/WebSocket submission races cleanup (TOCTOU) causing UAF #1019 installs a guard-page allocator via the existing HCMemSetFunctions hook. Freed libHttpClient allocations are quarantined PAGE_NOACCESS instead of returned to the CRT, so a dangling access faults at the exact instruction. Cleanup and the refused request run on separate manual queues so cleanup can be driven to completion — destroying NetworkState — while the refused request's deferred cleanup is still pending. Setup assertions confirm the ordering actually occurred before the result is trusted.

Verified red → green: with the two fixes reverted (test seam retained), both tests fail with the fault assertion; with them applied, both pass.

Validation

• libHttpClient.UnitTest.TE, x64 Debug: 111/111 pass (109 pre-existing + 2 new), no regressions
• Win32 and GDK shipping libraries build clean, no new warnings
• End-to-end: 24/24 PlayFab lhc device scenarios pass, including two new ones that overlap a request burst with cleanup at varying timing offsets

Notes for reviewers

• Both races are in shared NetworkState/HC_CALL code, so the fixes apply to all platforms, not just Windows.
• The only product change outside the two fixes is the HC_UNITTEST_API-guarded test hook in httpcall.h/httpcall.cpp; it compiles out of shipping builds.
• When these races regress, the failure is not limited to the two new tests — the NetworkState: HTTP submission races cleanup, causing crashes in HC_CALL::PerfomAsyncProvider #1018 access violation corrupts global state and strands a thread, cascading into ~33 failures across the suite. Worth knowing if a large multi-test failure shows up here.

Fixes #1018
Fixes #1019