ValveSoftware / ValveSoftware/GameNetworkingSockets

TSan: data race on the unsynchronized static FILE* lazy init in Plat_IsInDebugSession (Linux)

Open
#430 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
9.9k
Forks
749
Avg merge
1d 2h
Merged PRs (30d)
1

Description

Summary

On Linux, Plat_IsInDebugSession() caches its /proc/<pid>/status handle in a function-local static FILE * initialized by a plain test-and-set, with no synchronization:

src/tier0/dbg.cpp (master, L94-101):

#elif IsLinux()
	static FILE *fp;
	if ( !fp )                                     // <- unsynchronized read
	{
		char rgchProcStatusFile[256]; rgchProcStatusFile[0] = '\0';
		snprintf( rgchProcStatusFile, sizeof(rgchProcStatusFile), "/proc/%d/status", getpid() );
		fp = fopen( rgchProcStatusFile, "r" );     // <- unsynchronized write
	}

LockDebugInfo::AboutToUnlock() calls this from any thread that releases a lock it held past the long-lock warning threshold:

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel_misc.cpp:

if ( usecElapsed >= t.m_usecLongLockWarningThreshold && !Plat_IsInDebugSession() )

So two threads — an application thread inside a public API call, and GNS's own SteamNetworkingThreadProc — can execute the lazy-init concurrently. ThreadSanitizer reports the 8-byte race on fp.

Worth noting this is not the thread-safe form. A function-local static with an initializer is guaranteed thread-safe since C++11; an assignment inside an if body is not, and gets no such guarantee.

Two distinct problems live in those lines:

  1. The pointer. Both threads can see null, both fopen, and one handle leaks. This is what TSan flags.
  2. The stream. Even once initialized, rewind(fp) followed by fgets(fp, ...) (L109-110) is not atomic as a pair. glibc locks each call individually, so the FILE is not corrupted, but two threads interleaving them read from each other's file position — a wrong answer to "am I under a debugger". Minor in consequence, but wrong.

This is unrelated to #419 / #424 (the PlayStation code path missing a return) and to #279 (efficiency of the same block), both of which leave the lazy init as it is.

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. Current master (5f06b0a3c50be82297ccd012e5d6c298d90bfba7) carries the same implementation, so this is not fixed by upgrading.

Paths abbreviated to <GNS>. The application is a small authoritative game server; its only frame in the report is the flat-API call that was in progress.

WARNING: ThreadSanitizer: data race

  Read of size 8 by thread T1 (mutexes: write M0):
    #0 Plat_IsInDebugSession                      <GNS>/src/tier0/dbg.cpp:95
    #1 LockDebugInfo::AboutToUnlock()             <GNS>/.../steamnetworkingsockets_lowlevel_misc.cpp:321
    #2 Lock<std::recursive_timed_mutex>::unlock() <GNS>/.../steamnetworkingsockets_lowlevel.h:549
    #3 ScopeLock<ConnectionLock>::~ScopeLock()    <GNS>/.../steamnetworkingsockets_lowlevel.h:594
    #4 ConnectionScopeLock::~ConnectionScopeLock()<GNS>/.../steamnetworkingsockets_connections.h:344
    #5 CSteamNetworkingSockets::ReceiveMessagesOnConnection(...)
                                                  <GNS>/.../csteamnetworkingsockets.cpp:1425
    #6 SteamAPI_ISteamNetworkingSockets_ReceiveMessagesOnConnection
                                                  <GNS>/.../steamnetworkingsockets_flat.cpp:78
    #7 <application>::poll(...)                   <- the only application frame

  Previous write of size 8 by thread T2 (mutexes: write M1):
    #0 Plat_IsInDebugSession                      <GNS>/src/tier0/dbg.cpp:99
    #1 LockDebugInfo::AboutToUnlock()             <GNS>/.../steamnetworkingsockets_lowlevel_misc.cpp:321
    #2 Lock<std::recursive_timed_mutex>::unlock() <GNS>/.../steamnetworkingsockets_lowlevel.h:549
    #3 SteamNetworkingGlobalLock::Unlock()        <GNS>/.../steamnetworkingsockets_lowlevel_misc.cpp:445
    #4 PollRawUDPSockets(int, bool)               <GNS>/.../steamnetworkingsockets_socketthread.cpp:2646
    #5 SteamNetworkingSockets_InternalPoll(int, bool)
                                                  <GNS>/.../steamnetworkingsockets_socketthread.cpp:3320
    #6 SteamNetworkingThreadProc()                <GNS>/.../steamnetworkingsockets_socketthread.cpp:3438

SUMMARY: ThreadSanitizer: data race <GNS>/src/tier0/dbg.cpp:95 in Plat_IsInDebugSession

(Line 95 is the if ( !fp ) read and 99 the fp = fopen(...) write in the pinned v1.6.0 build; on current master the same two statements sit at 96 and 100.)

Reproduction

Nothing exotic — it only needs two threads to release a long-held lock at about the same time, which is what makes it show up under sanitizers rather than in release builds.

  1. Build GNS with -fsanitize=thread (application and GNS both instrumented).
  2. Run any workload with concurrent API traffic and a connection lifecycle — a server calling ReceiveMessagesOnConnection / SendMessages on one thread while GNS's service thread polls.
  3. Wait for two threads to cross the long-lock warning threshold together.

Observed intermittently — 1 run in 6 of a four-client integration test. The frequency is a property of how often the branch is taken, not of the defect: under TSan, locks routinely exceed m_usecLongLockWarningThreshold, which is what opens this normally-rare path at all.

Suggested fix directions

Any of these would resolve it; I have no stake in which:

  • A function-local static with an initializer, which C++11 makes thread-safe:
    static FILE *const fp = []() {
        char path[256];
        snprintf( path, sizeof(path), "/proc/%d/status", getpid() );
        return fopen( path, "r" );
    }();
    
  • std::call_once with a std::once_flag, if the lambda form is not to taste.
  • An atomic pointer (std::atomic<FILE *>, relaxed) if the double-fopen is considered acceptable and only the race needs removing.
  • Or any equivalent synchronized initialization.

If the stream sharing is also a concern, making the whole body thread_local, or guarding rewind/fgets together with a small mutex, would address point 2 as well.

Whatever the choice, it would help downstream users if the fix (or a decision that the race is intended and benign) were visible, since every project running GNS under ThreadSanitizer currently has to rediscover and characterize this independently, and decide on its own whether it is looking at a library-internal issue or a bug in its own code.

Environment

GNS v1.6.0, 2cb93a06350bb065db53abdb0d87cf297e0bfd34
Master checked 5f06b0a3c50be82297ccd012e5d6c298d90bfba7 — affected
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

Related but distinct: #419 / #424 (PlayStation return path), #279 (efficiency of the same block). Separately filed for this project: #429 (connection-state read outside the connection lock).

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/tier0/dbg.cpp at Plat_IsInDebugSession and trace its call from LockDebugInfo::AboutToUnlock in steamnetworkingsockets_lowlevel_misc.cpp. Reproduce the reported workload under ThreadSanitizer, then verify that initialization and the /proc status read are safe when concurrent threads release long-held locks. Done means the race is absent and debugger-session checks remain correct.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
networking, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.