Skip to content

NetworkState: HTTP/WebSocket submission races cleanup (TOCTOU), causing crashes or orphaned requests #999

Description

@jhugard

Summary

NetworkState (Source/Global/NetworkState.cpp) does not serialize network-operation
submission against the start of cleanup. When a client calls HCHttpCallPerformAsync or
HCWebSocketConnectAsync on one thread while another thread calls HCCleanup / HCCleanupAsync,
there is a time-of-check-to-time-of-use window that can:

  • dereference a moved-from m_networkState pointer and crash, and/or
  • insert a request into the cleanup tracking sets after cleanup has already snapshotted them,
    orphaning the request so it is never canceled or awaited and can run against a torn-down provider.

Both symptoms stem from a single root cause and are intermittent/timing-dependent.

Affected code

  • Source/Global/global_publics.cppHCHttpCallPerformAsync
  • Source/WebSocket/websocket_publics.cppHCWebSocketConnectAsync
  • Source/Global/global.cpphttp_singleton::CleanupAsyncProvider, singleton_access
  • Source/Global/NetworkState.cppHttpCallPerformAsyncProvider,
    WebSocketConnectAsyncProvider, CleanupAsyncProvider

Any consumer that can issue HTTP/WebSocket calls concurrently with library cleanup from a
different thread is exposed (e.g. XSAPI/PlayFab-style hosts that tear down while requests may still
be in flight).

Background

A submission goes through the public entry point, which acquires the singleton and then calls into
NetworkState:

// HCHttpCallPerformAsync (Source/HTTP/httpcall_publics.cpp)
auto httpSingleton = get_http_singleton();          // (A) strong ref; may be non-null
if (nullptr == httpSingleton) return E_HC_NOT_INITIALISED;
return httpSingleton->m_networkState->HttpCallPerformAsync(call, asyncBlock);  // (B) deref + submit

get_http_singleton() uses singleton_access_mode::get, which returns a copy of the shared
pointer without incrementing the singleton use count. XAsyncBegin dispatches the provider
XAsyncOp::Begin synchronously on the calling thread, so the submission's insert into the
tracking set happens synchronously inside step (B).

Cleanup runs as:

// http_singleton::CleanupAsyncProvider::Begin (Source/Global/global.cpp)
singleton_access(cleanup, ...);                       // detaches: resets the static singleton
NetworkState::CleanupAsync(std::move(singleton->m_networkState), ...);  // moves NetworkState out
// -> NetworkState::CleanupAsyncProvider::Begin snapshots m_activeHttpRequests under m_mutex

The in-code comment claimed new performs cannot enter after cleanup begins because the singleton is
detached first. That is insufficient: detaching only prevents future get_http_singleton()
callers from acquiring the singleton. A caller that already passed the null check at (A) holds a
strong reference and is completely unsynchronized against cleanup.

Race A — request inserted after the cleanup snapshot

HttpCallPerformAsyncProvider::Begin inserts into m_activeHttpRequests, and
CleanupAsyncProvider::Begin snapshots that set; likewise WebSocketConnectAsyncProvider::Begin
inserts into m_connectingWebSockets. Both the insert and the snapshot take m_mutex, but their
ordering is unconstrained:

  1. Cleanup acquires m_mutex, snapshots the (empty) set, sees nothing pending, and proceeds to
    schedule provider teardown.
  2. The submission then acquires m_mutex and inserts its context.

The inserted request is now absent from the cancel snapshot and is not counted by the pending-work
check, so cleanup can tear down the provider and complete while the request is still live. The
submission is never canceled or awaited and runs against a destroyed provider — use-after-free
and/or a request that completes long after the client believed cleanup finished. The submission
Begin path is the only mutation of the tracking sets that does not consult cleanup state, so a
late insert bypasses the entire cancel/await handshake.

Race B — dereference of a moved-from m_networkState

  1. Thread T1 executes (A): get_http_singleton() returns non-null and T1 holds a strong reference.
  2. Thread T2 runs http_singleton::CleanupAsyncProvider::Begin, which resets the static singleton
    and then std::move(singleton->m_networkState), leaving the singleton's m_networkState
    UniquePtr null. The moved-out NetworkState is subsequently destroyed when provider cleanup
    completes.
  3. Thread T1 executes (B): httpSingleton->m_networkState->... reads the moved-from (null) pointer
    and dereferences it.

This is both a null dereference and a data race on the m_networkState UniquePtr (concurrent
read on T1 and move-write on T2 with no synchronization), i.e. undefined behavior. The singleton's
use_count > 1 cleanup gate does not help: it defers destruction of the singleton object, but
m_networkState is moved out (and the NetworkState destroyed) independently of that gate.

Because the move nulls m_networkState before the submission reaches NetworkState, Race B often
manifests first on the HTTP submit path; when a submission does survive the move, Race A can then
occur. They are two facets of the same missing serialization between submission and cleanup-begin.

Impact

  • Intermittent crash (null dereference / UB) during shutdown when HTTP or WebSocket calls are
    submitted concurrently with HCCleanup.
  • Orphaned in-flight requests that outlive cleanup and run against a torn-down provider
    (use-after-free), or that never complete/cancel as the client expects.
  • Timing-dependent; more likely under load or with frequent init/cleanup cycles.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions