mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-08-09 17:31:45 +00:00
Fix data races when parsing IFC files on concurrent threads
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::string> ifcopenshell::schema_registry::names() {
|
||||
std::lock_guard<std::recursive_mutex> lock(mutex_);
|
||||
std::set<std::string> seen;
|
||||
for (const auto& pair : entries_) {
|
||||
seen.insert(pair.first);
|
||||
@@ -266,6 +279,7 @@ std::vector<std::string> ifcopenshell::schema_registry::names() {
|
||||
}
|
||||
|
||||
void ifcopenshell::schema_registry::clear() {
|
||||
std::lock_guard<std::recursive_mutex> lock(mutex_);
|
||||
for (auto& pair : entries_) {
|
||||
if (pair.second.clear_) {
|
||||
pair.second.clear_();
|
||||
|
||||
@@ -28,6 +28,7 @@
|
||||
#include <cctype>
|
||||
#include <iterator>
|
||||
#include <map>
|
||||
#include <mutex>
|
||||
#include <string>
|
||||
#include <vector>
|
||||
#include <optional>
|
||||
@@ -521,6 +522,14 @@ class IFC_PARSE_API schema_registry {
|
||||
};
|
||||
|
||||
std::map<std::string, entry> 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();
|
||||
|
||||
Reference in New Issue
Block a user