Fix Resource_Manager::Debug and Storage_Schema::ICurrentData() data races#1399
Open
gsdali wants to merge 1 commit into
Open
Fix Resource_Manager::Debug and Storage_Schema::ICurrentData() data races#1399gsdali wants to merge 1 commit into
gsdali wants to merge 1 commit into
Conversation
…aces Resource_Manager::Resource_Manager(const char*, bool) writes a file-scope static bool Debug on every construction with zero synchronization; two Resource_Manager instances constructed concurrently on different threads (e.g. two independent TDocStd_Application instances each lazily building their own via Resources()/DefineFormat()) race on this write. Fixed by making Debug a std::atomic<bool>. Storage_Schema::ICurrentData() is a function-local static Handle(Storage_Data) mutated with no synchronization: Write() sets it for the duration of one store, and any Storage_Schema construction (including a throwaway instance built only to read header info, e.g. PCDM_ReadWriter_1::ReadDocumentVersion) calls Clear() -> ICurrentData().Nullify() in its constructor, which can null out a different, unrelated Storage_Schema instance's in-flight Write() on another thread. Fixed by adding Storage_Schema::ICurrentDataMutex(), a recursive mutex (recursive because Write()'s own critical section re-enters BindType()/AddPersistent()/PersistentToAdd() on the same thread via Storage_CallBack::Write() callbacks), guarding every touch point: the constructor's Clear(), Write()'s entire body, BindType(), TypeBinding(), AddPersistent(), PersistentToAdd(), HasTypeBinding(), and ISetCurrentData(). Both races were found while testing a private TDocStd_Application-per-caller usage pattern (multiple independent application instances constructed concurrently, never sharing state) -- see the attached issue for a full TSan writeup and reproducer.
gsdali
added a commit
to SecondMouseAU/OCCTSwift
that referenced
this pull request
Jul 24, 2026
…aces (v1.15.18) Fixes the two upstream OCCT foundation-layer races #371's confirmation harness turned up (OCCT#1398): Resource_Manager::Debug is now a std::atomic<bool>, and Storage_Schema::ICurrentData() is guarded by a new recursive mutex spanning construction, Write(), and the type-binding accessors. Kernel-only change (Scripts/patches/0016); OCCT.xcframework rebuilt, OCCTBridge.xcframework unchanged. Upstream fix proposed as Open-Cascade-SAS/OCCT#1399 (fixes OCCT#1398). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1398.
Resource_Manager::Resource_Manager(const char*, bool)(Resource_Manager.cxx:109) writes afile-scope
static bool Debugon every construction with zero synchronization. TwoResource_Managerinstances constructed concurrently on different threads (e.g. two independentTDocStd_Applicationinstances each lazily building their own viaResources()/DefineFormat())race on this write. Fixed by making
Debugastd::atomic<bool>— it is a plain process-widedebug-logging flag, not meant to express per-instance intent, so an atomic is sufficient here.
Storage_Schema::ICurrentData()(Storage_Schema.cxx:802) is a function-local staticHandle(Storage_Data)mutated with no synchronization anywhere:Write()sets it for theduration of one store, and any
Storage_Schemaconstruction — including a throwaway instancebuilt only to read header info, e.g.
PCDM_ReadWriter_1::ReadDocumentVersion/ReadReferenceCounter/ReadReferences, all reachable unconditionally from a plainTDocStd_Application::Open()viaCDF_Application::Retrieve— callsClear()->ICurrentData().Nullify()in its constructor, which can null out a different, unrelatedStorage_Schemainstance's in-flightWrite()on another thread (or race a concurrent load's ownthrowaway construction). Fixed by adding
Storage_Schema::ICurrentDataMutex(), astd::recursive_mutex(recursive becauseWrite()'s own critical section re-entersBindType()/AddPersistent()/PersistentToAdd()on the same thread via per-typeStorage_CallBack::Write()callbacks — a plain mutex would deadlock there) guarding every touchpoint: the constructor's
Clear(),Write()'s entire body (held for the whole call, since oneWrite()is a single atomic "session" against this global, not just each individual access),BindType(),TypeBinding(),AddPersistent(),PersistentToAdd(),HasTypeBinding(), andISetCurrentData().Both races are invisible under the traditional single shared
XCAFApp_Application/TDocStd_Applicationusage pattern, because in that patternResources()'s own lazy-init mutexhappens to serialize
Resource_Manager/Storage_Schemaconstruction down to "runs once, ever, forthe whole process." They only become reachable once an application is constructed privately per
caller/thread (the pattern this project's own
TDocStd_Applicationdocumentation recommends overthe deprecated
XCAFApp_Application::GetApplication()singleton) and multiple independentinstances are built concurrently — see the linked issue for the full TSan writeup and a
standalone reproducer.
No public API signature changes.
Type of change
How Has This Been Tested?
ThreadSanitizer (minimal-module build: FoundationClasses + ModelingData + ModelingAlgorithms +
DataExchange,
RelWithDebInfo,-fsanitize=thread -g), 8 threads x 30 rounds, each round buildinga brand-new private
TDocStd_Application, defining formats, populating and saving a document,closing it, then a separate brand-new private application opening + verifying + closing it — no
two threads ever share an application/schema instance.
Resource_Manager::Resource_Manager, 11xStorage_Schema::Storage_Schema'sClear()call), 0 functional failures.Reproducer and full writeup:
https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/374-resource-manager-storage-schema-race
Checklist:
clang-format --dry-run --Werrorclean onevery touched file)
internal-only synchronization change)