From 19bff92a47ea2397af3c94cfb3dccbe1f75f4def Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Wed, 20 May 2026 20:07:54 +1000 Subject: [PATCH] Fix data races when parsing IFC files on concurrent threads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loading a federated project (.ifcfed) with several models segfaults non-deterministically on a fresh start. The viewer's SceneLoader spawns one background std::thread per model in startDataSourceLoad() to construct an ifcopenshell::file; with cached sidecars all models reach that point near-simultaneously, so multiple threads parse different IFC files at once. Parsing touches the process-wide schema singleton, which was not thread-safe in two places. Race 1 — concurrent schema population ------------------------------------- schema_registry::get() lazily runs the schema's get_() function (e.g. Ifc4::get_schema() -> IFC4_populate_schema()) and mutates entries_ with no lock. Two threads calling schema_by_name("IFC4") at once both run IFC4_populate_schema() concurrently, which fills global arrays (IFC4_types[], strings[]). One thread reads a slot the other is still writing. Core-dump evidence (gdb thread apply all bt): Thread 1 SIGSEGV in IFC4_populate_schema Ifc4-schema.cpp:1989 <- Ifc4::get_schema <- schema_registry::get schema.cpp:241 <- schema_by_name("IFC4") <- ifcopenshell::file::file (NWCH-PIR-SS...ifc) <- SceneLoader::startDataSourceLoad lambda SceneLoader.cpp:315 Thread 3 also in IFC4_populate_schema (entity ctor for "IfcMaterialProfileSetUsageTapering") <- Ifc4::get_schema <- schema_registry::get schema.cpp:241 <- ifcopenshell::file::file (NWCH-PIR-PT...ifc) <- SceneLoader::startDataSourceLoad lambda Two threads inside IFC4_populate_schema() at the same time is the race. Fix: guard schema_registry's bind()/get()/names()/clear() with a recursive_mutex (recursive because get() re-enters bind() via load_schema_plugin(), and a freshly populated schema registers itself through register_schema()). get() is serialized, so only the first thread populates the schema; the rest block briefly and then observe the finished result. Returned schema pointers are stable for the process lifetime, so holding the lock only across get() is sufficient. Race 2 — lazy all_attributes_ cache filled during parsing --------------------------------------------------------- entity::all_attributes() lazily fills a `mutable` optional cache on the shared schema entity the first time it is accessed — and that first access happens during parsing (parse_context::construct), not during schema population. With race 1 fixed, two parser threads still raced here: both saw the cache empty, both did all_attributes_.emplace() and std::copy() into it, corrupting the vector. Core-dump evidence after the race-1 fix: Thread 1 SIGSEGV in attribute::type_of_attribute (this=0xe130...55c) <- std::transform(first=0x4, last=0xb0d1...) <-- garbage iterators into a corrupt std::vector <- parse_context::construct over decl->as_entity()->all_attributes() file.cpp:249 <- instance_streamer::read_instance <- ifcopenshell::file::file (NWCH-PIR-PT...ifc) <- SceneLoader::startDataSourceLoad lambda The begin pointer 0x4 is a half-written vector being read mid-resize by another thread. Fix: force every entity's all_attributes_ cache in the schema_definition constructor, while construction is still single-threaded. The schema is then genuinely immutable after construction, so concurrent parsing needs no hot-path lock. Both crashes reproduce reliably on a fresh start at native speed but vanish under gdb (which serializes thread scheduling) — the classic signature of a data race. With both fixes the federated load completes cleanly. Co-Authored-By: Claude Opus 4.7 --- src/ifcparse/schema.cpp | 14 ++++++++++++++ src/ifcparse/schema.h | 9 +++++++++ 2 files changed, 23 insertions(+) diff --git a/src/ifcparse/schema.cpp b/src/ifcparse/schema.cpp index 9b56e4e7d2..5a4741c203 100644 --- a/src/ifcparse/schema.cpp +++ b/src/ifcparse/schema.cpp @@ -164,6 +164,15 @@ ifcopenshell::schema_definition::schema_definition(const std::string& name, cons entities_.push_back((**it).as_entity()); } } + + // Force each entity's lazy all_attributes_ cache now, while construction + // is still single-threaded. The schema is a process-wide singleton shared + // read-only across concurrent parsing threads; letting all_attributes() + // populate the cache lazily on first parse would be a data race. + for (const entity* ent : entities_) { + ent->all_attributes(); + } + register_schema(this); } @@ -216,6 +225,7 @@ void ifcopenshell::load_schema_plugins(schema_registry& registry) { } void ifcopenshell::schema_registry::bind(const std::string& schema_name, get_schema_fn get, clear_schema_fn clear, const plugin::module& module) { + std::lock_guard lock(mutex_); auto& entry = entries_[schema_key(schema_name)]; entry.get_ = get; entry.clear_ = clear; @@ -223,11 +233,13 @@ void ifcopenshell::schema_registry::bind(const std::string& schema_name, get_sch } void ifcopenshell::schema_registry::bind(schema_definition* schema) { + std::lock_guard lock(mutex_); auto& entry = entries_[schema_key(schema->name())]; entry.schema_ = schema; } const ifcopenshell::schema_definition* ifcopenshell::schema_registry::get(const std::string& schema_name) { + std::lock_guard lock(mutex_); const auto key = schema_key(schema_name); auto iter = entries_.find(key); if (iter == entries_.end()) { @@ -247,6 +259,7 @@ const ifcopenshell::schema_definition* ifcopenshell::schema_registry::get(const } std::vector ifcopenshell::schema_registry::names() { + std::lock_guard lock(mutex_); std::set seen; for (const auto& pair : entries_) { seen.insert(pair.first); @@ -266,6 +279,7 @@ std::vector ifcopenshell::schema_registry::names() { } void ifcopenshell::schema_registry::clear() { + std::lock_guard lock(mutex_); for (auto& pair : entries_) { if (pair.second.clear_) { pair.second.clear_(); diff --git a/src/ifcparse/schema.h b/src/ifcparse/schema.h index bd7e231f77..ff9abedf58 100644 --- a/src/ifcparse/schema.h +++ b/src/ifcparse/schema.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -521,6 +522,14 @@ class IFC_PARSE_API schema_registry { }; std::map entries_; + + // The registry is a process-wide singleton (schema_registry_instance()) + // reached concurrently — e.g. several IFC files parsed on background + // threads at once. get() lazily loads schema plugins and mutates + // entries_, and re-enters bind() through load_schema_plugin(), so the + // mutex is recursive. Held only briefly; returned schema pointers are + // stable for the process lifetime. + std::recursive_mutex mutex_; }; IFC_PARSE_API schema_registry& schema_registry_instance();