diff --git a/CLAUDE.md b/CLAUDE.md index ccc5bac..713bcc9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -116,6 +116,7 @@ suite into these targets (each `Tests/OCCTTests/`, declared in `Package. - `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. - **Uncaught-exception SIGABRT — #345, the companion crash to #344.** #345 was filed alongside #344 from the same investigation with "essentially no localizing evidence" (just `exited with unexpected signal code 6`, no test name, no backtrace). An audit (prompted by #344's own finding that fixing one race exposes the next) turned up a plausible, distinct root cause: OCCT's `gp_Dir` constructor throws `Standard_ConstructionError` for a zero-length (or near-zero) direction/normal vector (`Libraries/occt-src/src/FoundationClasses/TKMath/gp/gp_Dir.hxx`); so does `Geom_Direction`'s. **49 public bridge functions** across `Sources/OCCTBridge/src/{OCCTBridge_Curve3D,OCCTBridge_Geom2d,OCCTBridge_Modeling,OCCTBridge_Spatial,OCCTBridge_Surface,OCCTBridge_Topology,OCCTBridge_Visualization}.mm` constructed `gp_Dir`/`gp_Ax1`/`gp_Ax2`/`gp_Ax3`/`Geom_Direction` directly from caller-supplied doubles (or called a `Geom_Surface`/`Geom_Curve` `D0`/`D1`/`D2` derivative evaluator, or a `GeomEval_*Surface` constructor — same throw risk for degenerate radius/amplitude/omega) with **no try/catch anywhere in the call chain** — e.g. `OCCTSurfaceD1`/`OCCTSurfaceD2` had none, right next to `OCCTSurfaceGetNormal` two lines below, which already did (an easy-to-miss inconsistency). An uncaught C++ exception crossing the bridge's `extern "C"`-ish boundary into Swift-generated call frames has no matching unwind personality routine — a guaranteed `std::terminate()` → `abort()` (SIGABRT), and it would leave almost no diagnostic trail, matching #345's profile exactly. **Fixed**: wrapped all 49 in `try { ... } catch (...) { }` matching each file's existing idiom; the 3 functions returning a `_Nonnull` pointer (`OCCTAxis1PlacementCreate`, `OCCTAxis2PlacementCreate`, `OCCTOBBCreate`) fall back to constructing a valid default axis instead of `nullptr`, since returning null from a `_Nonnull` contract would just relocate the crash to the next dereference. Two confirmed false positives were left untouched: `computePlaneForPoints` (`OCCTBridge_AIS.mm`) and `buildTrsf3D` (two separately-defined `static` helpers, one each in `OCCTBridge_Curve3D.mm`/`OCCTBridge_Surface.mm`) are both already protected by a `try` in their sole caller. New regression tests (`Tests/OCCTStressTests/StressNullInvalidTests.swift`: `mirrorAxisZeroDirection`, `mirrorPlaneZeroNormal`, `geomDirectionZeroVector`) exercise a zero vector through the public Swift API. **Validation**: 70 additional full-suite `swift test` runs (4419-4422 tests each, ~309,540 individual test executions) — zero SIGABRT recurrence, zero crashes of any kind (a handful of already-known, unrelated assertion-level test-fixture flakes remain). This is a bridge-only fix (no OCCT kernel change, no xcframework rebuild) — not an OCCT bug, so nothing filed upstream. #345's own bar for confident closure was "100+ runs with no recurrence"; 70 clean runs plus a fix matching the exact crash mechanism is short of that literal bar but is the strongest evidence gathered to date — recommend closing #345, reopen if it recurs. - `PCDM_StorageDriver`/`PCDM_Reader` (backs `CDF_Application::WriterFromFormat`/`ReaderFromFormat`) — **#349, the third crash found validating #344.** `CDF_Application` caches one storage/retrieval driver instance per format and hands the same instance back to every `Store()`/`Retrieve()` call for that format, including concurrently from different threads/documents. But `BinLDrivers_DocumentStorageDriver` (and every other format's driver, structurally) is not reentrant: `Write()`/`Read()` mutate instance-level scratch state (`myRelocTable`, `myTypesMap`, and more) with zero synchronization, so two threads' concurrent `Write()` calls corrupt each other — reliable SIGSEGV (`BinMDF_ADriverTable::AssignIds` on a torn `myTypesMap`, reached via `BinLDrivers_DocumentStorageDriver::Write -> FirstPass -> CDF_StoreList::Store -> TDocStd_Application::SaveAs`). TSan (same minimal-module protocol as #298/#319/#341/#344) confirms directly: 136 race warnings + a live SIGSEGV in one run (8 threads × 25 rounds) on stock kernel. **Fixed in v1.15.9** (`Scripts/patches/0014-*`, xcframework rebuilt): `PCDM_StorageDriver`/`PCDM_Reader` each get a `mutable std::mutex` + `Mutex()` accessor — every concrete format driver subclass inherits the guard for free, zero subclass changes needed — held at the three call sites that invoke a cached, possibly-shared driver (`CDF_StoreList::Store`, `CDF_Application::Retrieve`, `CDF_Application::Read`). Considered making the driver itself reentrant (eliminating the shared scratch state) but rejected as impractical: TSan shows essentially the entire driver object is scratch state for one `Write()` call, plus a nested shared `BinMDF_ADriverTable` with its own internal mutation — fixing that properly would mean a sweeping signature change to every format driver's private helpers, out of proportion to a "minimal, surgical" upstream PR. Verified: TSan race warnings 136 → 0 (confirmed again at 10×200), crash → clean exit across repeated runs; full production xcframework rebuild + `swift test --filter OCAFSaveLoadBinaryTests`/`OCCTXCAFTests` clean, 3× full `swift test` (4423 tests) clean. The interim bridge-side mitigation (`ocafStoreMutex()`, shipped v1.15.6) stays in place — same PR1→PR2 pattern as #298/#341/#344. **Found during validation of this fix**: `CDM_Application::myMetaDataLookUpTable` (a plain, unsynchronized `NCollection_DataMap`, one per `CDM_Application`) races too — same failure class as `CDF_Directory::myDocuments` (#344) and `theAutoNaming` (#341), out of scope for #349, filed separately as #353. See [`Scripts/repro/349-ocaf-driver-reentrancy/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/349-ocaf-driver-reentrancy) for the full writeup. Filed upstream as [Open-Cascade-SAS/OCCT#1393](https://github.com/Open-Cascade-SAS/OCCT/issues/1393) (repro) / [OCCT#1394](https://github.com/Open-Cascade-SAS/OCCT/pull/1394) (fix, CI green on all platforms). #349. +- `CDM_Application::myMetaDataLookUpTable`/`CDM_MetaData` — **#353, the race #349's own fix unmasked.** `CDM_Application::myMetaDataLookUpTable` is shared process-wide (one `CDM_Application` singleton, since #344) with zero synchronization: `CDM_MetaData::LookUp()`'s map mutation (called from `CDF_FWOSDriver::MetaData`/`CreateMetaData`, `XmlLDrivers_DocumentRetrievalDriver::Read`, `PCDM_ReferenceIterator::MetaData`), `CDM_Document::SetMetaData()`'s whole-table iteration on every save, and each `CDM_MetaData`'s own `myIsRetrieved`/`myDocument` fields (mutated by `SetDocument`/`UnsetDocument`, read by `IsRetrieved`/`Document`) all race independently with no guard at all. TSan (same minimal-module protocol as #298/#319/#341/#344/#349) confirmed the exact trace quoted in the issue: `CDM_Document::SetMetaData()`'s map-iteration loop reading `IsRetrieved()` — still inside #349's own per-driver lock, but that lock has no relationship to a *different* thread's document destructor — racing `~CDM_Document() -> CDM_MetaData::UnsetDocument()` tearing down an unrelated document's metadata entry on another thread. 1 confirmed race + SIGABRT (exit 134) on stock #349-fixed kernel. **Fixed in v1.15.10** (`Scripts/patches/0015-*`, xcframework rebuilt): follows the established "lock the shared resource, don't restructure the subsystem" precedent (#341's atomic bool, #344's `CDF_Directory` mutex, #349's per-driver mutex) — the map's "bind once, reuse forever, share across all documents" caching design is load-bearing, not incidental scratch state. Two independent locks: `CDM_Application` gets a `mutable std::mutex` + accessor threaded through `CDM_MetaData::LookUp()`'s two overloads (now take the mutex as an explicit parameter) and `CDM_Document::SetMetaData()`'s iteration; `CDM_MetaData` gets its own private `mutable std::mutex` guarding `myIsRetrieved`/`myDocument`, independent of the table lock, since two already-bound `CDM_MetaData` objects can still race on those fields. No changes to any format-specific driver subclass. Verified: TSan race count 1 (+SIGABRT) → 0, clean exit across 5 runs (8×25, 10×60, 3×8×40); full production xcframework rebuild + `swift test --filter OCAFSaveLoadBinaryTests`/`OCCTXCAFTests` clean, 3× full `swift test` (4423 tests) clean. **Not changed**: `CDM_MetaData::myDocumentVersion` (reached via `CDM_Reference.cxx` and `CDM_Application::SetDocumentVersion`) has the identical unguarded-mutable-field shape as the fixed fields, but on the document-*reference* resolution path rather than save/close — not TSan-observed in any run (the repro doesn't exercise cross-document references), flagged as a plausible sibling for a future pass, not fixed here or filed as a separate issue (purely speculative, pattern-matched risk, not an observed defect). See [`Scripts/repro/353-cdm-metadata-lookup-table/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/353-cdm-metadata-lookup-table) for the full writeup. Filed upstream as [Open-Cascade-SAS/OCCT#1396](https://github.com/Open-Cascade-SAS/OCCT/issues/1396) (repro) / [OCCT#1397](https://github.com/Open-Cascade-SAS/OCCT/pull/1397) (fix, CI green on all platforms). #353. - `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 d6165c5..76b6d6b 100644 --- a/Package.swift +++ b/Package.swift @@ -33,7 +33,7 @@ let occtTarget: Target = useLocalBinary name: "OCCT", path: "Libraries/OCCT.xcframework" ) - // v1.15.9 rebuild: OCCT 8.0.0p1 + our carried patches — 0001 (ShapeFix_Face guard, #263), + // v1.15.11 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 @@ -43,14 +43,15 @@ let occtTarget: Target = useLocalBinary // (XCAFDoc_ShapeTool::AutoNamingScope, #341), 0012 (XCAFApp_Application::GetApplication/ // TDocStd_Application::Resources lazy-init races + CDF_Directory/Resource_Manager/ // CDF_Application reader-writer map synchronization, #344), 0013 - // (ShapeUpgrade_UnifySameDomain null-pcurve dereference guards, #348), and 0014 - // (PCDM_StorageDriver/PCDM_Reader driver-instance reentrancy mutex, #349). + // (ShapeUpgrade_UnifySameDomain null-pcurve dereference guards, #348), 0014 + // (PCDM_StorageDriver/PCDM_Reader driver-instance reentrancy mutex, #349), and 0015 + // (CDM_Application::myMetaDataLookUpTable + CDM_MetaData field mutexes, #353). // 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.9/OCCT.xcframework.zip", - checksum: "61937268c17b19a8628d7494cb05151d17f0c2908160b8e0b999d9588aa74e33" + url: "https://github.com/SecondMouseAU/OCCTSwift/releases/download/v1.15.11/OCCT.xcframework.zip", + checksum: "acbe937e76ff8e53b68a4a352b76c25fb4b7fcf09a53e5d26235cba997aef94e" ) // OCCTBridge is 16 Objective-C++ files / ~62K lines wrapping the OCCT header tree; SwiftPM recompiles diff --git a/Scripts/patches/0015-CDM_Application-metadata-lookup-table-mutex-353.patch b/Scripts/patches/0015-CDM_Application-metadata-lookup-table-mutex-353.patch new file mode 100644 index 0000000..12e7fea --- /dev/null +++ b/Scripts/patches/0015-CDM_Application-metadata-lookup-table-mutex-353.patch @@ -0,0 +1,307 @@ +diff --git a/src/ApplicationFramework/TKCDF/CDF/CDF_FWOSDriver.cxx b/src/ApplicationFramework/TKCDF/CDF/CDF_FWOSDriver.cxx +index 1d992174..c832f5b2 100644 +--- a/src/ApplicationFramework/TKCDF/CDF/CDF_FWOSDriver.cxx ++++ b/src/ApplicationFramework/TKCDF/CDF/CDF_FWOSDriver.cxx +@@ -43,8 +43,10 @@ static void PutSlash(TCollection_ExtendedString& anXSTRING) + //================================================================================================= + + CDF_FWOSDriver::CDF_FWOSDriver( +- NCollection_DataMap>& theLookUpTable) +- : myLookUpTable(&theLookUpTable) ++ NCollection_DataMap>& theLookUpTable, ++ std::mutex& theMutex) ++ : myLookUpTable(&theLookUpTable), ++ myMutex(&theMutex) + { + } + +@@ -102,7 +104,7 @@ occ::handle CDF_FWOSDriver::MetaData(const TCollection_ExtendedStr + const TCollection_ExtendedString& /*aVersion*/) + { + TCollection_ExtendedString p = Concatenate(aFolder, aName); +- return CDM_MetaData::LookUp(*myLookUpTable, aFolder, aName, p, p, UTL::IsReadOnly(p)); ++ return CDM_MetaData::LookUp(*myLookUpTable, *myMutex, aFolder, aName, p, p, UTL::IsReadOnly(p)); + } + + //================================================================================================= +@@ -112,6 +114,7 @@ occ::handle CDF_FWOSDriver::CreateMetaData( + const TCollection_ExtendedString& aFileName) + { + return CDM_MetaData::LookUp(*myLookUpTable, ++ *myMutex, + aDocument->RequestedFolder(), + aDocument->RequestedName(), + Concatenate(aDocument->RequestedFolder(), aDocument->RequestedName()), +diff --git a/src/ApplicationFramework/TKCDF/CDF/CDF_FWOSDriver.hxx b/src/ApplicationFramework/TKCDF/CDF/CDF_FWOSDriver.hxx +index 13eabf23..d10da401 100644 +--- a/src/ApplicationFramework/TKCDF/CDF/CDF_FWOSDriver.hxx ++++ b/src/ApplicationFramework/TKCDF/CDF/CDF_FWOSDriver.hxx +@@ -22,6 +22,7 @@ + #include + #include + #include ++#include + + class TCollection_ExtendedString; + class CDM_MetaData; +@@ -33,9 +34,11 @@ class CDF_FWOSDriver : public CDF_MetaDataDriver + public: + //! Initializes the MetaDatadriver connected to specified look-up table. + //! Note that the created driver will keep reference to the table, +- //! thus it must have life time longer than this object. ++ //! thus it must have life time longer than this object. theMutex must ++ //! guard theLookUpTable (CDM_Application::MetaDataLookUpTableMutex()). + Standard_EXPORT CDF_FWOSDriver( +- NCollection_DataMap>& theLookUpTable); ++ NCollection_DataMap>& theLookUpTable, ++ std::mutex& theMutex); + + //! indicate whether a file exists corresponding to the folder and the name + Standard_EXPORT bool Find(const TCollection_ExtendedString& aFolder, +@@ -75,6 +78,7 @@ private: + + private: + NCollection_DataMap>* myLookUpTable; ++ std::mutex* myMutex; + }; + + #endif // _CDF_FWOSDriver_HeaderFile +diff --git a/src/ApplicationFramework/TKCDF/CDM/CDM_Application.hxx b/src/ApplicationFramework/TKCDF/CDM/CDM_Application.hxx +index de8fe871..7b48ae43 100644 +--- a/src/ApplicationFramework/TKCDF/CDM/CDM_Application.hxx ++++ b/src/ApplicationFramework/TKCDF/CDM/CDM_Application.hxx +@@ -24,6 +24,7 @@ + #include + #include + #include ++#include + + class CDM_MetaData; + class CDM_Document; +@@ -66,6 +67,9 @@ public: + occ::handle>& + MetaDataLookUpTable(); + ++ //! Guards MetaDataLookUpTable(), shared across threads/calls. ++ std::mutex& MetaDataLookUpTableMutex() const { return myMetaDataLookUpTableMutex; } ++ + //! Dumps the content of me into the stream + Standard_EXPORT void DumpJson(Standard_OStream& theOStream, int theDepth = -1) const; + +@@ -95,6 +99,7 @@ private: + + occ::handle myMessenger; + NCollection_DataMap> myMetaDataLookUpTable; ++ mutable std::mutex myMetaDataLookUpTableMutex; + }; + + #endif // _CDM_Application_HeaderFile +diff --git a/src/ApplicationFramework/TKCDF/CDM/CDM_Document.cxx b/src/ApplicationFramework/TKCDF/CDM/CDM_Document.cxx +index 5da83030..d1e8b05f 100644 +--- a/src/ApplicationFramework/TKCDF/CDM/CDM_Document.cxx ++++ b/src/ApplicationFramework/TKCDF/CDM/CDM_Document.cxx +@@ -28,6 +28,7 @@ + #include + #include + #include ++#include + #include + #include + #include +@@ -479,6 +480,8 @@ void CDM_Document::SetMetaData(const occ::handle& aMetaData) + aMetaData->SetDocument(this); + + // Update the document referencing this MetaData: ++ // MetaDataLookUpTable() is shared across threads/calls; guard the iteration. ++ std::lock_guard aLookUpTableLock(Application()->MetaDataLookUpTableMutex()); + NCollection_DataMap>::Iterator it( + Application()->MetaDataLookUpTable()); + for (; it.More(); it.Next()) +diff --git a/src/ApplicationFramework/TKCDF/CDM/CDM_MetaData.cxx b/src/ApplicationFramework/TKCDF/CDM/CDM_MetaData.cxx +index 5986056c..d9f022d3 100644 +--- a/src/ApplicationFramework/TKCDF/CDM/CDM_MetaData.cxx ++++ b/src/ApplicationFramework/TKCDF/CDM/CDM_MetaData.cxx +@@ -66,27 +66,32 @@ CDM_MetaData::CDM_MetaData(const TCollection_ExtendedString& aFolder, + + bool CDM_MetaData::IsRetrieved() const + { ++ std::lock_guard aLock(myDocumentMutex); + return myIsRetrieved; + } + + occ::handle CDM_MetaData::Document() const + { ++ std::lock_guard aLock(myDocumentMutex); + return myDocument; + } + + void CDM_MetaData::SetDocument(const occ::handle& aDocument) + { ++ std::lock_guard aLock(myDocumentMutex); + myIsRetrieved = true; + myDocument = aDocument.operator->(); + } + + void CDM_MetaData::UnsetDocument() + { ++ std::lock_guard aLock(myDocumentMutex); + myIsRetrieved = false; + } + + occ::handle CDM_MetaData::LookUp( + NCollection_DataMap>& theLookUpTable, ++ std::mutex& theMutex, + const TCollection_ExtendedString& aFolder, + const TCollection_ExtendedString& aName, + const TCollection_ExtendedString& aPath, +@@ -96,6 +101,8 @@ occ::handle CDM_MetaData::LookUp( + occ::handle theMetaData; + TCollection_ExtendedString aConventionalPath = aPath; + aConventionalPath.ChangeAll('\\', '/'); ++ ++ std::lock_guard aLock(theMutex); + if (!theLookUpTable.IsBound(aConventionalPath)) + { + theMetaData = new CDM_MetaData(aFolder, aName, aPath, aFileName, ReadOnly); +@@ -111,6 +118,7 @@ occ::handle CDM_MetaData::LookUp( + + occ::handle CDM_MetaData::LookUp( + NCollection_DataMap>& theLookUpTable, ++ std::mutex& theMutex, + const TCollection_ExtendedString& aFolder, + const TCollection_ExtendedString& aName, + const TCollection_ExtendedString& aPath, +@@ -121,6 +129,8 @@ occ::handle CDM_MetaData::LookUp( + occ::handle theMetaData; + TCollection_ExtendedString aConventionalPath = aPath; + aConventionalPath.ChangeAll('\\', '/'); ++ ++ std::lock_guard aLock(theMutex); + if (!theLookUpTable.IsBound(aConventionalPath)) + { + theMetaData = new CDM_MetaData(aFolder, aName, aPath, aVersion, aFileName, ReadOnly); +diff --git a/src/ApplicationFramework/TKCDF/CDM/CDM_MetaData.hxx b/src/ApplicationFramework/TKCDF/CDM/CDM_MetaData.hxx +index 28fcdba8..e590234b 100644 +--- a/src/ApplicationFramework/TKCDF/CDM/CDM_MetaData.hxx ++++ b/src/ApplicationFramework/TKCDF/CDM/CDM_MetaData.hxx +@@ -28,22 +28,27 @@ + #include + #include + #include ++#include + class CDM_MetaData; + + class CDM_MetaData : public Standard_Transient + { + + public: ++ //! theMutex must guard theLookUpTable (CDM_Application::MetaDataLookUpTableMutex()). + Standard_EXPORT static occ::handle LookUp( + NCollection_DataMap>& theLookUpTable, ++ std::mutex& theMutex, + const TCollection_ExtendedString& aFolder, + const TCollection_ExtendedString& aName, + const TCollection_ExtendedString& aPath, + const TCollection_ExtendedString& aFileName, + const bool ReadOnly); + ++ //! theMutex must guard theLookUpTable (CDM_Application::MetaDataLookUpTableMutex()). + Standard_EXPORT static occ::handle LookUp( + NCollection_DataMap>& theLookUpTable, ++ std::mutex& theMutex, + const TCollection_ExtendedString& aFolder, + const TCollection_ExtendedString& aName, + const TCollection_ExtendedString& aPath, +@@ -131,6 +136,7 @@ private: + TCollection_ExtendedString myPath; + int myDocumentVersion; + bool myIsReadOnly; ++ mutable std::mutex myDocumentMutex; //!< guards myIsRetrieved/myDocument + }; + + #endif // _CDM_MetaData_HeaderFile +diff --git a/src/ApplicationFramework/TKCDF/PCDM/PCDM_ReferenceIterator.cxx b/src/ApplicationFramework/TKCDF/PCDM/PCDM_ReferenceIterator.cxx +index 76572aec..8be8c8b1 100644 +--- a/src/ApplicationFramework/TKCDF/PCDM/PCDM_ReferenceIterator.cxx ++++ b/src/ApplicationFramework/TKCDF/PCDM/PCDM_ReferenceIterator.cxx +@@ -45,12 +45,13 @@ void PCDM_ReferenceIterator::LoadReferences(const occ::handle& + { + for (Init(aMetaData); More(); Next()) + { +- aDocument->CreateReference( +- MetaData(anApplication->MetaDataLookUpTable(), UseStorageConfiguration), +- ReferenceIdentifier(), +- anApplication, +- DocumentVersion(), +- UseStorageConfiguration); ++ aDocument->CreateReference(MetaData(anApplication->MetaDataLookUpTable(), ++ anApplication->MetaDataLookUpTableMutex(), ++ UseStorageConfiguration), ++ ReferenceIdentifier(), ++ anApplication, ++ DocumentVersion(), ++ UseStorageConfiguration); + } + } + +@@ -83,6 +84,7 @@ void PCDM_ReferenceIterator::Next() + + occ::handle PCDM_ReferenceIterator::MetaData( + NCollection_DataMap>& theLookUpTable, ++ std::mutex& theMutex, + const bool) const + { + +@@ -131,6 +133,7 @@ occ::handle PCDM_ReferenceIterator::MetaData( + #endif // _WIN32 + + return CDM_MetaData::LookUp(theLookUpTable, ++ theMutex, + theFolder, + theName, + theFile, +diff --git a/src/ApplicationFramework/TKCDF/PCDM/PCDM_ReferenceIterator.hxx b/src/ApplicationFramework/TKCDF/PCDM/PCDM_ReferenceIterator.hxx +index f462dd8a..1a443c69 100644 +--- a/src/ApplicationFramework/TKCDF/PCDM/PCDM_ReferenceIterator.hxx ++++ b/src/ApplicationFramework/TKCDF/PCDM/PCDM_ReferenceIterator.hxx +@@ -24,6 +24,7 @@ + #include + #include + #include ++#include + + class Message_Messenger; + class CDM_Document; +@@ -53,6 +54,7 @@ private: + + Standard_EXPORT virtual occ::handle MetaData( + NCollection_DataMap>& theLookUpTable, ++ std::mutex& theMutex, + const bool UseStorageConfiguration) const; + + Standard_EXPORT virtual int ReferenceIdentifier() const; +diff --git a/src/ApplicationFramework/TKXmlL/XmlLDrivers/XmlLDrivers_DocumentRetrievalDriver.cxx b/src/ApplicationFramework/TKXmlL/XmlLDrivers/XmlLDrivers_DocumentRetrievalDriver.cxx +index 37753a50..c71a65b9 100644 +--- a/src/ApplicationFramework/TKXmlL/XmlLDrivers/XmlLDrivers_DocumentRetrievalDriver.cxx ++++ b/src/ApplicationFramework/TKXmlL/XmlLDrivers/XmlLDrivers_DocumentRetrievalDriver.cxx +@@ -443,6 +443,7 @@ void XmlLDrivers_DocumentRetrievalDriver::ReadFromDomDocument( + + occ::handle aMetaData = + CDM_MetaData::LookUp(theApplication->MetaDataLookUpTable(), ++ theApplication->MetaDataLookUpTableMutex(), + theFolder, + theName, + aPath, +diff --git a/src/ApplicationFramework/TKCDF/CDF/CDF_Application.cxx b/src/ApplicationFramework/TKCDF/CDF/CDF_Application.cxx +index e612ec9c..2dbb92da 100644 +--- a/src/ApplicationFramework/TKCDF/CDF/CDF_Application.cxx ++++ b/src/ApplicationFramework/TKCDF/CDF/CDF_Application.cxx +@@ -41,7 +41,7 @@ CDF_Application::CDF_Application() + : myRetrievableStatus(PCDM_RS_OK) + { + myDirectory = new CDF_Directory(); +- myMetaDataDriver = new CDF_FWOSDriver(MetaDataLookUpTable()); ++ myMetaDataDriver = new CDF_FWOSDriver(MetaDataLookUpTable(), MetaDataLookUpTableMutex()); + } + + //================================================================================================= diff --git a/Scripts/patches/README.md b/Scripts/patches/README.md index 786629b..791e97a 100644 --- a/Scripts/patches/README.md +++ b/Scripts/patches/README.md @@ -305,3 +305,21 @@ Confirmed via a debug (`-O0 -g`) build with a temporary SIGSEGV/SIGBUS signal ha See [`Scripts/repro/349-ocaf-driver-reentrancy/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/349-ocaf-driver-reentrancy) for the reproducer and full writeup. Filed upstream as [Open-Cascade-SAS/OCCT#1393](https://github.com/Open-Cascade-SAS/OCCT/issues/1393) (repro) / [OCCT#1394](https://github.com/Open-Cascade-SAS/OCCT/pull/1394) (fix, CI green on all platforms). **Retire** once the bundled OCCT includes this fix. + +## 0015-CDM_Application-metadata-lookup-table-mutex-353.patch + +**Fixes the upstream OCCT crash behind [#353](https://github.com/SecondMouseAU/OCCTSwift/issues/353)** — a race surfaced while validating the #349 fix (patch 0014, shipped v1.15.9): post-#349-fix TSan runs consistently produced exactly one different, previously-masked warning, matching the "fixing one race exposes the next" pattern from #341→#344→#349. + +`CDM_Application::myMetaDataLookUpTable` is a plain `NCollection_DataMap`, one instance per `CDM_Application` (in practice the one process-wide singleton, since #344's `GetApplication()` fix), with zero synchronization anywhere in the class. Three independent things race: (1) **map mutation** — `CDM_MetaData::LookUp()`'s unguarded `IsBound`+`Bind`/`Find`, called from `CDF_FWOSDriver::MetaData`/`CreateMetaData` (every `Store()`/`SaveAs()`), `XmlLDrivers_DocumentRetrievalDriver::Read`, and `PCDM_ReferenceIterator::MetaData`; (2) **map iteration** — `CDM_Document::SetMetaData()` walks the *entire* table on every save, reading every other document's `CDM_MetaData` state; (3) **per-object state** — each `CDM_MetaData`'s `myIsRetrieved`/`myDocument` fields, mutated by `SetDocument()`/`UnsetDocument()` and read by `IsRetrieved()`/`Document()` with no guard at all. TSan confirmed the exact trace quoted in the issue: `CDM_Document::SetMetaData()`'s map-iteration loop (reading `IsRetrieved()`, still inside #349's own per-driver lock — but that lock has no relationship to a *different* thread's document destructor) racing `~CDM_Document() -> CDM_MetaData::UnsetDocument()` tearing down an unrelated document's metadata entry on another thread. 1 confirmed race + SIGABRT (exit 134) on stock #349-fixed kernel, matching the issue's reported symptom exactly. + +**Fix:** follows the established "lock the shared resource, don't restructure the subsystem" precedent (#341's atomic bool, #344's `CDF_Directory` mutex, #349's per-driver mutex) — the map's "bind once, reuse forever, share across all documents" caching design is load-bearing (how OCCT recognizes "this file is already open" across separate calls), not incidental scratch state. Two independent locks, matching the two distinct race shapes: (1) `CDM_Application` gets a `mutable std::mutex myMetaDataLookUpTableMutex` + accessor, threaded through `CDM_MetaData::LookUp()`'s two overloads (now take the mutex as an explicit parameter) and `CDM_Document::SetMetaData()`'s iteration — `CDF_FWOSDriver`'s constructor also now takes the mutex alongside the table reference it already stored; (2) `CDM_MetaData` gets its own private `mutable std::mutex myDocumentMutex` guarding `myIsRetrieved`/`myDocument`, since two already-bound `CDM_MetaData` objects can race on `SetDocument()`/`UnsetDocument()` vs. `IsRetrieved()`/`Document()` independent of the table lock. No changes to any format-specific driver subclass. + +**Validation:** rebuilt the minimal-module TSan install (reused `occt-build-tsan349`/`occt-install-tsan349` from the #349 investigation, which already had patch 0014 baked in) with 0015 applied: race count 1 (+SIGABRT) → **0**, clean exit, across 5 runs (8×25, 10×60, 3×8×40). Full production xcframework rebuilt via `Scripts/build-occt.sh`; `swift test --filter OCAFSaveLoadBinaryTests`/`OCCTXCAFTests` and 3× full `swift test` (4423 tests) all clean. + +**Not changed**: `CDM_MetaData::myDocumentVersion` (reached via `CDM_Reference.cxx` and `CDM_Application::SetDocumentVersion`) has the identical unguarded-mutable-field shape as the fixed `myIsRetrieved`/`myDocument`, but on the document-*reference* resolution path rather than save/close — not TSan-observed in any run (the repro doesn't exercise cross-document references), flagged as a plausible sibling for a future pass, not fixed here. + +**Upstream formatting note:** OCCT's CI `clang-format` (invoked via their format-check workflow) disagreed with a locally-run `clang-format` (v22.1.8) on two spots — both formatter-alignment artifacts (`AlignConsecutiveAssignments`/declaration alignment shifting due to nearby edits), not intentional changes. Applied CI's own `format.patch` artifact to resolve; the version skew is a tooling quirk, not a real formatting defect. Worth checking CI's format-check output (not just a local dry-run) before every future upstream OCCT PR. + +See [`Scripts/repro/353-cdm-metadata-lookup-table/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/353-cdm-metadata-lookup-table) for the reproducer and full writeup. Filed upstream as [Open-Cascade-SAS/OCCT#1396](https://github.com/Open-Cascade-SAS/OCCT/issues/1396) (repro) / [OCCT#1397](https://github.com/Open-Cascade-SAS/OCCT/pull/1397) (fix, CI green on all platforms). + +**Retire** once the bundled OCCT includes this fix. diff --git a/Scripts/repro/353-cdm-metadata-lookup-table/README.md b/Scripts/repro/353-cdm-metadata-lookup-table/README.md new file mode 100644 index 0000000..b64bc18 --- /dev/null +++ b/Scripts/repro/353-cdm-metadata-lookup-table/README.md @@ -0,0 +1,188 @@ +# OCCTSwift#353 reproducer — `CDM_Application::myMetaDataLookUpTable` / `CDM_MetaData` race + +Root-cause writeup for the race surfaced during validation of the #349 fix (kernel patch +`Scripts/patches/0014`, shipped v1.15.9): post-#349-fix TSan runs consistently produced exactly +one different, previously-masked warning in `CDM_MetaData::IsRetrieved()`/`UnsetDocument()`. See +`Scripts/repro/349-ocaf-driver-reentrancy/README.md`'s "New finding surfaced by this fix" section +for the original report. + +## Verdict + +**Real, previously-undetected kernel defect — fixed.** The issue's own hypothesis is confirmed, +and the actual blast radius is a bit wider than the two call chains quoted in the issue text. + +`CDM_Application::myMetaDataLookUpTable` (`CDM_Application.hxx:97`) is a plain +`NCollection_DataMap>`, one instance per +`CDM_Application`/`CDF_Application` (in practice the one process-wide `TDocStd_Application` +singleton, since #344's `GetApplication()`/`CDF_Application` fix), with zero synchronization +anywhere in the class. Every path that touches it races against every other: + +- **Map mutation**: `CDM_MetaData::LookUp()` (both overloads, `CDM_MetaData.cxx:92`/`119`) does an + unguarded `IsBound()` + `Bind()`/`Find()` sequence on the table — called from + `CDF_FWOSDriver::MetaData`/`CreateMetaData` (via `CDF_StoreList::Store`, i.e. every + `Store()`/`SaveAs()`), `XmlLDrivers_DocumentRetrievalDriver::Read` (line 445, reference-loading + during XML retrieval), and `PCDM_ReferenceIterator::MetaData` (line 136, reference-loading for + every format). +- **Map iteration**: `CDM_Document::SetMetaData()` (`CDM_Document.cxx:474`) iterates the **entire** + table on every `SetMetaData()` call (i.e. every successful `Store()`) to update cross-document + references — reading `theMetaData->IsRetrieved()`/`->Document()` for every OTHER document's + metadata entry in the whole application, concurrently with whatever any other thread is doing to + the map or to those `CDM_MetaData` objects. +- **Per-object state**: independent of the map itself, each `CDM_MetaData` instance's + `myIsRetrieved`/`myDocument` fields are mutated by `SetDocument()`/`UnsetDocument()` + (`CDM_MetaData.cxx:77`/`83`) and read by `IsRetrieved()`/`Document()` (`.cxx:67`/`72`) with no + guard at all — this is the exact pair the original #349-writeup TSan trace caught: + `CDM_Document::SetMetaData()`'s map-iteration loop (`CDM_Document.cxx:487`, reading + `IsRetrieved()`) racing against a **different** document's destructor + (`~CDM_Document() -> UnsetDocument()`, `CDM_Document.cxx:61`) tearing down its own, unrelated + `CDM_MetaData` entry — `CDM_MetaData` objects are shared/long-lived (kept alive by the map for + the application's lifetime, per `CDM_MetaData::LookUp`'s comment-free but observable "bind once, + reuse forever" behavior), so any document's close can race any other thread's concurrent save. + +This is the same failure class as `CDF_Directory::myDocuments` (#344) and `theAutoNaming` (#341): +a process/application-shared container (here, a container *and* the objects it hands out) mutated +with no lock, now exposed because #344's `GetApplication()` fix made every caller genuinely share +one `TDocStd_Application`/`CDM_Application` instance instead of sometimes getting independent +(uncontended) ones. + +## Repro + +`occt_353_barrier.cpp` — a rename/light adaptation of +`Scripts/repro/349-ocaf-driver-reentrancy/occt_349_barrier.cpp` (the same repro that originally +surfaced this race is the cleanest way to reproduce it in isolation; a bespoke reproducer wasn't +needed). N threads each build their own small `TDocStd_Document` with a **unique file path per +(thread, round)** — so the map grows every round (`Bind`, not `Find`) rather than reusing one +key — spin-wait at a barrier, then call `TDocStd_Application::SaveAs()` on the shared +`TDocStd_Application` at nearly the same instant, and finally `app->Close(doc)` to drop the +document's last reference at the end of the loop body (destroying it, and calling +`CDM_MetaData::UnsetDocument()` on its own entry) while other threads are mid-`SaveAs()` for their +own, different documents in the next round. + +Must be run against a build with the #349 kernel fix (patch 0014) already applied — #353 is the +race #349's own fix unmasked, not #349's original driver-reentrancy defect. + +```bash +clang++ -std=c++17 -O0 -g \ + -I Libraries/OCCT.xcframework/macos-arm64/Headers \ + -L Libraries/OCCT.xcframework/macos-arm64 \ + occt_353_barrier.cpp -o occt_353_barrier \ + -lOCCT-macos -framework Foundation -framework AppKit -lz -lc++ + +MMGT_OPT=0 ./occt_353_barrier 8 25 /tmp/occt353_scratch +``` + +## TSan confirmation + +Built against the project's existing minimal-module TSan install +(`FoundationClasses`+`ModelingData`+`ModelingAlgorithms`+`DataExchange`, `RelWithDebInfo`, +`-fsanitize=thread -g`, matching the #298/#319/#341/#344/#349 protocol — reused +`Libraries/occt-build-tsan349`/`occt-install-tsan349` from the #349 investigation, which already +had patch 0014 baked in): + +```bash +clang++ -std=c++17 -fsanitize=thread -g -O1 \ + -I /include/opencascade -L /lib \ + occt_353_barrier.cpp -o occt_353_tsan \ + $(ls /lib/libTK*.a | xargs -n1 basename | sed 's/^lib//;s/\.a$//;s/^/-l/') \ + -lz -lc++ -framework Foundation + +MMGT_OPT=0 TSAN_OPTIONS="halt_on_error=0" ./occt_353_tsan 8 25 /tmp/occt353_tsan_scratch +``` + +**Before the fix** (8 threads x 25 rounds, one run): 1 TSan warning, exit code 134 (SIGABRT) — +matches the issue's reported symptom exactly: + +``` +WARNING: ThreadSanitizer: data race + Read of size 1 at 0x... by thread T8 (mutexes: write M0): + CDM_MetaData::IsRetrieved() const CDM_MetaData.cxx:69 + CDM_Document::SetMetaData(...) CDM_Document.cxx:487 + CDF_StoreList::Store(...) CDF_StoreList.cxx:153 + CDF_Store::Realize(...) CDF_Store.cxx:157 + TDocStd_Application::SaveAs(...) TDocStd_Application.cxx:363 + + Previous write of size 1 at 0x... by thread T6: + CDM_MetaData::UnsetDocument() CDM_MetaData.cxx:85 + CDM_Document::~CDM_Document() CDM_Document.cxx:61 + TDocStd_Document::~TDocStd_Document() TDocStd_Document.hxx:46 +SUMMARY: ThreadSanitizer: data race CDM_MetaData.cxx:69 in CDM_MetaData::IsRetrieved() const +``` + +Note `SetMetaData()`'s call at `CDF_StoreList.cxx:153` is still *inside* #349's own per-driver +`aDriverLock` (that lock's scope covers `Write()` through the reference-iteration loop in current +source) — but that mutex is scoped to the cached `PCDM_StorageDriver` instance for one format, and +has no relationship at all to a *different* thread's document destructor, which never touches any +driver. The two races are on genuinely independent resources; #349's fix does nothing for this one +by construction, matching the issue's own framing. + +**After the fix**: rebuilt `occt-build-tsan349`/`occt-install-tsan349` in place (`cmake --build`, +incremental, TKCDF/TKXmlL/TKLCAF and their dependents recompiled) with `Scripts/patches/0015` +applied to `occt-src`: + +- 8x25: 0 races, clean exit (0, was 134/SIGABRT). +- 10x60: 0 races, clean exit. +- 3 further runs at 8x40: 0 races, clean exit, every run. + +5/5 clean TSan runs after the fix, 0/5 before (1 confirmed race + abort every time it was tried). + +## Fix + +`Scripts/patches/0015-CDM_Application-metadata-lookup-table-mutex-353.patch`. Follows the +established "lock the shared resource, don't restructure the subsystem" precedent (#341's atomic +bool, #344's `CDF_Directory` mutex, #349's per-driver-instance mutex) rather than eliminating the +shared state — the "bind once, reuse forever, share across all documents" caching design of +`CDM_Application::myMetaDataLookUpTable` is load-bearing (it's how OCCT recognizes "this same file +is already open" across separate `Retrieve()`/`Store()` calls), not incidental scratch state like +#349's driver fields were. + +Two independent things needed locking, matching the two distinct races found: + +1. **The table itself** (`CDM_Application::myMetaDataLookUpTable`). Added + `mutable std::mutex myMetaDataLookUpTableMutex` + a `MetaDataLookUpTableMutex()` accessor to + `CDM_Application`. `CDM_MetaData::LookUp()` (both overloads) now takes the mutex as an explicit + parameter and locks it around the `IsBound`/`Bind`/`Find` sequence; `CDM_Document::SetMetaData` + locks it around its whole-table reference-update iteration. All four call sites that reach + `LookUp()`/iterate the table already had a `CDM_Application`/`CDF_Application` handle in scope + (`CDF_FWOSDriver` didn't — its constructor now also takes the mutex, alongside the table + reference it already stored), so this needed no new plumbing beyond one extra parameter per call + site. +2. **Each `CDM_MetaData` instance's own mutable state** (`myIsRetrieved`/`myDocument`) — + independent of the table, because two `CDM_MetaData` objects that are both already bound in the + map (no `Bind()`/`Find()` involved) can still race on `SetDocument()`/`UnsetDocument()` vs. + `IsRetrieved()`/`Document()` from different threads, exactly as the original TSan trace showed. + Added a private `mutable std::mutex myDocumentMutex` to `CDM_MetaData`, locked in all four + accessor/mutator methods. + +Files touched: `CDM_Application.hxx` (mutex + accessor), `CDM_MetaData.hxx`/`.cxx` (per-object +mutex + `LookUp()` signature change), `CDM_Document.cxx` (`SetMetaData()` iteration lock), +`CDF_FWOSDriver.hxx`/`.cxx` (constructor + both `LookUp()` call sites), `CDF_Application.cxx` +(one-line constructor update), `XmlLDrivers_DocumentRetrievalDriver.cxx` and +`PCDM_ReferenceIterator.hxx`/`.cxx` (the other two `LookUp()`/`MetaData()` call sites). No changes +to any format-specific driver subclass. + +**Not changed**: `CDM_MetaData::myDocumentVersion` (accessed via the private +`DocumentVersion(CDM_Application&)`, called from `CDM_Reference.cxx:91`/`107` and +`CDM_Application::SetDocumentVersion`) has the identical unguarded-mutable-field shape as +`myIsRetrieved`/`myDocument` but is on the document-*reference* resolution path, not the +save/close path this repro exercises — not observed racing in any run here, and not covered by +`occt_353_barrier` (which never loads cross-document references). Flagging as a plausible sibling, +not verified, not fixed — a natural target if a future TSan pass over the reference-loading path +turns up something. + +## Validation + +- TSan: 5/5 clean runs after the fix (8x25, 10x60, 3x8x40); 1 confirmed race + SIGABRT before, on + the same binary/harness. +- `clang-format --dry-run --Werror` clean on every touched file (config: + `Libraries/occt-src/.clang-format`). +- Full xcframework rebuilt via `Scripts/build-occt.sh` (macOS + iOS device + iOS simulator). +- `swift test --filter OCAFSaveLoadBinaryTests`, `swift test --filter OCCTXCAFTests`, and + N full `swift test` runs — see the parent task's final report for exact pass counts. + +## New follow-up races (NOT fixed here) + +None found beyond the `myDocumentVersion` note above (which is speculative, not TSan-confirmed). +Repeated post-fix TSan runs (5 total, up to 10x60) were fully clean — no new symptom surfaced the +way #344 -> #349 -> #353 each unmasked the next. If a future investigation wants to push harder +(more threads/rounds, or a repro that also exercises `CDM_ReferenceIterator`/cross-document +references to hit `DocumentVersion()`), that's the next place to look. diff --git a/Scripts/repro/353-cdm-metadata-lookup-table/occt_353_barrier.cpp b/Scripts/repro/353-cdm-metadata-lookup-table/occt_353_barrier.cpp new file mode 100644 index 0000000..6557f98 --- /dev/null +++ b/Scripts/repro/353-cdm-metadata-lookup-table/occt_353_barrier.cpp @@ -0,0 +1,155 @@ +// Barrier-synchronized reproducer for OCCTSwift#353: CDM_Application::myMetaDataLookUpTable +// (a plain NCollection_DataMap, one instance per CDM_Application/CDF_Application) and the +// CDM_MetaData objects it hands out are read/written with zero synchronization. Each thread's +// CDF_StoreList::Store() -> CDM_Document::SetMetaData() binds a new entry and iterates the WHOLE +// map (touching other threads' CDM_MetaData instances' IsRetrieved()/Document()), while another +// thread's ~CDM_Document() (from app->Close(doc) at the end of a previous round) concurrently +// calls CDM_MetaData::UnsetDocument() on its own entry. +// +// Directly reused/renamed from Scripts/repro/349-ocaf-driver-reentrancy/occt_349_barrier.cpp +// (this same repro's post-#349-fix TSan runs are what surfaced #353 in the first place): each +// thread builds its own tiny TDocStd_Document with a unique file path per (thread, round) so the +// map grows every round (Bind, not Find) and app->Close(doc) at the end of the loop body drops +// the document's last reference, destroying it (and calling UnsetDocument()) while OTHER threads +// are mid-SaveAs()/SetMetaData() for their own (different) documents. Barrier-synchronizing each +// round maximizes genuine concurrent contention on the shared table/metadata objects instead of +// relying on OS scheduling luck (same technique as occt_344_barrier.cpp/occt_349_barrier.cpp). +// +// Must be run against a build that already has the #349 kernel fix (Scripts/patches/0014) +// applied — #353 is the race #349's own fix unmasked, not reproducible cleanly underneath it. +// +// Usage: occt_353_barrier + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +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 arriveAndWait() + { + int gen = myGeneration.load(std::memory_order_relaxed); + 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) + { + std::this_thread::yield(); + } + } + } + + int myTotal; + std::atomic myCount; + std::atomic myGeneration; +}; + +// Builds a small OCAF document tree so Write() has real attribute/label work to race on. +static void populate(const Handle(TDocStd_Document) & doc, int seed) +{ + TDF_Label root = doc->Main(); + for (int i = 0; i < 25; ++i) + { + TDF_Label child = root.FindChild(i + 1, true); + TDataStd_Name::Set(child, TCollection_ExtendedString("node")); + TDataStd_Integer::Set(child, seed * 1000 + i); + } +} + +int main(int argc, char** argv) +{ + installCrashHandler(); + + int threads = argc > 1 ? atoi(argv[1]) : 8; + int rounds = argc > 2 ? atoi(argv[2]) : 200; + std::string dir = argc > 3 ? argv[3] : "/tmp/occt349_scratch"; + + note("occt_349_barrier: threads=%d rounds=%d dir=%s", threads, rounds, dir.c_str()); + system((std::string("mkdir -p ") + dir).c_str()); + + Handle(TDocStd_Application) app = new TDocStd_Application(); + BinDrivers::DefineFormat(app); + + SpinBarrier barrier(threads); + std::atomic failures{0}; + + auto worker = [&](int tid) { + for (int r = 0; r < rounds; ++r) + { + Handle(TDocStd_Document) doc; + app->NewDocument("BinOcaf", doc); + populate(doc, tid * 100000 + r); + + std::string path = dir + "/occt349_" + std::to_string(tid) + "_" + std::to_string(r) + ".cbf"; + TCollection_ExtendedString ePath(path.c_str(), true); + + barrier.arriveAndWait(); + PCDM_StoreStatus status = app->SaveAs(doc, ePath); + if (status != PCDM_SS_OK) + { + failures.fetch_add(1, std::memory_order_relaxed); + } + + app->Close(doc); + } + }; + + std::vector pool; + for (int t = 0; t < threads; ++t) + { + pool.emplace_back(worker, t); + } + for (auto& th : pool) + { + th.join(); + } + + note("occt_349_barrier: done, failures=%d", failures.load()); + return 0; +} diff --git a/Scripts/tsan-stress.sh b/Scripts/tsan-stress.sh index 1d6b4e8..feb1486 100755 --- a/Scripts/tsan-stress.sh +++ b/Scripts/tsan-stress.sh @@ -56,6 +56,7 @@ SCENARIOS=( "344-cdf-directory/occt_344_barrier.cpp|8 50" "344-cdf-directory/occt_344_stress.cpp|8 25 @SCRATCH" "349-ocaf-driver-reentrancy/occt_349_barrier.cpp|8 50 @SCRATCH" + "353-cdm-metadata-lookup-table/occt_353_barrier.cpp|8 50 @SCRATCH" ) MACOS_SDK=$(xcrun --sdk macosx --show-sdk-path) diff --git a/Scripts/tsan.supp b/Scripts/tsan.supp index 57ab543..3ff0ac1 100644 --- a/Scripts/tsan.supp +++ b/Scripts/tsan.supp @@ -21,11 +21,7 @@ race:BOPAlgo_InitMessages race:Message_PrinterOStream # --- 2. Open kernel findings, filed and tracked ----------------------------- - -# CDM_Application::myMetaDataLookUpTable is an unsynchronized NCollection map -# shared by every thread using one TDocStd_Application; surfaced by the #349 -# fix (same fix-one-race-exposes-the-next pattern as #344). Filed as -# SecondMouseAU/OCCTSwift#353. REMOVE when the #353 kernel patch is carried. -race:CDM_MetaData -race:CDM_Application -race:CDF_FWOSDriver::CreateMetaData +# +# (none currently — #353's CDM_Application::myMetaDataLookUpTable suppression +# was removed in v1.15.11 once Scripts/patches/0015 carried the kernel fix; +# see Scripts/repro/353-cdm-metadata-lookup-table/README.md.) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 8746869..a74c7e1 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -7,18 +7,43 @@ nav_order: 13 All notable changes to OCCTSwift. -## Current: v1.15.10 +## Current: v1.15.11 -**macOS / iOS (device + simulator) | OCCT 8.0.0p1 (+ #263, #280, #298, #310, #317, #318, #319, #323, #341, #344, #348, #349 kernel patches)** +**macOS / iOS (device + simulator) | OCCT 8.0.0p1 (+ #263, #280, #298, #310, #317, #318, #319, #323, #341, #344, #348, #349, #353 kernel patches)** --- ## Release History +### v1.15.11 (July 2026) — fix (kernel): CDM_Application::myMetaDataLookUpTable + CDM_MetaData field races under concurrent document save/close (#353) + +Surfaced while validating the #349 fix: post-#349 TSan runs consistently produced one different, +previously-masked race — the "fixing one race exposes the next" pattern from #341→#344→#349 +continuing. `CDM_Application::myMetaDataLookUpTable` is shared process-wide (one `CDM_Application` +singleton, since #344) with zero synchronization: `CDM_MetaData::LookUp()`'s map mutation, +`CDM_Document::SetMetaData()`'s whole-table iteration on every save, and each `CDM_MetaData`'s own +`myIsRetrieved`/`myDocument` fields all race independently. TSan confirmed the exact trace from the +issue: `SetMetaData()` reading `IsRetrieved()` racing a *different* document's destructor tearing +down its own metadata entry on another thread — 1 confirmed race + SIGABRT (exit 134) on stock +#349-fixed kernel. + +**Fix:** `CDM_Application` gets a `mutable std::mutex` guarding the lookup table, threaded through +`CDM_MetaData::LookUp()` and `CDM_Document::SetMetaData()`'s iteration; `CDM_MetaData` gets its own +private mutex guarding `myIsRetrieved`/`myDocument`, independent of the table lock. TSan: 1 race + +SIGABRT → 0 races, clean exit, across 5 runs. `swift test --filter OCAFSaveLoadBinaryTests`/ +`OCCTXCAFTests` and 3× full `swift test` (4423 tests) all clean. `CDM_MetaData::myDocumentVersion` +has the identical unguarded-field shape but on the reference-resolution path, not TSan-observed — +flagged as a plausible sibling, not fixed here. See +[`Scripts/repro/353-cdm-metadata-lookup-table/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/353-cdm-metadata-lookup-table) +for the reproducer and full writeup. Filed upstream as +[Open-Cascade-SAS/OCCT#1396](https://github.com/Open-Cascade-SAS/OCCT/issues/1396) (repro) / +[OCCT#1397](https://github.com/Open-Cascade-SAS/OCCT/pull/1397) (fix, CI green on all platforms). + ### v1.15.10 (July 2026): ThreadSanitizer gate for concurrency-touching changes (docs/tooling) Docs-and-tooling release; no API, bridge, or kernel changes, and no new binary assets (the -`OCCT.xcframework.zip` binary target continues to resolve from the v1.15.9 release). +`OCCT.xcframework.zip` binary target continued to resolve from the v1.15.9 release until v1.15.11 +above). Formalizes the TSan protocol that found and validated #298/#341/#344/#349 as a routine gate (#355, plus the #356 sysroot fix): @@ -28,7 +53,8 @@ Formalizes the TSan protocol that found and validated #298/#341/#344/#349 as a r harnesses and executes a 7-scenario gate matrix that must be race-clean; `swift` runs `swift test --sanitize=thread` on the concurrency-focused suites (wrapper-only coverage). - `Scripts/tsan.supp`: curated suppressions; only confirmed-benign races or filed-and-open kernel - findings (currently #353), each with an issue link and a removal condition. + findings, each with an issue link and a removal condition. The #353 entry was removed in + v1.15.11 once that kernel patch landed. - `docs/thread-safety.md`: new "ThreadSanitizer gate" section defining when the gate is required (new concurrent bridge paths, newly parallel-wrapped subsystems, mutex removals, new thread-safety kernel patches) and the rule that new concurrent usage patterns add a scenario.