Glassvision/IfcOpenShell
1
0
Fork 0
mirror of https://github.com/IfcOpenShell/IfcOpenShell.git synced 2026-08-05 23:41:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix null-pointer derefs in reference resolution

Browse Source 
Two related bugs in read_from_stream's reference-resolution
loop, both reachable from malformed input:

- has_attribute_value<IfcBaseClass*> 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 56121ca061)
This commit is contained in:
Bruno Postle
2026-07-18 00:45:55 +01:00
committed by Dion Moult
parent 141cf95568
commit 57d9c47c6c
2 changed files with 27 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ifcopenshell-python/test/test_parse.py
+12
View File
@@ -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())
src/ifcparse/parse.cpp
+15 -7
View File
@@ -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<reference_or_simple_type>(&p.second)) {
if (auto* name = std::get_if<instance_reference>(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<express::Base>(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<express::Base>(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<std::vector<reference_or_simple_type>>(&p.second)) {
std::vector<express::Base> 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<express::Base>(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<express::Base>(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();