From 57d9c47c6c7c5471b064358e970b5fbc048d85d3 Mon Sep 17 00:00:00 2001 From: Bruno Postle Date: Sat, 18 Jul 2026 00:45:55 +0100 Subject: [PATCH] Fix null-pointer derefs in reference resolution Two related bugs in read_from_stream's reference-resolution loop, both reachable from malformed input: - has_attribute_value only checks the stored slot's type, not that it's non-null (e.g. an explicit $ value), so the following get_attribute_value() call could return null and inst->declaration() crashed on it. - byid_[ref] default-inserts (and returns) a null pointer when the owning instance id isn't present, which was then dereferenced unconditionally via ->data(). Added regression tests using the two minimized crash inputs that found these. Generated with the assistance of an AI coding tool. (cherry picked from commit 56121ca061eb541d5d4f92dfd6a271cec4b03b2f) --- src/ifcopenshell-python/test/test_parse.py | 12 ++++++++++++ src/ifcparse/parse.cpp | 22 +++++++++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/ifcopenshell-python/test/test_parse.py b/src/ifcopenshell-python/test/test_parse.py index 1a0a379e93..f1585cc0f0 100644 --- a/src/ifcopenshell-python/test/test_parse.py +++ b/src/ifcopenshell-python/test/test_parse.py @@ -17,3 +17,15 @@ END-ISO-10303-21; f = ifcopenshell.file.from_string(data) print(ifcopenshell.get_log()) f.by_id(5) + + +def test_reference_to_undefined_owning_instance(): + data = "ISO-10303-21;HEADER;FILE_DESCRIPTION();FILE_NAME();FILE_SCHEMA(('IFC4'));#=IFCRELAGGREGATES((#))#5=IFCPOINT)" + ifcopenshell.file.from_string(data) + print(ifcopenshell.get_log()) + + +def test_reference_to_undefined_owning_instance_simple_type(): + data = "ISO-10303-21;HEADER;FILE_DESCRIPTION();FILE_NAME();FILE_SCHEMA(('IFC4'));#=IFCPROJECT((#))#4=IFCSIUNIT(" + ifcopenshell.file.from_string(data) + print(ifcopenshell.get_log()) diff --git a/src/ifcparse/parse.cpp b/src/ifcparse/parse.cpp index 9b80731992..d95f138cb2 100644 --- a/src/ifcparse/parse.cpp +++ b/src/ifcparse/parse.cpp @@ -2286,6 +2286,14 @@ void ifcopenshell::impl::in_memory_file_storage::read_from_stream(Reader* s, con for (const auto& p : streamer.references()) { const auto& ref = p.first.name_; const auto& refattr = p.first.index_; + + auto owner_it = byid_.find(ref); + if (owner_it == byid_.end()) { + logger_.get().error("Instance #" + std::to_string(ref) + " referenced at attribute index " + std::to_string(refattr) + " not found"); + continue; + } + auto& owner = owner_it->second; + if (auto* v = std::get_if(&p.second)) { if (auto* name = std::get_if(v)) { if (std::binary_search(bypassed.begin(), bypassed.end(), *name)) { @@ -2295,12 +2303,12 @@ void ifcopenshell::impl::in_memory_file_storage::read_from_stream(Reader* s, con if (it == byid_.end()) { logger_.get().error("Instance reference #" + std::to_string(*name) + " used by instance #" + std::to_string(ref) + " at attribute index " + std::to_string(refattr) + " not found at offset " + std::to_string(name->file_offset)); } else { - auto& storage = byid_[p.first.name_]; + auto& storage = owner; auto attr_index = p.first.index_; if (storage->template has_attribute_value(attr_index)) { express::Base inst = storage->get_attribute_value(attr_index); - if (!inst.declaration().as_entity()) { + if (inst && !inst.declaration().as_entity()) { // Probably a case of IfcPropertySetDefinitionSet, divert storage of reference to the simply type instance #ifdef IFOPSH_SAFE_INSTANCE storage = inst.data_weak().lock(); @@ -2318,7 +2326,7 @@ void ifcopenshell::impl::in_memory_file_storage::read_from_stream(Reader* s, con } } } else if (auto inst = std::get_if(v)) { - byid_[p.first.name_]->set_attribute_value(p.first.index_, *inst); + owner->set_attribute_value(p.first.index_, *inst); } } else if (auto* vv = std::get_if>(&p.second)) { std::vector instances; @@ -2339,12 +2347,12 @@ void ifcopenshell::impl::in_memory_file_storage::read_from_stream(Reader* s, con } } - auto& storage = byid_[p.first.name_]; + auto& storage = owner; auto attr_index = p.first.index_; if (storage->template has_attribute_value(attr_index)) { express::Base inst = storage->get_attribute_value(attr_index); - if (!inst.declaration().as_entity()) { + if (inst && !inst.declaration().as_entity()) { // Probably a case of IfcPropertySetDefinitionSet, divert storage of reference to the simply type instance #ifdef IFOPSH_SAFE_INSTANCE storage = inst.data_weak().lock(); @@ -2381,12 +2389,12 @@ void ifcopenshell::impl::in_memory_file_storage::read_from_stream(Reader* s, con } } - auto& storage = byid_[p.first.name_]; + auto& storage = owner; auto attr_index = p.first.index_; if (storage->template has_attribute_value(attr_index)) { express::Base inst = storage->get_attribute_value(attr_index); - if (!inst.declaration().as_entity()) { + if (inst && !inst.declaration().as_entity()) { // Probably a case of IfcPropertySetDefinitionSet, divert storage of reference to the simply type instance #ifdef IFOPSH_SAFE_INSTANCE storage = inst.data_weak().lock();