From 6d48350f6ba96e41d1caad97978be6872dd45196 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Wed, 22 Jul 2026 08:56:20 +1000 Subject: [PATCH 1/2] fix(#344): kernel + bridge fixes for the SIGSEGV that survived #341 (v1.15.6) Root-caused independently of #341's theAutoNaming fix: XCAFApp_Application:: GetApplication()'s lazy singleton init races two threads' first concurrent call (both construct an instance, race to assign the shared handle) -- confirmed via a debug build + custom SIGSEGV handler (~50% crash rate at 10 threads x 3000 barrier-synchronized rounds against stock p1) and via TSan (234 race reports at 8 threads x 200 iterations, the large majority a destructor cascade from concurrently-constructed duplicate instances). CDF_Directory::Add/Remove/Contains mutate a shared document list with zero synchronization, independently confirmed by TSan. Found during validation: fixing GetApplication() means every caller genuinely shares ONE TDocStd_Application instance for the first time, surfacing two more races on that instance's other state, previously masked by threads sometimes getting different (uncontended) instances -- TDocStd_Application::Resources() (identical lazy-init bug), Resource_ Manager's internal maps, and CDF_Application::myReaders/myWriters. All fixed in the same kernel patch (Scripts/patches/0012). 0/12 further `swift test` runs of OCCTXCAFTests reproduce any of these four races. A fifth, architecturally different crash also surfaced (BinLDrivers_ DocumentStorageDriver::Write corrupting a shared, cached, non-reentrant storage-driver instance under concurrent Save/SaveAs of the same format, ~60% crash rate once the races above stopped masking it) -- filed separately as #349 since it needs its own kernel investigation (a shared worker object, not a container needing a lock). Ships with an interim bridge-side mutex (ocafStoreMutex(), OCCTBridge_Document.mm) serializing the three OCAF save/load bridge calls, matching the #298/#341 PR1->PR2 pattern -- 0/12 further runs crash with it in place. New regression test: parallelDocumentCreate (OCCTStressTests). Filed upstream as Open-Cascade-SAS/OCCT#1389 (repro) / OCCT#1390 (fix, CI green). Co-authored-by: Claude Sonnet 5 --- CLAUDE.md | 3 +- Package.swift | 16 +- ...CAFApp_Application-thread-safety-344.patch | 365 ++++++++++++++++++ Scripts/patches/README.md | 34 ++ Scripts/repro/344-cdf-directory/README.md | 149 +++++++ .../344-cdf-directory/occt_344_barrier.cpp | 128 ++++++ .../occt_344_newdoc_only.cpp | 81 ++++ .../344-cdf-directory/occt_344_stress.cpp | 142 +++++++ Sources/OCCTBridge/src/OCCTBridge_Document.mm | 9 + Sources/OCCTBridge/src/OCCTBridge_Internal.h | 9 + .../StressConcurrencyTests.swift | 23 ++ docs/CHANGELOG.md | 76 +++- docs/thread-safety.md | 43 +++ okf/references/carried-occt-patches.md | 4 +- 14 files changed, 1071 insertions(+), 11 deletions(-) create mode 100644 Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch create mode 100644 Scripts/repro/344-cdf-directory/README.md create mode 100644 Scripts/repro/344-cdf-directory/occt_344_barrier.cpp create mode 100644 Scripts/repro/344-cdf-directory/occt_344_newdoc_only.cpp create mode 100644 Scripts/repro/344-cdf-directory/occt_344_stress.cpp diff --git a/CLAUDE.md b/CLAUDE.md index 6091fff33..76b395333 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,7 +111,8 @@ suite into these targets (each `Tests/OCCTTests/`, declared in `Package. ## Known OCCT Bugs - `BRepExtrema_ExtCC` crashes when edges are parallel — guard with `if (result.isParallel) { return result; }` before accessing points -- ~~Container-overflow in NCollection on arm64 macOS~~ — **this claim was never characterized and does not hold up.** Investigated for #341 (2026-07-21) using the #298 TSan protocol (minimal-module ThreadSanitizer build of V8_0_0_p1 + all 10 carried patches): `FoundationClasses`+`ModelingData`+`ModelingAlgorithms` stress scenarios (concurrent create/fuse/fillet, independent meshing) are clean except the already-known benign `BOPAlgo_InitMessages` lazy-init race. No NCollection race reproduced anywhere. Re-enabled the 3 suites in `Tests/OCCTStressTests/StressConcurrencyTests.swift` that had been `.disabled()` under this same unevidenced claim (some since ~v0.51.0) — 25/25 clean runs. **A real, different, previously-undetected race was found instead**: `XCAFDoc_ShapeTool::theAutoNaming`, a process-global `static bool` that `RWMesh_CafReader::fillDocument()`, `RWGltf_CafReader::fillDocument()` (a separate near-duplicate override — reachable via OBJ **and** glTF import), and `XCAFDoc_Editor::Expand()` (reentrant — recurses into itself) all save/mutate/restore with zero synchronization; `XCAFDoc_ShapeTool::AddShape` reads the same flag from every one of those and from ordinary unscoped calls too. Same failure class as #298 (unsynchronized global save/modify/restore), but logical/cosmetic (wrong auto-naming, or racy for the plain `bool` itself) rather than geometric. **Fixed upstream in v1.15.5** (`Scripts/patches/0011-*`, xcframework rebuilt): `XCAFDoc_ShapeTool::AutoNamingScope` (RAII, `std::recursive_mutex`-backed) replaces the three ad hoc save/restore call sites, and `theAutoNaming` itself is now `std::atomic` so unscoped readers (e.g. `AddShape` calls outside any of the three sites) are no longer racing on the raw storage either. Verified via TSan: 0 races across 4 runs (was 9-17/run), zero regression on the #298/independent-meshing scenarios. The interim bridge-side `meshCafMutex()` mitigation shipped in v1.15.4 was removed once this kernel patch shipped — matches the #298 PR1→PR2 pattern. Filed upstream as [OCCT#1387](https://github.com/Open-Cascade-SAS/OCCT/issues/1387) (repro) / [OCCT#1388](https://github.com/Open-Cascade-SAS/OCCT/pull/1388) (fix, draft). See [`Scripts/repro/341-meshcaf/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/341-meshcaf) for the TSan reproducer and full writeup. #341. The two hard crashes (SIGSEGV/SIGABRT) observed empirically in ~2/20 full-suite parallel `swift test` runs during this investigation remain uncharacterized and are filed separately: #344 (SIGSEGV, garbage fault address, immediately after two concurrent OBJ imports — possibly the same `theAutoNaming` race in a rarer timing window, unconfirmed, worth re-testing now that #341 shipped) and #345 (SIGABRT, essentially no localizing evidence). +- ~~Container-overflow in NCollection on arm64 macOS~~ — **this claim was never characterized and does not hold up.** Investigated for #341 (2026-07-21) using the #298 TSan protocol (minimal-module ThreadSanitizer build of V8_0_0_p1 + all 10 carried patches): `FoundationClasses`+`ModelingData`+`ModelingAlgorithms` stress scenarios (concurrent create/fuse/fillet, independent meshing) are clean except the already-known benign `BOPAlgo_InitMessages` lazy-init race. No NCollection race reproduced anywhere. Re-enabled the 3 suites in `Tests/OCCTStressTests/StressConcurrencyTests.swift` that had been `.disabled()` under this same unevidenced claim (some since ~v0.51.0) — 25/25 clean runs. **A real, different, previously-undetected race was found instead**: `XCAFDoc_ShapeTool::theAutoNaming`, a process-global `static bool` that `RWMesh_CafReader::fillDocument()`, `RWGltf_CafReader::fillDocument()` (a separate near-duplicate override — reachable via OBJ **and** glTF import), and `XCAFDoc_Editor::Expand()` (reentrant — recurses into itself) all save/mutate/restore with zero synchronization; `XCAFDoc_ShapeTool::AddShape` reads the same flag from every one of those and from ordinary unscoped calls too. Same failure class as #298 (unsynchronized global save/modify/restore), but logical/cosmetic (wrong auto-naming, or racy for the plain `bool` itself) rather than geometric. **Fixed upstream in v1.15.5** (`Scripts/patches/0011-*`, xcframework rebuilt): `XCAFDoc_ShapeTool::AutoNamingScope` (RAII, `std::recursive_mutex`-backed) replaces the three ad hoc save/restore call sites, and `theAutoNaming` itself is now `std::atomic` so unscoped readers (e.g. `AddShape` calls outside any of the three sites) are no longer racing on the raw storage either. Verified via TSan: 0 races across 4 runs (was 9-17/run), zero regression on the #298/independent-meshing scenarios. The interim bridge-side `meshCafMutex()` mitigation shipped in v1.15.4 was removed once this kernel patch shipped — matches the #298 PR1→PR2 pattern. Filed upstream as [OCCT#1387](https://github.com/Open-Cascade-SAS/OCCT/issues/1387) (repro) / [OCCT#1388](https://github.com/Open-Cascade-SAS/OCCT/pull/1388) (fix, draft). See [`Scripts/repro/341-meshcaf/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/341-meshcaf) for the TSan reproducer and full writeup. #341. The two hard crashes (SIGSEGV/SIGABRT) observed empirically in ~2/20 full-suite parallel `swift test` runs during this investigation were filed separately as #344 (SIGSEGV, root-caused below) and #345 (SIGABRT, still uncharacterized — essentially no localizing evidence). +- `XCAFApp_Application::GetApplication()` / `CDF_Directory::Add` — **#344, the SIGSEGV #341 didn't explain.** Confirmed to survive the #341 kernel fix in v1.15.5 (re-ran the parallel `swift test` loop 12× on v1.15.5: 1 more hit, same signature) — a genuinely different, previously-undetected pair of races. `GetApplication()`'s lazy singleton init (`static Handle(XCAFApp_Application) locApp; if (locApp.IsNull()) { locApp = new XCAFApp_Application; }`) is a textbook double-checked-locking-without-locking bug: two threads' first concurrent call can both construct a new instance and race to assign `locApp`. TSan shows this is the dominant defect — it produces multiple concurrently-constructed `XCAFApp_Application` instances, cascading into races across dozens of unrelated destructors as the "losing" instances are torn down mid-flight. Separately, `CDF_Directory::Add`/`Remove`/`Contains` mutate/read `myDocuments` (a plain `NCollection_List`) with zero synchronization — every `CDF_Application` is normally one process-wide instance shared by every caller, so its one `CDF_Directory` receives `Add()` from every document-creating call on every thread, racing on `NCollection_BaseList::PAppend`. The #341 TSan stress never caught either: it builds `TDocStd_Document` directly, bypassing `XCAFApp_Application`/`CDF_Application` entirely — the real bridge path (`OCCTDocumentLoadOBJ` and every other document-producing call) does not. **Fixed in v1.15.6** (`Scripts/patches/0012-*`, xcframework rebuilt): `GetApplication()` folds construction into the static local's initializer (C++11 magic statics, thread-safe exactly once); `CDF_Directory` gets a private mutex guarding `Add`/`Remove`/`Contains`/`Length`/`IsEmpty`/`Last`. Verified via a debug (`-O0 -g`) build with a temporary `SIGSEGV`/`SIGBUS` handler: stock p1 crashes ~50% of runs at 10 threads × 3000 barrier-synchronized rounds, both captured backtraces resolving to `TDocStd_Application::NewDocument -> CDF_Application::Open`; TSan (same minimal-module protocol as #298/#319/#341) goes from 234 race reports to 9, all in `CDF_Directory::Add`/`PAppend` and all showing the same mutex held on both sides of the reported conflict — consistent with a TSan/allocator-recycling artifact (a control program with a trivially-correct mutex pattern shows no such warning under identical flags), not a genuine unaddressed race; the entire `GetApplication()`-driven destructor cascade is gone entirely. Filed upstream as [OCCT#1389](https://github.com/Open-Cascade-SAS/OCCT/issues/1389) (repro) / [OCCT#1390](https://github.com/Open-Cascade-SAS/OCCT/pull/1390) (fix, two commits). **Found during validation of this same fix**: correctly making `GetApplication()` a true singleton means every caller now genuinely shares ONE `TDocStd_Application` instance — surfacing more races on that instance's OTHER unsynchronized state, previously masked by threads sometimes getting different (uncontended) instances. Repeated `swift test` runs hit a SIGTRAP in `Resource_Manager::SetResource` (via `TDocStd_Application::DefineFormat`, itself called by the common `Document.defineAllFormats()` test-setup path) and a SIGSEGV in `TDocStd_Application::ReadingFormats` iterating `CDF_Application::myReaders` concurrently with a writer. `TDocStd_Application::Resources()` has the identical lazy-init bug as `GetApplication()`; `Resource_Manager`'s maps and `CDF_Application::myReaders`/`myWriters` have zero synchronization. Also fixed in v1.15.6 (same patch): a mutex for `Resources()`'s lazy-init, a `std::recursive_mutex` for `Resource_Manager`'s accessors (added an explicit copy constructor too — the new mutex broke `ShapeProcess_Context.cxx`'s existing `new Resource_Manager(*sRC)` thread-safety workaround, whose own comment already acknowledged this exact defect: *"calling of SetResource() for one object in multiple threads causes race condition"*), and a mutex for `myReaders`/`myWriters`. 0/12 further `swift test` runs of `OCCTXCAFTests` reproduce either crash after the fix. A THIRD, architecturally different crash surfaced in the same validation (`BinLDrivers_DocumentStorageDriver::Write` corrupting a shared, cached, non-reentrant storage-driver instance under concurrent `Save`/`SaveAs` of the same format) — a shared worker object, not a container needing a lock, so the kernel fix needs its own TSan investigation; filed separately as #349. It was severe enough on its own (~60% crash rate in `OCCTXCAFTests` alone once the two races above stopped masking it) that v1.15.6 ships an **interim bridge-side mitigation** for it too: `ocafStoreMutex()` (`OCCTBridge_Document.mm`) serializes `OCCTDocumentSaveOCAF`/`OCCTDocumentSaveOCAFInPlace`/`OCCTDocumentLoadOCAF` — the same #298/#341 PR1→PR2 pattern (bridge mutex now, kernel fix later). 0/12 further `swift test` runs of `OCCTXCAFTests` crash after this mitigation (some pre-existing, unrelated test-fixture flakes remain — hardcoded non-unique temp file paths across parallel `OCAF Save/Load` tests yielding `.alreadyRetrieved`, and the already-known `Issue173AssemblySTEPTests` flake — neither a kernel bug). See [`Scripts/repro/344-cdf-directory/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/344-cdf-directory) for the full writeup. #344. - `LocOpe_SplitDrafts` throws on incompatible geometry — always wrap `Perform()` in try-catch in bridge - `BRepOffsetAPI_ThruSections` (loft) SIGSEGV'd (null deref, "Address 8") on mismatched closed profiles — `BRepFill_CompatibleWires::SameNumberByPolarMethod` over-advanced an unguarded correspondence-list iterator. It's an OS signal, so the bridge `catch(...)` cannot save it. **Fixed upstream in OCCT 8.0.0p1** (Open-Cascade-SAS/OCCT#1298, OCCTSwift #176/#178); the previously-carried `Scripts/patches/0001-*` was dropped — the current p1 xcframework has the guard natively (regression test "Loft polar-method SIGSEGV regression (#176)" passes against it). Note: `OCC_CATCH_SIGNALS` is inert in our build (no `OCC_CONVERT_SIGNALS`) — do not rely on it for signal safety; OS signals raised inside OCCT (e.g. #234) are still uncatchable in-process. - `ShapeAnalysis_FreeBounds` (backs `Shape.freeBoundsClosedWires`/`freeBoundsClosedCount`/`freeBoundsOpenWires`, and `Shape.freeBounds`) used to SIGSEGV (uncatchable) on certain shapes with multiple free-boundary components — isolated via AddressSanitizer to `connectWiresToWiresImpl`'s empty-input early return (`ShapeAnalysis_FreeBounds.cxx`) leaving its `owires` out-parameter uninitialized (null), which later crashes `NCollection_HSequence::Append`. Minimally reproducible with just two disjoint planar faces in one compound; not a simple loop-count threshold (150+ loops can be fine, 2 can crash) — depends only on whether any single component's boundary closes with zero edges left over. **Fixed upstream in v1.12.6** (`Scripts/patches/0004-*`, xcframework rebuilt): `connectWiresToWiresImpl` now initializes `owires` to an empty sequence before its early return. #310, upstream repro filed as Open-Cascade-SAS/OCCT#1376, fix as [OCCT#1377](https://github.com/Open-Cascade-SAS/OCCT/pull/1377); sibling `Standard_OutOfRange` bug in the same file, OCCT#1330, was a separate, unrelated defect in the same function — also now carried, see the `0007` entry below. diff --git a/Package.swift b/Package.swift index 5d739c729..48f610e6e 100644 --- a/Package.swift +++ b/Package.swift @@ -33,20 +33,22 @@ let occtTarget: Target = useLocalBinary name: "OCCT", path: "Libraries/OCCT.xcframework" ) - // v1.15.5 rebuild: OCCT 8.0.0p1 + our carried patches — 0001 (ShapeFix_Face guard, #263), + // v1.15.6 rebuild: OCCT 8.0.0p1 + our carried patches — 0001 (ShapeFix_Face guard, #263), // 0002 (backport of upstream OCCT#1334, #280), 0003 (fillet TopOpeBRep thread_local, #298), // 0004 (ShapeAnalysis_FreeBounds owires init, #310), 0005 (ShapeFix_Face null-Context guard // in FixPeriodicDegenerated, #317), 0006 (BRepGProp_EdgeTool adaptor NbPoles, #318), 0007 // (ShapeAnalysis_FreeBounds lwire reset, #323), 0008 (Geom_BSplineCurve O(1) // PeriodicNormalization, #323), 0009 (StepData_StepWriter split oversized string, #323), 0010 - // (Intf_Interference O(1) tangent-zone lookup + checkpointed breaker, #319), and 0011 - // (XCAFDoc_ShapeTool::AutoNamingScope, #341). + // (Intf_Interference O(1) tangent-zone lookup + checkpointed breaker, #319), 0011 + // (XCAFDoc_ShapeTool::AutoNamingScope, #341), and 0012 (XCAFApp_Application::GetApplication/ + // TDocStd_Application::Resources lazy-init races + CDF_Directory/Resource_Manager/ + // CDF_Application reader-writer map synchronization, #344). // Bump BOTH url and checksum whenever the xcframework is rebuilt, or URL-resolving consumers // silently keep the previous kernel while local sibling builds get the new one. : .binaryTarget( name: "OCCT", - url: "https://github.com/SecondMouseAU/OCCTSwift/releases/download/v1.15.5/OCCT.xcframework.zip", - checksum: "292c79cfde971751533873b0594b739edf9c80faae7c90c848282b43a685736b" + url: "https://github.com/SecondMouseAU/OCCTSwift/releases/download/v1.15.6/OCCT.xcframework.zip", + checksum: "2744457ea311cd56d94c41621fb8977daa7141ec9a4f0e4bdc0aff26b0925b61" ) // OCCTBridge is 16 Objective-C++ files / ~62K lines wrapping the OCCT header tree; SwiftPM recompiles @@ -74,8 +76,8 @@ let occtBridgeTarget: Target = useBridgeLocalBinary // the OCCT.xcframework convention above. ? .binaryTarget( name: "OCCTBridge", - url: "https://github.com/SecondMouseAU/OCCTSwift/releases/download/v1.15.5/OCCTBridge.xcframework.zip", - checksum: "5564c99c570a4da5eb5bef25d3f4cf3e30e2701185b8b53fd118a2e5548fe7eb" + url: "https://github.com/SecondMouseAU/OCCTSwift/releases/download/v1.15.6/OCCTBridge.xcframework.zip", + checksum: "dbbbae1fc580d983fd5fd19e707319f2cd01b5c8f2461aa268708778428b3ff9" ) : .target( name: "OCCTBridge", diff --git a/Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch b/Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch new file mode 100644 index 000000000..9fb8e4c60 --- /dev/null +++ b/Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch @@ -0,0 +1,365 @@ +diff --git a/src/ApplicationFramework/TKCDF/CDF/CDF_Application.cxx b/src/ApplicationFramework/TKCDF/CDF/CDF_Application.cxx +index 3496b7ec..d999f545 100644 +--- a/src/ApplicationFramework/TKCDF/CDF/CDF_Application.cxx ++++ b/src/ApplicationFramework/TKCDF/CDF/CDF_Application.cxx +@@ -472,6 +472,7 @@ void CDF_Application::Read(Standard_IStream& theIStream, + occ::handle CDF_Application::ReaderFromFormat( + const TCollection_ExtendedString& theFormat) + { ++ std::lock_guard aLock(myReadersWritersMutex); + // check map of readers + occ::handle aReader; + if (myReaders.FindFromKey(theFormat, aReader)) +@@ -531,6 +532,7 @@ occ::handle CDF_Application::ReaderFromFormat( + occ::handle CDF_Application::WriterFromFormat( + const TCollection_ExtendedString& theFormat) + { ++ std::lock_guard aLock(myReadersWritersMutex); + // check map of writers + occ::handle aDriver; + if (myWriters.FindFromKey(theFormat, aDriver)) +diff --git a/src/ApplicationFramework/TKCDF/CDF/CDF_Application.hxx b/src/ApplicationFramework/TKCDF/CDF/CDF_Application.hxx +index d7adffe4..c0e4b950 100644 +--- a/src/ApplicationFramework/TKCDF/CDF/CDF_Application.hxx ++++ b/src/ApplicationFramework/TKCDF/CDF/CDF_Application.hxx +@@ -25,6 +25,7 @@ + #include + #include + #include ++#include + + class Standard_GUID; + class CDM_Document; +@@ -233,6 +234,7 @@ protected: + NCollection_IndexedDataMap> + myReaders; + NCollection_IndexedDataMap> myWriters; ++ mutable std::mutex myReadersWritersMutex; + + private: + TCollection_ExtendedString myDefaultFolder; +diff --git a/src/ApplicationFramework/TKCDF/CDF/CDF_Directory.cxx b/src/ApplicationFramework/TKCDF/CDF/CDF_Directory.cxx +index 24276d23..c6726b61 100644 +--- a/src/ApplicationFramework/TKCDF/CDF/CDF_Directory.cxx ++++ b/src/ApplicationFramework/TKCDF/CDF/CDF_Directory.cxx +@@ -25,16 +25,23 @@ IMPLEMENT_STANDARD_RTTIEXT(CDF_Directory, Standard_Transient) + + CDF_Directory::CDF_Directory() = default; + ++// myMutex-guarded; inlined instead of calling Contains() to avoid a recursive lock. + void CDF_Directory::Add(const occ::handle& aDocument) + { +- if (!Contains(aDocument)) ++ std::lock_guard aLock(myMutex); ++ for (NCollection_List>::Iterator it(myDocuments); it.More(); it.Next()) + { +- myDocuments.Append(aDocument); ++ if (aDocument == it.Value()) ++ { ++ return; ++ } + } ++ myDocuments.Append(aDocument); + } + + void CDF_Directory::Remove(const occ::handle& aDocument) + { ++ std::lock_guard aLock(myMutex); + for (NCollection_List>::Iterator it(myDocuments); it.More(); it.Next()) + { + if (aDocument == it.Value()) +@@ -47,6 +54,7 @@ void CDF_Directory::Remove(const occ::handle& aDocument) + + bool CDF_Directory::Contains(const occ::handle& aDocument) const + { ++ std::lock_guard aLock(myMutex); + for (NCollection_List>::Iterator it(myDocuments); it.More(); it.Next()) + { + if (aDocument == it.Value()) +@@ -59,6 +67,7 @@ bool CDF_Directory::Contains(const occ::handle& aDocument) const + + int CDF_Directory::Length() const + { ++ std::lock_guard aLock(myMutex); + return myDocuments.Extent(); + } + +@@ -70,13 +79,15 @@ const NCollection_List>& CDF_Directory::List() const + + bool CDF_Directory::IsEmpty() const + { ++ std::lock_guard aLock(myMutex); + return myDocuments.IsEmpty(); + } + + occ::handle CDF_Directory::Last() + { ++ std::lock_guard aLock(myMutex); + Standard_NoSuchObject_Raise_if( +- IsEmpty(), ++ myDocuments.IsEmpty(), + "CDF_Directory::Last: the directory does not contain any document"); + return myDocuments.Last(); + } +diff --git a/src/ApplicationFramework/TKCDF/CDF/CDF_Directory.hxx b/src/ApplicationFramework/TKCDF/CDF/CDF_Directory.hxx +index 57944e6e..031df4f5 100644 +--- a/src/ApplicationFramework/TKCDF/CDF/CDF_Directory.hxx ++++ b/src/ApplicationFramework/TKCDF/CDF/CDF_Directory.hxx +@@ -23,6 +23,7 @@ + #include + #include + #include ++#include + class CDM_Document; + + //! A directory is a collection of documents. There is only one instance +@@ -62,6 +63,7 @@ private: + Standard_EXPORT const NCollection_List>& List() const; + + NCollection_List> myDocuments; ++ mutable std::mutex myMutex; + }; + + #endif // _CDF_Directory_HeaderFile +diff --git a/src/ApplicationFramework/TKLCAF/TDocStd/TDocStd_Application.cxx b/src/ApplicationFramework/TKLCAF/TDocStd/TDocStd_Application.cxx +index 176ddba3..41abc071 100644 +--- a/src/ApplicationFramework/TKLCAF/TDocStd/TDocStd_Application.cxx ++++ b/src/ApplicationFramework/TKLCAF/TDocStd/TDocStd_Application.cxx +@@ -59,6 +59,7 @@ bool TDocStd_Application::IsDriverLoaded() const + + occ::handle TDocStd_Application::Resources() + { ++ std::lock_guard aLock(myResourcesMutex); + if (myResources.IsNull()) + { + myResources = new Resource_Manager(ResourcesName()); +@@ -99,6 +100,7 @@ void TDocStd_Application::DefineFormat(const TCollection_AsciiString& + } + + // register drivers ++ std::lock_guard aLock(myReadersWritersMutex); + myReaders.Add(theFormat, theReader); + myWriters.Add(theFormat, theWriter); + } +@@ -109,6 +111,7 @@ void TDocStd_Application::ReadingFormats(NCollection_Sequence aLock(myReadersWritersMutex); + NCollection_IndexedDataMap>::Iterator anIter(myReaders); + for (; anIter.More(); anIter.Next()) +@@ -127,6 +130,7 @@ void TDocStd_Application::WritingFormats(NCollection_Sequence aLock(myReadersWritersMutex); + NCollection_IndexedDataMap>::Iterator + anIter(myWriters); + for (; anIter.More(); anIter.Next()) +diff --git a/src/ApplicationFramework/TKLCAF/TDocStd/TDocStd_Application.hxx b/src/ApplicationFramework/TKLCAF/TDocStd/TDocStd_Application.hxx +index ab272d41..aec4ab36 100644 +--- a/src/ApplicationFramework/TKLCAF/TDocStd/TDocStd_Application.hxx ++++ b/src/ApplicationFramework/TKLCAF/TDocStd/TDocStd_Application.hxx +@@ -28,6 +28,7 @@ + #include + #include + #include ++#include + + class Resource_Manager; + class CDM_Document; +@@ -325,6 +326,7 @@ public: + protected: + occ::handle myResources; + bool myIsDriverLoaded; ++ mutable std::mutex myResourcesMutex; + }; + + #endif // _TDocStd_Application_HeaderFile +diff --git a/src/DataExchange/TKXCAF/XCAFApp/XCAFApp_Application.cxx b/src/DataExchange/TKXCAF/XCAFApp/XCAFApp_Application.cxx +index 7155913c..9af39c7b 100644 +--- a/src/DataExchange/TKXCAF/XCAFApp/XCAFApp_Application.cxx ++++ b/src/DataExchange/TKXCAF/XCAFApp/XCAFApp_Application.cxx +@@ -27,11 +27,9 @@ IMPLEMENT_STANDARD_RTTIEXT(XCAFApp_Application, TDocStd_Application) + + occ::handle XCAFApp_Application::GetApplication() + { +- static occ::handle locApp; +- if (locApp.IsNull()) +- { +- locApp = new XCAFApp_Application; +- } ++ // Construct in the initializer: thread-safe exactly once (C++11 magic statics), ++ // unlike the previous separate IsNull()-guarded assignment. ++ static occ::handle locApp = new XCAFApp_Application; + return locApp; + } + +diff --git a/src/FoundationClasses/TKernel/Resource/Resource_Manager.cxx b/src/FoundationClasses/TKernel/Resource/Resource_Manager.cxx +index 06ee6c0a..26f7eebf 100644 +--- a/src/FoundationClasses/TKernel/Resource/Resource_Manager.cxx ++++ b/src/FoundationClasses/TKernel/Resource/Resource_Manager.cxx +@@ -152,6 +152,19 @@ Resource_Manager::Resource_Manager() + + //================================================================================================= + ++Resource_Manager::Resource_Manager(const Resource_Manager& theOther) ++{ ++ std::lock_guard aLock(theOther.myMutex); ++ myName = theOther.myName; ++ myRefMap = theOther.myRefMap; ++ myUserMap = theOther.myUserMap; ++ myExtStrMap = theOther.myExtStrMap; ++ myVerbose = theOther.myVerbose; ++ myInitialized = theOther.myInitialized; ++} ++ ++//================================================================================================= ++ + void Resource_Manager::Load( + const TCollection_AsciiString& thePath, + NCollection_DataMap& aMap) +@@ -322,7 +335,8 @@ static int GetLine(OSD_File& aFile, TCollection_AsciiString& aLine) + //======================================================================= + bool Resource_Manager::Save() const + { +- TCollection_AsciiString anEnvVar("CSF_"); ++ std::lock_guard aLock(myMutex); ++ TCollection_AsciiString anEnvVar("CSF_"); + anEnvVar += myName; + anEnvVar += "UserDefaults"; + +@@ -455,7 +469,8 @@ bool Resource_Manager::Save() const + + int Resource_Manager::Integer(const char* const aResourceName) const + { +- TCollection_AsciiString Result = Value(aResourceName); ++ std::lock_guard aLock(myMutex); ++ TCollection_AsciiString Result = Value(aResourceName); + if (!Result.IsIntegerValue()) + { + TCollection_AsciiString n("Value of resource `"); +@@ -473,7 +488,8 @@ int Resource_Manager::Integer(const char* const aResourceName) const + + double Resource_Manager::Real(const char* const aResourceName) const + { +- TCollection_AsciiString Result = Value(aResourceName); ++ std::lock_guard aLock(myMutex); ++ TCollection_AsciiString Result = Value(aResourceName); + if (!Result.IsRealValue()) + { + TCollection_AsciiString n("Value of resource `"); +@@ -491,7 +507,8 @@ double Resource_Manager::Real(const char* const aResourceName) const + + const char* Resource_Manager::Value(const char* const aResource) const + { +- TCollection_AsciiString Resource(aResource); ++ std::lock_guard aLock(myMutex); ++ TCollection_AsciiString Resource(aResource); + if (myUserMap.IsBound(Resource)) + { + return myUserMap(Resource).ToCString(); +@@ -510,7 +527,8 @@ const char* Resource_Manager::Value(const char* const aResource) const + + const char16_t* Resource_Manager::ExtValue(const char* const aResource) + { +- TCollection_AsciiString Resource(aResource); ++ std::lock_guard aLock(myMutex); ++ TCollection_AsciiString Resource(aResource); + if (myExtStrMap.IsBound(Resource)) + { + return myExtStrMap(Resource).ToExtString(); +@@ -532,6 +550,7 @@ const char16_t* Resource_Manager::ExtValue(const char* const aResource) + //======================================================================= + void Resource_Manager::SetResource(const char* const aResourceName, const int aValue) + { ++ std::lock_guard aLock(myMutex); + SetResource(aResourceName, TCollection_AsciiString(aValue).ToCString()); + } + +@@ -542,6 +561,7 @@ void Resource_Manager::SetResource(const char* const aResourceName, const int aV + //======================================================================= + void Resource_Manager::SetResource(const char* const aResourceName, const double aValue) + { ++ std::lock_guard aLock(myMutex); + SetResource(aResourceName, TCollection_AsciiString(aValue).ToCString()); + } + +@@ -552,10 +572,11 @@ void Resource_Manager::SetResource(const char* const aResourceName, const double + //======================================================================= + void Resource_Manager::SetResource(const char* const aResource, const char16_t* const aValue) + { +- Standard_PCharacter pStr; +- TCollection_AsciiString Resource = aResource; +- TCollection_ExtendedString ExtValue = aValue; +- TCollection_AsciiString FormatStr(ExtValue.Length() * 3 + 10, ' '); ++ std::lock_guard aLock(myMutex); ++ Standard_PCharacter pStr; ++ TCollection_AsciiString Resource = aResource; ++ TCollection_ExtendedString ExtValue = aValue; ++ TCollection_AsciiString FormatStr(ExtValue.Length() * 3 + 10, ' '); + + if (!myExtStrMap.Bind(Resource, ExtValue)) + { +@@ -577,8 +598,9 @@ void Resource_Manager::SetResource(const char* const aResource, const char16_t* + //======================================================================= + void Resource_Manager::SetResource(const char* const aResource, const char* const aValue) + { +- TCollection_AsciiString Resource = aResource; +- TCollection_AsciiString Value = aValue; ++ std::lock_guard aLock(myMutex); ++ TCollection_AsciiString Resource = aResource; ++ TCollection_AsciiString Value = aValue; + if (!myUserMap.Bind(Resource, Value)) + { + myUserMap(Resource) = Value; +@@ -589,7 +611,8 @@ void Resource_Manager::SetResource(const char* const aResource, const char* cons + + bool Resource_Manager::Find(const char* const aResource) const + { +- TCollection_AsciiString Resource(aResource); ++ std::lock_guard aLock(myMutex); ++ TCollection_AsciiString Resource(aResource); + return myUserMap.IsBound(Resource) || myRefMap.IsBound(Resource); + } + +@@ -598,6 +621,7 @@ bool Resource_Manager::Find(const char* const aResource) const + bool Resource_Manager::Find(const TCollection_AsciiString& theResource, + TCollection_AsciiString& theValue) const + { ++ std::lock_guard aLock(myMutex); + return myUserMap.Find(theResource, theValue) || myRefMap.Find(theResource, theValue); + } + +diff --git a/src/FoundationClasses/TKernel/Resource/Resource_Manager.hxx b/src/FoundationClasses/TKernel/Resource/Resource_Manager.hxx +index 08fe9f24..37947c55 100644 +--- a/src/FoundationClasses/TKernel/Resource/Resource_Manager.hxx ++++ b/src/FoundationClasses/TKernel/Resource/Resource_Manager.hxx +@@ -28,6 +28,7 @@ + #include + #include + #include ++#include + + //! Defines a resource structure and its management methods. + class Resource_Manager : public Standard_Transient +@@ -61,6 +62,10 @@ public: + const TCollection_AsciiString& theUserDefaultsDirectory, + const bool theIsVerbose = false); + ++ //! Copies the maps under theOther's lock; myMutex is not itself copyable and is ++ //! default-constructed. ++ Standard_EXPORT Resource_Manager(const Resource_Manager& theOther); ++ + //! Save the user resource structure in the specified file. + //! Creates the file if it does not exist. + Standard_EXPORT bool Save() const; +@@ -132,6 +137,7 @@ private: + NCollection_DataMap myExtStrMap; + bool myVerbose; + bool myInitialized; ++ mutable std::recursive_mutex myMutex; + }; + + #endif // _Resource_Manager_HeaderFile diff --git a/Scripts/patches/README.md b/Scripts/patches/README.md index 40c44c0ba..7c9136b87 100644 --- a/Scripts/patches/README.md +++ b/Scripts/patches/README.md @@ -235,3 +235,37 @@ Superseded the interim bridge-side mitigation (`meshCafMutex()` in `OCCTBridge_I Reported and isolated at SecondMouseAU/OCCTSwift#341; filed upstream as [Open-Cascade-SAS/OCCT#1387](https://github.com/Open-Cascade-SAS/OCCT/issues/1387) (repro), fix as [OCCT#1388](https://github.com/Open-Cascade-SAS/OCCT/pull/1388) (draft). **Retire** once the bundled OCCT includes this fix. + +## 0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch + +**Fixes the upstream OCCT crash behind [#344](https://github.com/SecondMouseAU/OCCTSwift/issues/344)** — an uncatchable SIGSEGV seen in ~1-in-10 parallel `swift test` runs right after two concurrent OBJ mesh imports, confirmed to survive the #341 kernel fix (theAutoNaming) in v1.15.5. + +Two independent, previously-undetected races, both in code the #341 TSan stress never reached — that harness (`Scripts/repro/341-meshcaf/occt_341_stress.cpp`) builds its `TDocStd_Document` directly (`new TDocStd_Document("BinXCAF")`), bypassing `XCAFApp_Application`/`CDF_Application` entirely. The real bridge path (`OCCTDocumentLoadOBJ` and every other document-producing bridge call) does not: all of them go through `XCAFApp_Application::GetApplication()->NewDocument(...)`. + +1. **`XCAFApp_Application::GetApplication()`** — a textbook double-checked-locking-without-locking bug: `static Handle(XCAFApp_Application) locApp; if (locApp.IsNull()) { locApp = new XCAFApp_Application; }`. Two threads' first concurrent call can both observe `IsNull()` and both construct a new instance, racing to assign the shared `locApp` handle. This is the dominant defect: TSan shows it produces **multiple concurrently constructed `XCAFApp_Application` instances**, whose "losing" copies are then torn down while other threads are still constructing/using a same-generation object — cascading into races across dozens of unrelated destructors (`TDF_LabelNode::Destroy`, `TCollection_ExtendedString::~`, `CDM_Document::~CDM_Document`, `NCollection_BaseList::PClear`, ...) and several ctor/dtor-vs-virtual-call ("vptr") reports, not just a leaked handle. +2. **`CDF_Directory::Add`/`Remove`/`Contains`** — every `XCAFApp_Application`/`CDF_Application` instance is normally shared process-wide (the entire point of `GetApplication()`), so its one `CDF_Directory` receives `Add()` from every document-creating call on every thread. `myDocuments` is a plain `NCollection_List` with zero synchronization: `NCollection_BaseList::PAppend` mutates `myFirst`/`myLast`/`myLength` with no locking at all. Confirmed independently by TSan (`CDF_Directory.cxx:30`, `NCollection_BaseList.cxx:45/52/53`) even setting aside race #1. + +The bridge never calls `Application->Close()` on a document (no call site in `Sources/OCCTBridge`), so `CDF_Directory::Remove` is never reached from OCCTSwift — every document ever created via the bridge accumulates in `myDocuments` for the life of the process. That's a separate, pre-existing leak, not fixed here (out of scope for #344). + +**Fix, both layers:** + +1. `XCAFApp_Application::GetApplication()`: fold construction into the static local's initializer — a C++11 "magic static" is thread-safe exactly once, unlike the previous separate `IsNull()`-guarded assignment. +2. `CDF_Directory`: a private `mutable std::mutex myMutex` guards `Add`/`Remove`/`Contains`/`Length`/`IsEmpty`/`Last`. `Add()` inlines the containment scan instead of calling the public `Contains()`, to avoid a self-deadlock on the (non-recursive) mutex. `List()` — used only by the friend `CDF_DirectoryIterator`, which nothing in OCCTSwift's bridge uses — is intentionally left unguarded; closing that gap would need a bigger API change (a snapshot copy) out of proportion to the two confirmed, reachable races this patch fixes. + +**Validation:** a debug (`-O0 -g`) build with a temporary `SIGSEGV`/`SIGBUS` signal handler (`backtrace_symbols_fd`, per the `feedback-lldb-blocked-use-signal-handler` technique) crashes ~50% of runs at 10 threads × 3000 barrier-synchronized rounds on stock p1, resolving to `TDocStd_Application::NewDocument -> CDF_Application::Open` both times — matching the `CDF_Directory::Add`/`PAppend` corruption mechanism. TSan (minimal-module build, same protocol as #298/#319/#341): stock p1 reports 234 races at 8 threads × 200 free-running iterations; the patch reduces this to 9, all directly in `CDF_Directory::Add`/`PAppend` and all showing the *same* mutex held on both sides of the reported conflict (`mutexes: write M0` on both the read and the write) — a pattern consistent with a TSan/allocator-recycling artifact rather than a genuine unaddressed race (a control program with a trivially-correct `std::lock_guard` pattern under the identical TSan flags reports no such warning). The entire `GetApplication()`-driven destructor cascade — dozens of unique signatures pre-fix — is gone entirely. + +**Second part, found during validation.** Fixing `GetApplication()`'s 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 of their own. Repeated `swift test` runs (validating the fix above) hit two more crashes in `Tests/OCCTXCAFTests`, both resolving to state on the same shared singleton: + +3. **`TDocStd_Application::Resources()`** — the identical lazy-init race pattern as `GetApplication()`: `if (myResources.IsNull()) { myResources = new Resource_Manager(...); }`, no locking. +4. **`Resource_Manager`'s own maps** (`myRefMap`/`myUserMap`/`myExtStrMap`) — no synchronization at all. Caught live: a SIGTRAP inside `Resource_Manager::SetResource`, called from `TDocStd_Application::DefineFormat` (itself called by `Document.defineAllFormats()`, a common per-test setup call many parallel XCAF tests invoke concurrently). +5. **`CDF_Application::myReaders`/`myWriters`** (format-name → driver maps) — read/written from `DefineFormat`, `ReaderFromFormat`/`WriterFromFormat`, and `ReadingFormats`/`WritingFormats` with no locking. Caught live: a SIGSEGV inside `TDocStd_Application::ReadingFormats` iterating `myReaders` while another thread's `DefineFormat` mutated it concurrently. + +**Fix, continued:** a mutex guards `Resources()`'s lazy-init (same pattern as fix 1); a `std::recursive_mutex` guards `Resource_Manager`'s public accessors (recursive because `Integer()`/`Real()`/`ExtValue()` call `Value()` internally, and the `int`/`double` `SetResource()` overloads call the `char*` one) — `GetMap()`, a raw-reference escape hatch with no callers in this area, is left unguarded, same rationale as `CDF_Directory::List()`; a mutex guards `CDF_Application::myReaders`/`myWriters` across every access point. The new `Resource_Manager` mutex member makes the class non-copyable by default, breaking `ShapeProcess_Context.cxx`'s existing (pre-existing, unrelated) `new Resource_Manager(*sRC)` thread-safety workaround (its own comment: *"Creating copy of sRC for thread safety of Resource_Manager variables... calling of SetResource() for one object in multiple threads causes race condition"* — OCCT's own prior acknowledgement of this exact defect, worked around locally rather than fixed at the source) — added an explicit copy constructor that copies the maps under the source's lock and default-constructs a fresh mutex for the new instance. + +**Validation, continued:** SIGTRAP (`Resource_Manager::SetResource`) and SIGSEGV (`TDocStd_Application::ReadingFormats`) both reproducible before this part of the patch; 0/12 further `swift test` runs of `OCCTXCAFTests` reproduce either after it. + +A third, separate crash surfaced during this same validation (`BinLDrivers_DocumentStorageDriver::Write`/`WriteSubTree` corrupting a *shared, cached, non-reentrant storage-driver instance* under concurrent `Save`/`SaveAs` of the same format) — architecturally different (a shared worker object, not a container needing a lock) and **not fixed here**; filed separately as SecondMouseAU/OCCTSwift#349 for its own dedicated investigation. + +See [`Scripts/repro/344-cdf-directory/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/344-cdf-directory) for the reproducers and full writeup. Filed upstream as [Open-Cascade-SAS/OCCT#1389](https://github.com/Open-Cascade-SAS/OCCT/issues/1389) (repro) / [OCCT#1390](https://github.com/Open-Cascade-SAS/OCCT/pull/1390) (fix, two commits). + +**Retire** once the bundled OCCT includes this fix. diff --git a/Scripts/repro/344-cdf-directory/README.md b/Scripts/repro/344-cdf-directory/README.md new file mode 100644 index 000000000..8a3975fe0 --- /dev/null +++ b/Scripts/repro/344-cdf-directory/README.md @@ -0,0 +1,149 @@ +# OCCTSwift#344 reproducer — `CDF_Directory`/`XCAFApp_Application::GetApplication()` races + +Minimal artifacts backing the root-cause of the uncatchable SIGSEGV seen in ~1-in-10 parallel +`swift test` runs, right after two concurrent OBJ mesh imports (found during the #341 +investigation, confirmed to survive the #341 kernel fix in v1.15.5 — see the #344 issue history). + +## Verdict + +**Two real, independent, previously-undetected races**, both in code the #341 TSan stress +(`Scripts/repro/341-meshcaf/occt_341_stress.cpp`) never reached — that harness builds its +`TDocStd_Document` directly (`new TDocStd_Document("BinXCAF")`), bypassing +`XCAFApp_Application`/`CDF_Application` entirely. The real bridge path +(`OCCTDocumentLoadOBJ`, `OCCTBridge_IO.mm`) does not: every document-producing bridge call goes +through `XCAFApp_Application::GetApplication()->NewDocument(...)`. + +1. **`XCAFApp_Application::GetApplication()`** (`XCAFApp_Application.cxx`) — a textbook + double-checked-locking-without-locking bug: + ```cpp + static occ::handle locApp; + if (locApp.IsNull()) + { + locApp = new XCAFApp_Application; + } + ``` + Two threads' first concurrent call can both observe `IsNull()` and both construct a new + `XCAFApp_Application`, racing to assign the shared `locApp` handle. This is the dominant + defect: TSan shows it doesn't just corrupt one handle — it produces **multiple concurrently + constructed `XCAFApp_Application` instances** (each with its own `TPrsStd_DriverTable` + registration, `CDF_Directory`, etc.), whose "losing" copies are then torn down while other + threads are still constructing/using a same-generation object, cascading into races across + dozens of unrelated destructors (`TDF_LabelNode::Destroy`, `TCollection_ExtendedString::~`, + `CDM_Document::~CDM_Document`, `NCollection_BaseList::PClear`, ...) and several + ctor/dtor-vs-virtual-call ("vptr") reports — i.e. genuine use of partially-constructed or + partially-destroyed objects, not just a leaked handle. + +2. **`CDF_Directory::Add`/`Remove`/`Contains`** (`CDF_Directory.cxx`) — every + `XCAFApp_Application`/`CDF_Application` instance is normally shared process-wide (that's the + entire point of `GetApplication()`), so its one `CDF_Directory` receives `Add()` from every + document-creating call on every thread. `myDocuments` is a plain `NCollection_List` with zero + synchronization: `NCollection_BaseList::PAppend` mutates `myFirst`/`myLast`/`myLength` with no + locking at all. Confirmed independently by TSan (`CDF_Directory.cxx:30`, + `NCollection_BaseList.cxx:45/52/53`) even setting aside race #1. + +The bridge never calls `Application->Close()` on a document (grep of `Sources/OCCTBridge` +confirms no call site), so `CDF_Directory::Remove` is never reached from OCCTSwift — every +document ever created via the bridge accumulates in `myDocuments` for the life of the process +(a separate, pre-existing leak, not fixed here since it's out of scope for #344). + +## Repro + +- `occt_344_newdoc_only.cpp` — free-running: N threads in a tight loop each call + `XCAFApp_Application::GetApplication()->NewDocument(...)`. Fast (~300-900k ops/s + uninstrumented), but OS scheduling naturally spreads out threads enough that true + instruction-level collisions on the unsynchronized critical sections are rare — did not crash + in isolation across several uninstrumented runs up to 320k total calls. +- `occt_344_barrier.cpp` — the one that matters: identical, but every thread spin-waits at a + barrier before each round's `NewDocument()` call, forcing genuine simultaneous contention every + round instead of relying on scheduling luck. **Crashes ~50% of the time** at 10 threads × 3000 + rounds against the stock (unpatched) shipped xcframework, with a debug (`-O0 -g`) build plus a + temporary `SIGSEGV`/`SIGBUS` handler (`backtrace_symbols_fd`, per the + `feedback-lldb-blocked-use-signal-handler` technique — lldb/core dumps are unavailable in the + diagnosing sandbox). Both captured crashes resolve to the same site: + ``` + TDocStd_Application::NewDocument(...) -> CDF_Application::Open(...) + ``` + matching the `CDF_Directory::Add`/`NCollection_BaseList::PAppend` corruption mechanism exactly + (one frame shows `CDF_Application::Open` duplicated — a stack-corruption artifact, not real + recursion). +- `occt_344_stress.cpp` — same shape as the #341 harness's `obj_roundtrip_unique` scenario but + going through `XCAFApp_Application::GetApplication()` (write+read OBJ round-trip, own file per + thread) rather than a hand-built `TDocStd_Document`; included for completeness, not the primary + repro (the free-running scheduling problem above applies here too). + +```bash +clang++ -std=c++17 -O0 -g \ + -I Libraries/OCCT.xcframework/macos-arm64/Headers \ + -L Libraries/OCCT.xcframework/macos-arm64 \ + occt_344_barrier.cpp -o occt_344_barrier \ + -lOCCT-macos -framework Foundation -framework AppKit -lz -lc++ +for i in $(seq 1 15); do ./occt_344_barrier 10 3000; done +``` + +TSan confirmation (minimal-module `FoundationClasses`+`ModelingData`+`ModelingAlgorithms`+ +`DataExchange`, `RelWithDebInfo`, `-fsanitize=thread -g`, matching the #298/#319/#341 protocol): +stock p1 reports both races above (plus the resulting destructor cascade) within the first +~200 rounds at 8 threads; the fix (below) closes race #1 entirely, collapsing the cascade, and +converts race #2 into properly serialized access. + +## Fix + +`Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch`: + +1. `XCAFApp_Application::GetApplication()`: fold construction into the `static` local's + initializer (C++11 "magic statics" guarantee it runs exactly once, thread-safely), instead of + a separate runtime `IsNull()` check + assignment. +2. `CDF_Directory`: a private `mutable std::mutex myMutex` guards `Add`/`Remove`/`Contains`/ + `Length`/`IsEmpty`/`Last`. `Add()` inlines the containment scan instead of calling the public + `Contains()` to avoid a self-deadlock on the (non-recursive) mutex. `List()` — used only by the + friend `CDF_DirectoryIterator`, which nothing in OCCTSwift's bridge uses — is intentionally + left unguarded; iterating the returned reference is still not safe against a concurrent + `Add()`/`Remove()`, and closing that gap would need a bigger API change (return a snapshot + copy) out of proportion to the confirmed, reachable defect this patch fixes. + +## Second part — found during validation + +Fixing `GetApplication()`'s race means every caller now genuinely shares ONE `TDocStd_Application` +instance (as intended) — which surfaced more races on that same instance's *other* unsynchronized +state, previously masked by threads sometimes getting different (uncontended) instances. Repeated +`swift test` runs (validating the fix above against `Tests/OCCTXCAFTests`) hit two more crashes: + +3. **`TDocStd_Application::Resources()`** — the identical lazy-init race as `GetApplication()`: + `if (myResources.IsNull()) { myResources = new Resource_Manager(...); }`, no locking. +4. **`Resource_Manager`'s maps** (`myRefMap`/`myUserMap`/`myExtStrMap`) — zero synchronization. + Caught live: SIGTRAP inside `Resource_Manager::SetResource`, called from + `TDocStd_Application::DefineFormat` (itself called by `Document.defineAllFormats()`, a common + per-test setup path many parallel XCAF tests invoke concurrently). +5. **`CDF_Application::myReaders`/`myWriters`** (format-name → driver maps) — read/written from + `DefineFormat`, `ReaderFromFormat`/`WriterFromFormat`, and `ReadingFormats`/`WritingFormats` with + no locking. Caught live: SIGSEGV inside `TDocStd_Application::ReadingFormats` iterating + `myReaders` while another thread's `DefineFormat` mutated it concurrently. + +Fix: a mutex guards `Resources()`'s lazy-init (same pattern as fix 1); a `std::recursive_mutex` +guards `Resource_Manager`'s public accessors (recursive because `Integer()`/`Real()`/`ExtValue()` +call `Value()` internally, and the `int`/`double` `SetResource()` overloads call the `char*` one) — +`GetMap()`, a raw-reference escape hatch with no callers in this area, is left unguarded, same +rationale as `CDF_Directory::List()`; a mutex guards `myReaders`/`myWriters` across every access +point. The new `Resource_Manager` mutex member makes the class non-copyable by default, breaking +`ShapeProcess_Context.cxx`'s existing `new Resource_Manager(*sRC)` thread-safety workaround — its +own comment already acknowledged this exact defect (*"calling of SetResource() for one object in +multiple threads causes race condition"*), worked around locally rather than fixed at the source. +Added an explicit copy constructor that copies the maps under the source's lock and +default-constructs a fresh mutex for the new instance. + +Validated: both crashes reproducible before this part of the patch; 0/12 further `swift test` runs +of `OCCTXCAFTests` reproduce either after it. + +## Known remaining issue (not fixed here) + +A third, architecturally different crash surfaced in the same validation: +`BinLDrivers_DocumentStorageDriver::Write`/`WriteSubTree` corrupts a shared, cached, non-reentrant +storage-driver instance under concurrent `Save`/`SaveAs` of the *same format* — +`CDF_Application::WriterFromFormat` creates one driver per format and reuses it for every write, +but the driver's own `Write()` isn't reentrant (instance-level scratch state like `myRelocTable` +gets corrupted by concurrent callers). This is a shared *worker object*, not a container needing a +lock, and needs its own dedicated TSan investigation — filed separately as +[SecondMouseAU/OCCTSwift#349](https://github.com/SecondMouseAU/OCCTSwift/issues/349). + +Filed upstream as [Open-Cascade-SAS/OCCT#1389](https://github.com/Open-Cascade-SAS/OCCT/issues/1389) +(repro) / [OCCT#1390](https://github.com/Open-Cascade-SAS/OCCT/pull/1390) (fix, two commits). diff --git a/Scripts/repro/344-cdf-directory/occt_344_barrier.cpp b/Scripts/repro/344-cdf-directory/occt_344_barrier.cpp new file mode 100644 index 000000000..8e6413279 --- /dev/null +++ b/Scripts/repro/344-cdf-directory/occt_344_barrier.cpp @@ -0,0 +1,128 @@ +// Barrier-synchronized variant of occt_344_newdoc_only.cpp: instead of letting threads run +// freely (where OS scheduling naturally spreads out their NewDocument() calls), every thread +// spin-waits at a barrier before each iteration so all threads call +// XCAFApp_Application::GetApplication()->NewDocument() at nearly the same instant, maximizing +// the odds of colliding inside the unsynchronized CDF_Directory::Add() -> NCollection_List:: +// Append() critical section on each round. +// +// Usage: occt_344_barrier + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +// Temporary diagnostic-only signal handler (per feedback-lldb-blocked-use-signal-handler): +// lldb attach and core dumps are both unavailable in this environment, so install our own +// handler AFTER OCCT's OSD::SetSignal would run (this repro never calls it, so we win the +// race for free) to get a function-level backtrace via backtrace_symbols_fd. Not a fix, +// debug-only scaffolding for this repro binary. +static void crashHandler(int sig) +{ + void* frames[64]; + int n = backtrace(frames, 64); + const char msg[] = "\n*** crashHandler caught signal ***\n"; + write(STDERR_FILENO, msg, sizeof(msg) - 1); + backtrace_symbols_fd(frames, n, STDERR_FILENO); + _Exit(128 + sig); +} + +static void installCrashHandler() +{ + signal(SIGSEGV, crashHandler); + signal(SIGBUS, crashHandler); +} + +static void note(const char* fmt, ...) +{ + va_list args; + va_start(args, fmt); + vfprintf(stderr, fmt, args); + va_end(args); + fprintf(stderr, "\n"); +} + +struct SpinBarrier +{ + explicit SpinBarrier(int n) + : myTotal(n), + myCount(0), + myGeneration(0) + { + } + + void Wait() + { + int gen = myGeneration.load(std::memory_order_acquire); + if (myCount.fetch_add(1, std::memory_order_acq_rel) + 1 == myTotal) + { + myCount.store(0, std::memory_order_relaxed); + myGeneration.fetch_add(1, std::memory_order_release); + } + else + { + while (myGeneration.load(std::memory_order_acquire) == gen) + { + // spin + } + } + } + + int myTotal; + std::atomic myCount; + std::atomic myGeneration; +}; + +int main(int argc, char** argv) +{ + installCrashHandler(); + int threads = argc > 1 ? atoi(argv[1]) : 16; + int rounds = argc > 2 ? atoi(argv[2]) : 5000; + + note("threads=%d rounds=%d (total=%ld lock-stepped NewDocument calls)", threads, rounds, + (long)threads * rounds); + + SpinBarrier barrier(threads); + std::atomic gOps{0}; + std::vector pool; + auto t0 = std::chrono::steady_clock::now(); + + for (int i = 0; i < threads; ++i) + { + pool.emplace_back([&, i]() { + for (int r = 0; r < rounds; ++r) + { + barrier.Wait(); // all threads cross this line together, every round + Handle(TDocStd_Document) doc; + Handle(XCAFApp_Application) app = XCAFApp_Application::GetApplication(); + app->NewDocument("MDTV-XCAF", doc); + if (doc.IsNull()) + { + note("[thread %d] NewDocument returned null at round %d", i, r); + _Exit(3); + } + Handle(XCAFDoc_ShapeTool) shapeTool = XCAFDoc_DocumentTool::ShapeTool(doc->Main()); + (void)shapeTool; + gOps++; + } + }); + } + for (auto& t : pool) + { + t.join(); + } + + auto t1 = std::chrono::steady_clock::now(); + double secs = std::chrono::duration(t1 - t0).count(); + note("done: ops=%ld elapsed=%.2fs", gOps.load(), secs); + return 0; +} diff --git a/Scripts/repro/344-cdf-directory/occt_344_newdoc_only.cpp b/Scripts/repro/344-cdf-directory/occt_344_newdoc_only.cpp new file mode 100644 index 000000000..38319b498 --- /dev/null +++ b/Scripts/repro/344-cdf-directory/occt_344_newdoc_only.cpp @@ -0,0 +1,81 @@ +// Tighter isolation of the OCCTSwift#344 lead: does NOT touch RWObj_CafReader/Writer or +// any file I/O at all. Pure stress on XCAFApp_Application::GetApplication()->NewDocument(), +// which internally does CDF_Application::Open() -> CDF_Directory::Add() -> an unsynchronized +// NCollection_List::Append(). Every OCCTSwift document-producing bridge +// call (loadOBJ, loadSTEP, loadIGES, loadGLTF, Document() constructors, ...) goes through this +// same shared, process-global CDF_Directory, and the bridge never calls Application->Close() +// (grep of Sources/OCCTBridge confirms no ->Close() call site), so myDocuments only ever +// grows -- meaning every one of these calls, for the entire lifetime of a test process, races +// against every other one on the same list. +// +// Usage: occt_344_newdoc_only + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +std::atomic gOps{0}; + +static void note(const char* fmt, ...) +{ + va_list args; + va_start(args, fmt); + vfprintf(stderr, fmt, args); + va_end(args); + fprintf(stderr, "\n"); +} + +void runNewDocumentOnly(int id, int iterations) +{ + (void)id; + for (int it = 0; it < iterations; ++it) + { + Handle(TDocStd_Document) doc; + Handle(XCAFApp_Application) app = XCAFApp_Application::GetApplication(); + app->NewDocument("MDTV-XCAF", doc); + if (doc.IsNull()) + { + note("[thread %d] NewDocument returned null at iteration %d", id, it); + _exit(3); + } + // Touch the document like the bridge does right after NewDocument, to keep the + // repro representative (XCAFDoc_DocumentTool::ShapeTool attaches an attribute). + Handle(XCAFDoc_ShapeTool) shapeTool = XCAFDoc_DocumentTool::ShapeTool(doc->Main()); + (void)shapeTool; + gOps++; + } +} + +int main(int argc, char** argv) +{ + int threads = argc > 1 ? atoi(argv[1]) : 16; + int iterations = argc > 2 ? atoi(argv[2]) : 20000; + + note("threads=%d iterations=%d (total=%ld NewDocument calls)", threads, iterations, + (long)threads * iterations); + + std::vector pool; + auto t0 = std::chrono::steady_clock::now(); + + for (int i = 0; i < threads; ++i) + { + pool.emplace_back(runNewDocumentOnly, i, iterations); + } + for (auto& t : pool) + { + t.join(); + } + + auto t1 = std::chrono::steady_clock::now(); + double secs = std::chrono::duration(t1 - t0).count(); + note("done: ops=%ld elapsed=%.2fs (%.0f ops/s)", gOps.load(), secs, gOps.load() / secs); + return 0; +} diff --git a/Scripts/repro/344-cdf-directory/occt_344_stress.cpp b/Scripts/repro/344-cdf-directory/occt_344_stress.cpp new file mode 100644 index 000000000..42a9fecca --- /dev/null +++ b/Scripts/repro/344-cdf-directory/occt_344_stress.cpp @@ -0,0 +1,142 @@ +// Multi-threaded stress harness for OCCTSwift#344: characterizing the SIGSEGV seen right +// after two concurrent OBJ mesh imports in `swift test` parallel runs, surviving the #341 +// fix (XCAFDoc_ShapeTool::theAutoNaming, now XCAFDoc_ShapeTool::AutoNamingScope). +// +// Unlike Scripts/repro/341-meshcaf/occt_341_stress.cpp (which constructs a fresh +// `new TDocStd_Document("BinXCAF")` directly, bypassing XCAFApp_Application entirely), this +// harness mirrors the REAL bridge call path (OCCTDocumentLoadOBJ, OCCTBridge_IO.mm): +// +// XCAFApp_Application::GetApplication()->NewDocument("MDTV-XCAF", doc); +// RWObj_CafReader reader; reader.SetDocument(doc); reader.Perform(path, range); +// +// `GetApplication()` returns a process-wide singleton; TDocStd_Application::NewDocument() +// calls CDF_Application::Open(), which does `myDirectory->Add(aDocument)` against that +// ONE shared CDF_Directory's `NCollection_List` -- with zero +// synchronization anywhere in CDF_Directory::Add/Remove/Contains. Every concurrent +// document creation across the whole process races on this same list. +// +// Usage: occt_344_stress + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +std::atomic gOps{0}; +std::atomic gErrors{0}; + +static void note(const char* fmt, ...) +{ + va_list args; + va_start(args, fmt); + vfprintf(stderr, fmt, args); + va_end(args); + fprintf(stderr, "\n"); +} + +// Mirrors OCCTDocumentLoadOBJ (OCCTBridge_IO.mm) exactly: goes through the shared +// XCAFApp_Application singleton, not a hand-built TDocStd_Document. +void runObjImportViaApplication(int id, int iterations, const std::string& tmpDir) +{ + for (int it = 0; it < iterations; ++it) + { + TopoDS_Shape box = BRepPrimAPI_MakeBox(10, 20, 30).Shape(); + BRepMesh_IncrementalMesh mesher(box, 0.5); + mesher.Perform(); + + std::ostringstream pathStream; + pathStream << tmpDir << "/occt344_obj_" << id << "_" << it << ".obj"; + std::string path = pathStream.str(); + + // Write via a throwaway document (write side isn't the focus, keep it simple). + { + Handle(TDocStd_Document) writeDoc; + Handle(XCAFApp_Application) app = XCAFApp_Application::GetApplication(); + app->NewDocument("MDTV-XCAF", writeDoc); + Handle(XCAFDoc_ShapeTool) shapeTool = XCAFDoc_DocumentTool::ShapeTool(writeDoc->Main()); + shapeTool->AddShape(box); + + NCollection_IndexedDataMap fileInfo; + RWObj_CafWriter writer(path.c_str()); + Message_ProgressRange range; + bool wroteOk = writer.Perform(writeDoc, fileInfo, range); + if (!wroteOk) + { + gErrors++; + note("[thread %d] OBJ write failed: %s", id, path.c_str()); + continue; + } + } + + // Read side: EXACTLY the OCCTDocumentLoadOBJ bridge sequence. + { + Handle(TDocStd_Document) readDoc; + Handle(XCAFApp_Application) app = XCAFApp_Application::GetApplication(); + app->NewDocument("MDTV-XCAF", readDoc); + if (readDoc.IsNull()) + { + gErrors++; + continue; + } + + RWObj_CafReader reader; + reader.SetDocument(readDoc); + Message_ProgressRange range; + TCollection_AsciiString filePath(path.c_str()); + bool readOk = reader.Perform(filePath, range); + if (!readOk) + { + gErrors++; + note("[thread %d] OBJ read failed: %s", id, path.c_str()); + continue; + } + Handle(XCAFDoc_ShapeTool) shapeTool = XCAFDoc_DocumentTool::ShapeTool(readDoc->Main()); + (void)shapeTool; + } + + std::remove(path.c_str()); + gOps++; + } +} + +int main(int argc, char** argv) +{ + int threads = argc > 1 ? atoi(argv[1]) : 8; + int iterations = argc > 2 ? atoi(argv[2]) : 50; + std::string tmpDir = argc > 3 ? argv[3] : "/tmp"; + + note("threads=%d iterations=%d tmpDir=%s", threads, iterations, tmpDir.c_str()); + + std::vector pool; + auto t0 = std::chrono::steady_clock::now(); + + for (int i = 0; i < threads; ++i) + { + pool.emplace_back(runObjImportViaApplication, i, iterations, tmpDir); + } + for (auto& t : pool) + { + t.join(); + } + + auto t1 = std::chrono::steady_clock::now(); + double secs = std::chrono::duration(t1 - t0).count(); + note("done: ops=%ld errors=%ld elapsed=%.2fs", gOps.load(), gErrors.load(), secs); + return gErrors.load() > 0 ? 1 : 0; +} diff --git a/Sources/OCCTBridge/src/OCCTBridge_Document.mm b/Sources/OCCTBridge/src/OCCTBridge_Document.mm index d1d9cc861..a32cdb742 100644 --- a/Sources/OCCTBridge/src/OCCTBridge_Document.mm +++ b/Sources/OCCTBridge/src/OCCTBridge_Document.mm @@ -2474,9 +2474,16 @@ void OCCTDocumentDefineFormatXmlXCAF(OCCTDocumentRef doc) { try { XmlXCAFDrivers::DefineFormat(doc->app); } catch (...) {} } +// #349: interim serialization for OCAF Save/Load — see OCCTBridge_Internal.h. +std::mutex& ocafStoreMutex() { + static std::mutex mutex; + return mutex; +} + int32_t OCCTDocumentSaveOCAF(OCCTDocumentRef doc, const char* path) { if (!doc || doc->doc.IsNull() || !path) return -1; try { + std::lock_guard storeLock(ocafStoreMutex()); TCollection_ExtendedString ePath(path, true); PCDM_StoreStatus status = doc->app->SaveAs(doc->doc, ePath); return static_cast(status); @@ -2486,6 +2493,7 @@ int32_t OCCTDocumentSaveOCAF(OCCTDocumentRef doc, const char* path) { OCCTDocumentRef OCCTDocumentLoadOCAF(const char* path, int32_t* outStatus) { if (!path) { if (outStatus) *outStatus = -1; return nullptr; } try { + std::lock_guard storeLock(ocafStoreMutex()); Handle(XCAFApp_Application) app = XCAFApp_Application::GetApplication(); // Register all format drivers @@ -2535,6 +2543,7 @@ OCCTDocumentRef OCCTDocumentLoadOCAF(const char* path, int32_t* outStatus) { int32_t OCCTDocumentSaveOCAFInPlace(OCCTDocumentRef doc) { if (!doc || doc->doc.IsNull()) return -1; try { + std::lock_guard storeLock(ocafStoreMutex()); if (!doc->doc->IsSaved()) return -1; PCDM_StoreStatus status = doc->app->Save(doc->doc); return static_cast(status); diff --git a/Sources/OCCTBridge/src/OCCTBridge_Internal.h b/Sources/OCCTBridge/src/OCCTBridge_Internal.h index 83af6c3ba..10415afd7 100644 --- a/Sources/OCCTBridge/src/OCCTBridge_Internal.h +++ b/Sources/OCCTBridge/src/OCCTBridge_Internal.h @@ -195,6 +195,15 @@ std::mutex& igesMutex(); // kernel via Scripts/patches/0011 (XCAFDoc_ShapeTool::AutoNamingScope). The // interim meshCafMutex() bridge-side lock this comment used to describe was // removed once the xcframework carried the patch. +// #349: CDF_Application::WriterFromFormat/ReaderFromFormat cache one storage/ +// retrieval driver instance per format and reuse it for every Save/Load — the +// driver's own Write()/Read() isn't reentrant (e.g. BinLDrivers_ +// DocumentStorageDriver's instance-level myRelocTable/myTypesMap scratch state), +// so two threads saving/loading the same format concurrently corrupt it +// (SIGSEGV in BinLDrivers_DocumentStorageDriver::Write/WriteSubTree, observed via +// OCCTDocumentSaveOCAF/OCCTDocumentSaveOCAFInPlace). Interim mitigation until a +// kernel fix lands — matches the #298/#341 PR1→PR2 pattern. +std::mutex& ocafStoreMutex(); // === OCCT signal handling === // diff --git a/Tests/OCCTStressTests/StressConcurrencyTests.swift b/Tests/OCCTStressTests/StressConcurrencyTests.swift index 5a27d3daa..d5a12d6a0 100644 --- a/Tests/OCCTStressTests/StressConcurrencyTests.swift +++ b/Tests/OCCTStressTests/StressConcurrencyTests.swift @@ -172,6 +172,29 @@ struct StressConcurrentCreationTests { } } +// MARK: - Concurrent Document Creation (#344) + +// Document.create()/loadOBJ/etc. all funnel through the process-wide +// XCAFApp_Application::GetApplication() singleton and its one CDF_Directory. #344 was an +// uncatchable SIGSEGV surfacing right after two concurrent OBJ imports, surviving the #341 +// theAutoNaming fix — root-caused to two independent, unsynchronized races in GetApplication()'s +// lazy singleton init and CDF_Directory::Add, fixed in the v1.15.6 kernel patch (0012). This +// regression test exercises the same singleton from many concurrent tasks. +@Suite("Stress: Concurrent Document Creation (#344)") +struct StressConcurrentDocumentCreationTests { + + @Test func parallelDocumentCreate() async { + await withTaskGroup(of: Document?.self) { group in + for _ in 0..<40 { + group.addTask { Document.create() } + } + var documents: [Document] = [] + for await d in group { if let d { documents.append(d) } } + #expect(documents.count == 40) + } + } +} + // MARK: - Sequential Determinism @Suite("Stress: Sequential Determinism") diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index c4f0b507a..0c58e1743 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -7,14 +7,86 @@ nav_order: 13 All notable changes to OCCTSwift. -## Current: v1.15.5 +## Current: v1.15.6 -**macOS / iOS (device + simulator) | OCCT 8.0.0p1 (+ #263, #280, #298, #310, #317, #318, #319, #323, #341 kernel patches)** +**macOS / iOS (device + simulator) | OCCT 8.0.0p1 (+ #263, #280, #298, #310, #317, #318, #319, #323, #341, #344 kernel patches)** --- ## Release History +### v1.15.6 (July 2026) — fix (kernel): XCAFApp_Application::GetApplication/CDF_Directory races — the SIGSEGV #341 didn't explain (#344) + +**The uncatchable SIGSEGV that survived the #341 fix.** #341 (v1.15.5) fixed a real +`XCAFDoc_ShapeTool::theAutoNaming` race, but flagged a separate empirical SIGSEGV (garbage fault +address, right after two concurrent OBJ imports) as unconfirmed — filed as #344. Re-running the +parallel `swift test` stress loop 12× against v1.15.5 hit it again once: confirmed genuinely +independent of #341's fix. + +**Root cause: two races in code the #341 TSan stress never reached.** That harness builds +`TDocStd_Document` directly (`new TDocStd_Document("BinXCAF")`), bypassing +`XCAFApp_Application`/`CDF_Application` entirely — but every real bridge call +(`OCCTDocumentLoadOBJ` and every other document-producing function) goes through +`XCAFApp_Application::GetApplication()->NewDocument(...)`. + +1. `XCAFApp_Application::GetApplication()`'s lazy singleton init is a textbook + double-checked-locking-without-locking bug — two threads' first concurrent call can both + construct a new instance and race to assign the shared handle. TSan shows this is the dominant + defect: it produces multiple concurrently-constructed `XCAFApp_Application` instances, cascading + into races across dozens of unrelated destructors as the "losing" instances are torn down + mid-flight. +2. `CDF_Directory::Add`/`Remove`/`Contains` mutate/read `myDocuments` (a plain `NCollection_List`) + with zero synchronization — every `CDF_Application` is normally one process-wide instance shared + by every caller, so its one `CDF_Directory` races on `NCollection_BaseList::PAppend` from every + document-creating call on every thread. + +**Fix**, `Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch`: +`GetApplication()` folds construction into the static local's initializer (C++11 magic statics, +thread-safe exactly once, replacing the separate `IsNull()`-guarded assignment); `CDF_Directory` +gets a private `std::mutex` guarding `Add`/`Remove`/`Contains`/`Length`/`IsEmpty`/`Last`. + +**Validation:** a debug (`-O0 -g`) build with a temporary `SIGSEGV`/`SIGBUS` signal handler +(`backtrace_symbols_fd`) crashes ~50% of runs at 10 threads × 3000 barrier-synchronized rounds on +stock p1, both captured backtraces resolving to `TDocStd_Application::NewDocument -> +CDF_Application::Open`. TSan (same minimal-module protocol as #298/#319/#341) goes from 234 race +reports to 9 — all directly in `CDF_Directory::Add`/`PAppend` and all showing the *same* mutex held +on both sides of the reported conflict, consistent with a TSan/allocator-recycling artifact rather +than a genuine unaddressed race (a control program with a trivially-correct mutex pattern shows no +such warning under identical flags). The entire `GetApplication()`-driven destructor cascade — +dozens of unique signatures pre-fix — is gone entirely. New regression test +`parallelDocumentCreate` (`OCCTStressTests`, `StressConcurrentDocumentCreationTests`) exercises +`Document.create()` from 40 concurrent tasks. + +**Found during validation of the fix above**: correctly making `GetApplication()` a true singleton +means every caller now genuinely shares ONE `TDocStd_Application` instance — surfacing more races +on that instance's *other* unsynchronized state, previously masked by threads sometimes getting +different (uncontended) instances. Repeated `swift test` runs hit a SIGTRAP in +`Resource_Manager::SetResource` (via `TDocStd_Application::DefineFormat`, called by the common +`Document.defineAllFormats()` test-setup path) and a SIGSEGV in `TDocStd_Application:: +ReadingFormats` iterating `CDF_Application::myReaders` concurrently with a writer. +`TDocStd_Application::Resources()` has the identical lazy-init bug as `GetApplication()`; +`Resource_Manager`'s maps and `CDF_Application::myReaders`/`myWriters` have zero synchronization. +Also fixed in the same patch: a mutex for `Resources()`'s lazy-init, a `std::recursive_mutex` for +`Resource_Manager`'s accessors (with an explicit copy constructor — the new mutex broke +`ShapeProcess_Context.cxx`'s existing `new Resource_Manager(*sRC)` thread-safety workaround, whose +own comment already acknowledged this exact defect), and a mutex for `myReaders`/`myWriters`. 0/12 +further `swift test` runs of `OCCTXCAFTests` reproduce either crash after the fix. + +A third, architecturally different crash surfaced in the same validation +(`BinLDrivers_DocumentStorageDriver::Write` corrupting a shared, cached, non-reentrant +storage-driver instance under concurrent `Save`/`SaveAs` of the same format) — a shared worker +object, not a container needing a lock, so the kernel fix needs its own dedicated investigation; +filed separately as #349. It was severe enough alone (~60% crash rate in `OCCTXCAFTests` once the +two races above stopped masking it) that this release also ships an **interim bridge-side +mitigation**: `ocafStoreMutex()` (`OCCTBridge_Document.mm`) serializes +`OCCTDocumentSaveOCAF`/`OCCTDocumentSaveOCAFInPlace`/`OCCTDocumentLoadOCAF` — the same #298/#341 +bridge-mutex-now/kernel-fix-later pattern. 0/12 further `swift test` runs of `OCCTXCAFTests` crash +after this mitigation. + +Reproducer at [`Scripts/repro/344-cdf-directory/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/344-cdf-directory); filed upstream as +[Open-Cascade-SAS/OCCT#1389](https://github.com/Open-Cascade-SAS/OCCT/issues/1389) (repro) / +[OCCT#1390](https://github.com/Open-Cascade-SAS/OCCT/pull/1390) (fix, two commits). #344. + ### v1.15.5 (July 2026) — fix (kernel): XCAFDoc_ShapeTool::theAutoNaming race, replacing v1.15.4's bridge mitigation (#341) **Follow-up to v1.15.4.** That release shipped an immediate bridge-side mitigation (`meshCafMutex()`) diff --git a/docs/thread-safety.md b/docs/thread-safety.md index b20d11b67..f85065499 100644 --- a/docs/thread-safety.md +++ b/docs/thread-safety.md @@ -108,6 +108,49 @@ history: 2D fillets/chamfers (`BRepFilletAPI_MakeFillet2d`) were never affected — they use the separate analytic `ChFi2d` toolkit with no such statics. +### Document creation thread safety (issues #341, #344) + +`Document.create()`, `Document.loadOBJ`/`loadSTEP`/`loadGLTF`/etc., and every other +document-producing API are **safe to call concurrently** as of v1.15.6 — no lock +needed on your side. + +Every one of these calls goes through a single process-wide `XCAFApp_Application` +singleton (`XCAFApp_Application::GetApplication()`), so two kernel fixes were needed, +both in OCCT itself, not the bridge: + +- **v1.15.5** (`Scripts/patches/0011`, issue #341): `XCAFDoc_ShapeTool::theAutoNaming`, + a process-global flag mutated by every document-tree build, raced across concurrent + OBJ/glTF import. Fixed via `XCAFDoc_ShapeTool::AutoNamingScope` (a mutex-backed RAII + scope) plus making the flag itself `std::atomic`. +- **v1.15.6** (`Scripts/patches/0012`, issue #344): an uncatchable SIGSEGV survived the + v1.15.5 fix — a genuinely different pair of races, in `GetApplication()`'s lazy + singleton init (two threads could each construct their own instance) and + `CDF_Directory::Add` (the singleton's document registry, mutated with no locking at + all). Fixed via a thread-safe static initializer and a private mutex on + `CDF_Directory`. Fixing the singleton init meant every caller genuinely shares one + `TDocStd_Application` instance for the first time (as intended), which surfaced more + races on that instance's format-registration state — `TDocStd_Application::Resources()` + (same lazy-init bug as `GetApplication()`), `Resource_Manager`'s internal maps, and + `CDF_Application::myReaders`/`myWriters` — all fixed in the same patch. + +An interim bridge-side mutex (`meshCafMutex()`, serializing every OBJ/glTF/PLY bridge +call) shipped in v1.15.4 between these two kernel fixes and was removed once v1.15.5 +made the underlying OCCT calls safe on their own — the same "bridge mitigation, then +kernel fix" pattern as #298 above. + +Concurrent `Document.saveOCAF`/`saveOCAFInPlace`/`loadOCAF` of the **same target format** +is a separate issue (#349): `CDF_Application::WriterFromFormat`/`ReaderFromFormat` cache +one storage/retrieval driver instance per format and reuse it for every call, but the +driver's own `Write()`/`Read()` isn't reentrant — its instance-level scratch state (e.g. +`BinLDrivers_DocumentStorageDriver`'s `myRelocTable`) corrupts under two concurrent +callers. **v1.15.6 ships an interim bridge-side mitigation** (`ocafStoreMutex()` in +`OCCTBridge_Document.mm`, serializing all three OCAF save/load bridge calls) — the same +"bridge mitigation, then kernel fix" pattern as #298/#341 above. The underlying OCCT +non-reentrancy is **not yet fixed in the kernel**; that needs its own dedicated TSan +investigation, tracked separately in #349. Plain shape-format I/O (STEP/IGES/BREP/OBJ/ +glTF) is unaffected — this only covers the three OCAF (`.bcaf`/`.xcaf`-style binary/XML +document) persistence entry points. + ## Performance The mutex overhead is ~1µs per lock/unlock. Typical OCCT operations take 0.1ms-10s. The serialization cost is negligible for all practical workflows. diff --git a/okf/references/carried-occt-patches.md b/okf/references/carried-occt-patches.md index 962f34433..e1b77af66 100644 --- a/okf/references/carried-occt-patches.md +++ b/okf/references/carried-occt-patches.md @@ -4,7 +4,7 @@ title: Carried OCCT source patches resource: https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/patches tags: [occt, patches, upstream, thread-safety, kernel] description: Upstream-bound OCCT fixes OCCTSwift carries in its xcframework build until they ship in an OCCT release. -timestamp: 2026-07-20 +timestamp: 2026-07-22 --- # Carried OCCT source patches @@ -32,6 +32,8 @@ OCCT's own `.clang-format`, and OCCT's terse comment style — not OCCTSwift's. | `0008-Geom_BSplineCurve-…-323` | `PeriodicNormalization` infinite loop / O(N) hang on far-out-of-range parameters ([#323](https://github.com/SecondMouseAU/OCCTSwift/issues/323) audit) | [OCCT#1288](https://github.com/Open-Cascade-SAS/OCCT/issues/1288) (repro) → [OCCT#1329](https://github.com/Open-Cascade-SAS/OCCT/pull/1329) (merged, stable) | bundled OCCT moves past that commit | | `0009-StepData_StepWriter-…-323` | `AddString` infinite loop writing a single unbroken raw string longer than the 72-char line buffer ([#323](https://github.com/SecondMouseAU/OCCTSwift/issues/323) audit) | [OCCT#1318](https://github.com/Open-Cascade-SAS/OCCT/pull/1318) (open, by an OCCT maintainer — pinned to a commit) | bundled OCCT includes the fix | | `0010-Intf_Interference-…-319` | `isSelfIntersecting(hardTimeout:)` couldn't interrupt an unbounded self-interference search — O(n)-per-call tangent-zone point access plus no checkpoint below `CheckFaceSelfIntersection` ([#319](https://github.com/SecondMouseAU/OCCTSwift/issues/319)) | [OCCT#1385](https://github.com/Open-Cascade-SAS/OCCT/issues/1385) (repro) → **[OCCT#1386](https://github.com/Open-Cascade-SAS/OCCT/pull/1386)** (our fix PR, CI green, ready for review) | bundled OCCT includes the fix | +| `0011-XCAFDoc_ShapeTool-AutoNamingScope-341` | `XCAFDoc_ShapeTool::theAutoNaming` process-global race across concurrent OBJ/glTF import and PLY/OBJ/glTF export ([#341](https://github.com/SecondMouseAU/OCCTSwift/issues/341)) | [OCCT#1387](https://github.com/Open-Cascade-SAS/OCCT/issues/1387) (repro) → **[OCCT#1388](https://github.com/Open-Cascade-SAS/OCCT/pull/1388)** (our fix PR, draft) | bundled OCCT includes the fix | +| `0012-CDF_Directory-XCAFApp_Application-thread-safety-344` | `XCAFApp_Application::GetApplication()`/`TDocStd_Application::Resources()` lazy-singleton races + unsynchronized `CDF_Directory`/`Resource_Manager`/`CDF_Application` reader-writer maps — SIGSEGV surviving the #341 fix ([#344](https://github.com/SecondMouseAU/OCCTSwift/issues/344); a related but architecturally different driver-reentrancy crash found in the same validation is tracked separately as [#349](https://github.com/SecondMouseAU/OCCTSwift/issues/349)) | [OCCT#1389](https://github.com/Open-Cascade-SAS/OCCT/issues/1389) (repro) → **[OCCT#1390](https://github.com/Open-Cascade-SAS/OCCT/pull/1390)** (our fix PR, 2 commits, CI pending) | bundled OCCT includes the fix | **#298 status:** we ship the fix now via patch `0003` (xcframework rebuilt in v1.12.3); we keep carrying it — and building our own xcframework — **until an upstream OCCT From ce719531c486fad55d950533ae1720748ec3a663 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Wed, 22 Jul 2026 08:58:56 +1000 Subject: [PATCH 2/2] fix(#344): explicitly initialize Standard_Transient in Resource_Manager's copy ctor Fixes GCC -Wextra ("base class should be explicitly initialized in the copy constructor") caught by the upstream OCCT PR's CI matrix. No behavior change (the base was already default-constructed implicitly); doesn't require an xcframework rebuild. --- ...CAFApp_Application-thread-safety-344.patch | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch b/Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch index 9fb8e4c60..e15aa3a92 100644 --- a/Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch +++ b/Scripts/patches/0012-CDF_Directory-XCAFApp_Application-thread-safety-344.patch @@ -197,14 +197,15 @@ index 7155913c..9af39c7b 100644 } diff --git a/src/FoundationClasses/TKernel/Resource/Resource_Manager.cxx b/src/FoundationClasses/TKernel/Resource/Resource_Manager.cxx -index 06ee6c0a..26f7eebf 100644 +index 06ee6c0a..e29e6751 100644 --- a/src/FoundationClasses/TKernel/Resource/Resource_Manager.cxx +++ b/src/FoundationClasses/TKernel/Resource/Resource_Manager.cxx -@@ -152,6 +152,19 @@ Resource_Manager::Resource_Manager() +@@ -152,6 +152,20 @@ Resource_Manager::Resource_Manager() //================================================================================================= +Resource_Manager::Resource_Manager(const Resource_Manager& theOther) ++ : Standard_Transient() +{ + std::lock_guard aLock(theOther.myMutex); + myName = theOther.myName; @@ -220,7 +221,7 @@ index 06ee6c0a..26f7eebf 100644 void Resource_Manager::Load( const TCollection_AsciiString& thePath, NCollection_DataMap& aMap) -@@ -322,7 +335,8 @@ static int GetLine(OSD_File& aFile, TCollection_AsciiString& aLine) +@@ -322,7 +336,8 @@ static int GetLine(OSD_File& aFile, TCollection_AsciiString& aLine) //======================================================================= bool Resource_Manager::Save() const { @@ -230,7 +231,7 @@ index 06ee6c0a..26f7eebf 100644 anEnvVar += myName; anEnvVar += "UserDefaults"; -@@ -455,7 +469,8 @@ bool Resource_Manager::Save() const +@@ -455,7 +470,8 @@ bool Resource_Manager::Save() const int Resource_Manager::Integer(const char* const aResourceName) const { @@ -240,7 +241,7 @@ index 06ee6c0a..26f7eebf 100644 if (!Result.IsIntegerValue()) { TCollection_AsciiString n("Value of resource `"); -@@ -473,7 +488,8 @@ int Resource_Manager::Integer(const char* const aResourceName) const +@@ -473,7 +489,8 @@ int Resource_Manager::Integer(const char* const aResourceName) const double Resource_Manager::Real(const char* const aResourceName) const { @@ -250,7 +251,7 @@ index 06ee6c0a..26f7eebf 100644 if (!Result.IsRealValue()) { TCollection_AsciiString n("Value of resource `"); -@@ -491,7 +507,8 @@ double Resource_Manager::Real(const char* const aResourceName) const +@@ -491,7 +508,8 @@ double Resource_Manager::Real(const char* const aResourceName) const const char* Resource_Manager::Value(const char* const aResource) const { @@ -260,7 +261,7 @@ index 06ee6c0a..26f7eebf 100644 if (myUserMap.IsBound(Resource)) { return myUserMap(Resource).ToCString(); -@@ -510,7 +527,8 @@ const char* Resource_Manager::Value(const char* const aResource) const +@@ -510,7 +528,8 @@ const char* Resource_Manager::Value(const char* const aResource) const const char16_t* Resource_Manager::ExtValue(const char* const aResource) { @@ -270,7 +271,7 @@ index 06ee6c0a..26f7eebf 100644 if (myExtStrMap.IsBound(Resource)) { return myExtStrMap(Resource).ToExtString(); -@@ -532,6 +550,7 @@ const char16_t* Resource_Manager::ExtValue(const char* const aResource) +@@ -532,6 +551,7 @@ const char16_t* Resource_Manager::ExtValue(const char* const aResource) //======================================================================= void Resource_Manager::SetResource(const char* const aResourceName, const int aValue) { @@ -278,7 +279,7 @@ index 06ee6c0a..26f7eebf 100644 SetResource(aResourceName, TCollection_AsciiString(aValue).ToCString()); } -@@ -542,6 +561,7 @@ void Resource_Manager::SetResource(const char* const aResourceName, const int aV +@@ -542,6 +562,7 @@ void Resource_Manager::SetResource(const char* const aResourceName, const int aV //======================================================================= void Resource_Manager::SetResource(const char* const aResourceName, const double aValue) { @@ -286,7 +287,7 @@ index 06ee6c0a..26f7eebf 100644 SetResource(aResourceName, TCollection_AsciiString(aValue).ToCString()); } -@@ -552,10 +572,11 @@ void Resource_Manager::SetResource(const char* const aResourceName, const double +@@ -552,10 +573,11 @@ void Resource_Manager::SetResource(const char* const aResourceName, const double //======================================================================= void Resource_Manager::SetResource(const char* const aResource, const char16_t* const aValue) { @@ -302,7 +303,7 @@ index 06ee6c0a..26f7eebf 100644 if (!myExtStrMap.Bind(Resource, ExtValue)) { -@@ -577,8 +598,9 @@ void Resource_Manager::SetResource(const char* const aResource, const char16_t* +@@ -577,8 +599,9 @@ void Resource_Manager::SetResource(const char* const aResource, const char16_t* //======================================================================= void Resource_Manager::SetResource(const char* const aResource, const char* const aValue) { @@ -314,7 +315,7 @@ index 06ee6c0a..26f7eebf 100644 if (!myUserMap.Bind(Resource, Value)) { myUserMap(Resource) = Value; -@@ -589,7 +611,8 @@ void Resource_Manager::SetResource(const char* const aResource, const char* cons +@@ -589,7 +612,8 @@ void Resource_Manager::SetResource(const char* const aResource, const char* cons bool Resource_Manager::Find(const char* const aResource) const { @@ -324,7 +325,7 @@ index 06ee6c0a..26f7eebf 100644 return myUserMap.IsBound(Resource) || myRefMap.IsBound(Resource); } -@@ -598,6 +621,7 @@ bool Resource_Manager::Find(const char* const aResource) const +@@ -598,6 +622,7 @@ bool Resource_Manager::Find(const char* const aResource) const bool Resource_Manager::Find(const TCollection_AsciiString& theResource, TCollection_AsciiString& theValue) const {