Skip to content

Fix CDF_Application-cached storage/retrieval driver reentrancy (SIGSEGV, #1393)#1394

Open
gsdali wants to merge 1 commit into
Open-Cascade-SAS:masterfrom
gsdali:fix/349-cdf-driver-reentrancy
Open

Fix CDF_Application-cached storage/retrieval driver reentrancy (SIGSEGV, #1393)#1394
gsdali wants to merge 1 commit into
Open-Cascade-SAS:masterfrom
gsdali:fix/349-cdf-driver-reentrancy

Conversation

@gsdali

@gsdali gsdali commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Fixes #1393.

CDF_Application::WriterFromFormat/ReaderFromFormat cache one storage/retrieval driver instance
per format and hand the same instance back to every subsequent Store()/Retrieve() call for
that format — including from different threads, different documents, concurrently.
PCDM_StorageDriver/PCDM_Reader subclasses (e.g. BinLDrivers_DocumentStorageDriver) are not
reentrant: Write()/Read() mutate instance-level scratch state with no synchronization, so two
threads calling Write() on the same cached instance corrupt it — reliably reproducible SIGSEGV,
see #1393 for the full trace and TSan evidence (136 race warnings → 0 with this patch).

CDF_StoreList::Store already carries a comment acknowledging multi-threaded access to the driver
("It has sense in multi-threaded access to the storage driver..."), but only the store-status was
ever actually made safe for concurrent use — every other member of the driver races freely.

Fix

Adds a mutable std::mutex + Mutex() accessor to PCDM_StorageDriver and PCDM_Reader, so every
concrete format driver (BinLDrivers, XmlLDrivers, BinXCAFDrivers, XmlXCAFDrivers, TObj
variants, ...) inherits the guard for free — no changes needed in any subclass. The three places
CDF_Application/CDF_StoreList invoke a cached driver's Write()/Read() now hold that driver's
own mutex for the call: CDF_StoreList::Store, CDF_Application::Retrieve, CDF_Application::Read.

This targets the actual shared resource (the cached driver instance) rather than a broad lock
around unrelated code, and doesn't serialize unrelated formats against each other — a "BinOcaf"
save and an unrelated "Xml" save use different cached driver instances and different mutexes.

Testing

  • Reproducer (occt_349_barrier.cpp, N threads racing TDocStd_Application::SaveAs() on a shared
    application): SIGSEGV within the first few hundred rounds at just 2 threads on unpatched master;
    clean across repeated runs with this patch.
  • ThreadSanitizer (minimal-module build: FoundationClasses+ModelingData+ModelingAlgorithms+
    DataExchange): 136 distinct race warnings + SIGSEGV (exit 139) on unpatched master → 0 races,
    clean exit, confirmed at both 8×25 and 10×200 thread/round configurations.
  • Full reproducer, writeup, and TSan logs: https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/349-ocaf-driver-reentrancy

PCDM_StorageDriver/PCDM_Reader subclasses cached per-format by
CDF_Application are not reentrant; concurrent Write()/Read() calls on
the same shared instance corrupt its scratch state, causing a SIGSEGV
in BinLDrivers_DocumentStorageDriver::FirstPass under concurrent
Save/SaveAs of the same format. Add a mutex to each base class guarding
its own Write()/Read(), held at the three call sites that use a cached,
possibly-shared driver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

CDF_Application-cached storage/retrieval driver instances are not reentrant (SIGSEGV on concurrent Save/SaveAs of the same format)

1 participant