OCAF - Fix data races in XCAFApp_Application::GetApplication and CDF_Directory#1390
Open
gsdali wants to merge 4 commits into
Open
OCAF - Fix data races in XCAFApp_Application::GetApplication and CDF_Directory#1390gsdali wants to merge 4 commits into
gsdali wants to merge 4 commits into
Conversation
…Directory XCAFApp_Application::GetApplication()'s lazy singleton init raced two threads' first concurrent call into both constructing a new instance and assigning the shared handle with no synchronization -- confirmed by ThreadSanitizer to construct multiple concurrent XCAFApp_Application instances, cascading into races across dozens of unrelated destructors (TDF_LabelNode::Destroy, CDM_Document::~CDM_Document, NCollection_BaseList::PClear, ...) as the "losing" instances are torn down. Fixed by folding construction into the static local's initializer, relying on the C++11 magic-statics guarantee. CDF_Directory::Add/Remove/Contains mutate/read myDocuments (a plain NCollection_List) with no synchronization at all -- every document created through a shared CDF_Application (the normal usage pattern, e.g. the above singleton) races on NCollection_BaseList::PAppend. Guarded with a private std::mutex. Reported and reproduced (10 threads, barrier-synchronized concurrent NewDocument() calls, SIGSEGV ~50% of runs) as SecondMouseAU/OCCTSwift#344.
…rces, and CDF_Application's reader/writer maps Discovered while validating the previous commit's fix under repeated swift test runs: fixing XCAFApp_Application::GetApplication()'s lazy-init race means every caller now genuinely shares ONE TDocStd_Application instance (as intended) -- which surfaced further races on that same instance's other unsynchronized state, previously masked by threads sometimes getting different (uncontended) instances. TDocStd_Application::Resources() has the same lazy-init race as GetApplication() did; Resource_Manager's own maps (myRefMap/myUserMap/ myExtStrMap) have no synchronization at all; CDF_Application's myReaders/ myWriters (format-name to driver maps) are read/written from DefineFormat, ReaderFromFormat/WriterFromFormat, and ReadingFormats/WritingFormats with no locking. Fix: a mutex guarding TDocStd_Application::Resources()'s lazy-init; a recursive_mutex guarding Resource_Manager's public accessors (nested calls, e.g. Integer()/ExtValue() call Value() internally); a mutex guarding CDF_Application's myReaders/myWriters across all their access points. Resource_Manager gains an explicit copy constructor (copying under the source's lock) since the new mutex member deletes the implicit one -- ShapeProcess_Context.cxx relies on Resource_Manager being copyable for its own (pre-existing) thread-safety workaround. Confirmed via repeated `swift test` runs against a downstream Swift wrapper (SecondMouseAU/OCCTSwift): SIGTRAP in Resource_Manager::SetResource (via TDocStd_Application::DefineFormat) and SIGSEGV in TDocStd_Application::ReadingFormats, both reproducible before this commit and clean across 12 further runs after it.
…'s copy ctor Fixes GCC -Wextra: "base class should be explicitly initialized in the copy constructor". No behavior change (the base was already default-constructed implicitly).
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.
Summary
XCAFApp_Application::GetApplication()'s lazy singleton init races two threads' first concurrentcall: both can observe
locApp.IsNull()and both construct a newXCAFApp_Application, racing toassign the shared handle. ThreadSanitizer shows this is the most severe of several races on the
same shared singleton — it produces multiple concurrently-constructed
XCAFApp_Applicationinstances, cascading into races across dozens of unrelated destructors (
TDF_LabelNode::Destroy,CDM_Document::~CDM_Document,NCollection_BaseList::PClear, ...) as the "losing" instances aretorn down while other threads are still constructing/using a same-generation object.
CDF_Directory::Add/Remove/Containsmutate/readmyDocuments(a plainNCollection_List) withzero synchronization. Every
CDF_Applicationis normally a single process-wide instance shared byevery caller — that's the entire point of a
GetApplication()-style accessor — so its oneCDF_DirectoryreceivesAdd()from every document-creating call on every thread, racing onNCollection_BaseList::PAppend.Confirmed via a real crash (not just a theoretical race): a debug build with a temporary
SIGSEGV/SIGBUShandler resolves the crash toTDocStd_Application::NewDocument -> CDF_Application::Open(which callsmyDirectory->Add(aDocument)), at a garbage-looking fault address consistent with linked-listcorruption from concurrent
PAppendcalls.Second commit, found during validation: fixing
GetApplication()'s race means every callernow genuinely shares ONE
TDocStd_Applicationinstance (as intended) — which surfaced furtherraces on that same instance's other unsynchronized state, previously masked by threads sometimes
getting different (uncontended) instances of their own:
TDocStd_Application::Resources()has the identical lazy-init race pattern asGetApplication().Resource_Manager's own maps (myRefMap/myUserMap/myExtStrMap) have no synchronization atall.
CDF_Application::myReaders/myWriters(format-name → driver maps) are read/written fromDefineFormat,ReaderFromFormat/WriterFromFormat, andReadingFormats/WritingFormatswithno locking.
Fix
XCAFApp_Application::GetApplication(): fold construction into the static local's initializer.A function-local static's dynamic initializer is guaranteed thread-safe exactly once (C++11
"magic statics", N2660), unlike the previous separate
IsNull()-guarded assignment.CDF_Directory: a privatemutable std::mutex myMutexguardsAdd/Remove/Contains/Length/IsEmpty/Last.Add()inlines the containment scan instead of calling the publicContains(), to avoid a self-deadlock on the (non-recursive) mutex.List()— used only by thefriend
CDF_DirectoryIterator— is intentionally left unguarded; closing that gap would need abigger API change (a snapshot copy) out of proportion to the races this PR fixes.
TDocStd_Application::Resources(): a mutex guards the lazy-init, same pattern as fix 1.Resource_Manager: astd::recursive_mutex(recursive becauseInteger()/Real()/ExtValue()call
Value()internally, and theint/doubleSetResource()overloads call thechar*one) guards
Save,Integer,Real,Value,ExtValue, all fourSetResourceoverloads, andboth
Findoverloads.GetMap()— a raw-reference escape hatch with no other callers in thisarea — is left unguarded, same rationale as
CDF_Directory::List()above. The new mutex membermakes the class non-copyable by default, breaking
ShapeProcess_Context.cxx's existing(pre-existing, unrelated)
new Resource_Manager(*sRC)thread-safety workaround — added anexplicit copy constructor that copies the maps under the source's lock and default-constructs a
fresh mutex for the new instance.
CDF_Application: a mutex guardsmyReaders/myWritersacross all their access points(
ReaderFromFormat/WriterFromFormatin this file;DefineFormat/ReadingFormats/WritingFormatsinTDocStd_Application.cxx, which directly touch these protected members).Testing
Minimal-module ThreadSanitizer build (
FoundationClasses+ModelingData+ModelingAlgorithms+DataExchange,RelWithDebInfo,-fsanitize=thread). A pure-C++ harness runs N threads, eachcalling
XCAFApp_Application::GetApplication()->NewDocument(...).large majority in the
GetApplication()/destructor-cascade class, the remainder directly onCDF_Directory.cxx/NCollection_BaseList.cxx(PAppend).CDF_Directory::Add/NCollection_BaseList:: PAppend, all showing the same mutex held on both sides of the reported conflict — a patternconsistent with a TSan/allocator-recycling artifact, not a genuine unaddressed race (a control
program with a trivially-correct
std::lock_guardpattern under identical TSan flags reports nosuch warning). The
GetApplication()-driven destructor cascade is gone entirely.swift testruns against a downstream Swift wrapper(SecondMouseAU/OCCTSwift) surfaced the
Resource_Manager/myReaders/myWritersraces commit 2fixes: SIGTRAP in
Resource_Manager::SetResource(viaTDocStd_Application::DefineFormat) andSIGSEGV in
TDocStd_Application::ReadingFormats, both reproducible before commit 2 and cleanacross 12 further runs after it.
scenarios exercised for XCAFDoc_ShapeTool::theAutoNaming global race: RWMesh_CafReader save/modify/restore has no locking #1387/DataExchange, XCAF - AutoNamingScope guards XCAFDoc_ShapeTool::theAutoNaming #1388).
No behavior change for the documented semantics of any of the affected classes — this only closes
the underlying data races. A separate, deeper issue (concurrent Save/Store of the same format
shares one non-reentrant cached storage-driver instance, e.g.
BinLDrivers_DocumentStorageDriver)was also found during validation but is architecturally different (a shared worker object, not a
container needing a lock) and is being tracked separately rather than folded into this PR.
Fixes #1389.