TSan: data race on CSteamNetworkConnectionBase::m_eConnectionState via the unlocked GetState() fast path in InternalGetConnectionByHandle

[oOTimothyOo]

Summary

InternalGetConnectionByHandle() reads the connection state before the connection lock has been taken, as a fast-path bail-out. The comment there says this is intentional:

src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp (master, L317-323):

// Fetch the state of the connection.  This is OK to do
// even if we don't have the lock.  If the connection
// is already dead we can avoid even trying to take
// the lock.  That's good because cleaning up is one
// of the rare cases where we take locks in the opposite
// order, so we want to avoid that.
ESteamNetworkingConnectionState s = pResult->GetState();

GetState() is a plain read of a plain member:

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h L421 and L897:

ESteamNetworkingConnectionState GetState() const { return m_eConnectionState; }
...
ESteamNetworkingConnectionState m_eConnectionState;

Meanwhile SetState() writes that same member from the SteamNetworkingThreadProc service thread while holding the global and connection locks — which the reader above does not hold at the point of the read.

The intent is clearly benign (the value is only a fast-path hint, and it is re-read under the connection lock once that lock is held). But because the member is a non-atomic object, the read/write pair is a data race under the C++ memory model, and ThreadSanitizer reports it as one.

I am not asking for the locking to change — the fast-path read looks deliberate and well-motivated. The narrow fix would be to make the member a std::atomic accessed with memory_order_relaxed, which preserves the fast path, costs nothing on any supported target, and removes the UB and the tool report.

Report

Reproduced under Clang 21.1.8 -fsanitize=thread on Linux (AlmaLinux 10.2, x86-64), against v1.6.0 (2cb93a06350bb065db53abdb0d87cf297e0bfd34), built from source via FetchContent. Paths abbreviated to <GNS>; the application is a small authoritative game server whose only frame in the report is the flat-API SendMessages call.

WARNING: ThreadSanitizer: data race (pid=10260)
  Write of size 4 at 0x728c00001a78 by thread T2 (mutexes: write M0, write M1):
    #0 CSteamNetworkConnectionBase::SetState(ESteamNetworkingConnectionState, long long)
         <GNS>/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp:2449
    #1 CSteamNetworkConnectionBase::ConnectionState_ClosedByPeer(int, char const*)
         <GNS>/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp:3263
    #2 CConnectionTransportUDPBase::Received_ConnectionClosed(...)
         <GNS>/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.cpp:922
    #3 CConnectionTransportUDP::PacketReceived(...)
         <GNS>/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_udp.cpp:1453
    #4 CRecvPacketCallback::operator()(...)   <GNS>/.../steamnetworkingsockets_lowlevel.h:100
    #5 CSharedSocket::DefaultCallbackRecvPacket(...)
         <GNS>/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_socketthread.cpp:3590
    #6 CRecvPacketCallback::operator()(...)   <GNS>/.../steamnetworkingsockets_lowlevel.h:100
    #7 DrainSocket(CRawUDPSocketImpl*)        <GNS>/.../steamnetworkingsockets_socketthread.cpp:2575
    #8 PollRawUDPSockets(int, bool)           <GNS>/.../steamnetworkingsockets_socketthread.cpp:2724
    #9 SteamNetworkingSockets_InternalPoll(int, bool)
         <GNS>/.../steamnetworkingsockets_socketthread.cpp:3320
       (called from SteamNetworkingThreadProc, <GNS>/.../steamnetworkingsockets_socketthread.cpp:3438)

  Previous read of size 4 at 0x728c00001a78 by thread T1 (mutexes: write M2):
    #0 CSteamNetworkConnectionBase::GetState() const
         <GNS>/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h:421
    #1 InternalGetConnectionByHandle(unsigned int, ConnectionScopeLock&, char const*, bool)
         <GNS>/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp:323
    #2 GetConnectionByHandleForAPI(unsigned int, ConnectionScopeLock&, char const*)
         <GNS>/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp:374
    #3 CSteamNetworkingSockets::SendMessages(int, SteamNetworkingMessage_t**, long long*, bool)
         <GNS>/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp:1354
    #4 SteamAPI_ISteamNetworkingSockets_SendMessages
         <GNS>/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_flat.cpp:70
    #5 <application>::send(...)          <- the only application frame in the report

SUMMARY: ThreadSanitizer: data race
  .../steamnetworkingsockets_connections.cpp:2449
  in SteamNetworkingSocketsLib::CSteamNetworkConnectionBase::SetState(...)

M2 on the reader is the table lock (g_tables_lock); M0/M1 on the writer are the global lock and the connection lock. The reader's line-323 access is the one taken before scopeLock.TryLock( *pResult->m_pLock, ... ) succeeds, so the two accesses share no lock, exactly as the comment describes.

Reproduction

Nothing exotic is required — just enough concurrent API traffic that a SendMessages() call overlaps a peer-initiated close.

1. Build GNS with -fsanitize=thread (the application and GNS both instrumented).
2. Run a server that calls SteamAPI_ISteamNetworkingSockets_SendMessages on a single application thread every tick, for every connected peer.
3. Connect several clients and have them disconnect while traffic is in flight.

In our case the report appeared reliably once the send rate rose: at ~12 outbound packets per run it never fired; at ~18,800 it fires on essentially every run. The race itself does not depend on the send rate, only the chance of catching the window does.

Suggested fix

// steamnetworkingsockets_connections.h
std::atomic<ESteamNetworkingConnectionState> m_eConnectionState;

ESteamNetworkingConnectionState GetState() const
{ return m_eConnectionState.load( std::memory_order_relaxed ); }

with the corresponding relaxed store in SetState(). This keeps the unlocked fast-path read that the comment is defending, keeps the re-check under the connection lock, generates the same instruction on every supported target, and makes the access well-defined so TSan stops reporting it.

If the read is intended to stay non-atomic, __tsan_acquire/__tsan_release annotations or a documented race_top: suppression entry shipped with the project would at least let downstream users tell this apart from their own bugs. Right now every application that runs GNS under ThreadSanitizer has to independently rediscover and characterize this, and decide on its own whether it is looking at a library-internal design choice or at a bug in its own code.

Environment

GNS	v1.6.0, 2cb93a06350bb065db53abdb0d87cf297e0bfd34 (behaviour verified unchanged on master)
Compiler	Clang 21.1.8, -fsanitize=thread
OS	AlmaLinux 10.2, x86-64, WSL2 kernel 6.18
Build	FetchContent, static OSS direct-IP client library, no ICE