Glassvision/IfcOpenShell
1
0
Fork 0
mirror of https://github.com/IfcOpenShell/IfcOpenShell.git synced 2026-09-28 03:05:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix leak of mid-parse entities when an exception escapes the scan loop

Browse Source 
read_from_stream() ran the entity-scanning while loop with no
enclosing try/catch. If an exception escaped construction of an
instance mid-stream (e.g. malformed attribute data following a
syntactically valid header), it unwound straight out of IfcFile's
constructor body, which has no try/catch of its own around this
call. The constructor never completes, ~IfcFile() never runs its
usual byid_ cleanup, and any entities already inserted into byid_
during this same scan are leaked with nothing left to free them.

Catch the exception at the loop instead, matching how a bad header
is already handled via tryRead(): mark the file INVALID_SYNTAX and
stop scanning, so the constructor always completes and existing
cleanup paths apply to whatever was parsed so far.

Caught broadly (std::exception, not just IfcException): parsing
helpers below this point also use plain std::out_of_range for
malformed-input control flow (e.g. FileReader::peek() at EOF), not
just IfcException, and any of those escaping here is exactly as
leak-prone.

Found by fuzzing (fuzzer-0f1); repro is a valid IFC4 header followed
by '#1=IFCPERSON(wi);#1=IFCORGANIZATION,e'.

Generated with the assistance of an AI coding tool.
This commit is contained in:
Bruno Postle
2026-07-21 07:37:34 +01:00
parent efac8a0ec0
commit 6b39666bfa
1 changed files with 47 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ifcparse/IfcParse.cpp
+47 -41
View File
@@ -1623,57 +1623,63 @@ void IfcParse::impl::in_memory_file_storage::read_from_stream(IfcParse::FileRead
logger().Status("Scanning file..."); logger().Status("Scanning file...");
while (streamer) { while (streamer) {
try {
auto inst = streamer.readInstance();
auto inst = streamer.readInstance(); if (!inst) {
// No more instances to read
break;
}
if (!inst) { auto current_id = std::get<0>(*inst);
// No more instances to read
break;
}
auto current_id = std::get<0>(*inst); auto instance = schema->instantiate(std::get<1>(*inst), std::move(std::get<2>(*inst)));
instance->file_ = file;
instance->id_ = (uint32_t) current_id;
auto instance = schema->instantiate(std::get<1>(*inst), std::move(std::get<2>(*inst))); if (instance->declaration().is(*ifcroot_type_)) {
instance->file_ = file; try {
instance->id_ = (uint32_t) current_id; // @nb here we know we're using in-memory so 'nullptr, nullptr, 0' is safe
const std::string guid = instance->data().get_attribute_value(nullptr, nullptr, 0, 0);
if (instance->declaration().is(*ifcroot_type_)) { if (byguid_.find(guid) != byguid_.end()) {
try { std::stringstream ss;
// @nb here we know we're using in-memory so 'nullptr, nullptr, 0' is safe ss << "Instance encountered with non-unique GlobalId " << guid;
const std::string guid = instance->data().get_attribute_value(nullptr, nullptr, 0, 0); logger().Message(Logger::LOG_WARNING, "SYN", 16, ss.str());
if (byguid_.find(guid) != byguid_.end()) { }
std::stringstream ss; byguid_[guid] = instance;
ss << "Instance encountered with non-unique GlobalId " << guid; } catch (const IfcException& ex) {
logger().Message(Logger::LOG_WARNING, "SYN", 16, ss.str()); logger().Message(Logger::LOG_ERROR, "SYN", 17, ex.what());
} }
byguid_[guid] = instance;
} catch (const IfcException& ex) {
logger().Message(Logger::LOG_ERROR, "SYN", 17, ex.what());
} }
}
const IfcParse::declaration* ty = &instance->declaration(); const IfcParse::declaration* ty = &instance->declaration();
{ {
if (bytype_excl_.find(ty) == bytype_excl_.end()) { if (bytype_excl_.find(ty) == bytype_excl_.end()) {
bytype_excl_[ty].reset(new aggregate_of_instance()); bytype_excl_[ty].reset(new aggregate_of_instance());
}
bytype_excl_[ty]->push(instance);
} }
bytype_excl_[ty]->push(instance);
if (byid_.find(current_id) != byid_.end()) {
std::stringstream ss;
ss << "Overwriting instance with name #" << current_id;
logger().Message(Logger::LOG_WARNING, "SYN", 18, ss.str());
}
// byidentity_[instance->identity()] = instance;
byid_.insert({(uint32_t) current_id, instance });
// @nb cannot assign to byid_;
// byid_[current_id] = instance;
max_id = (std::max)(max_id, (unsigned int) current_id);
} catch (const std::exception& e) {
// Stop scanning (like a bad header already does via tryRead()) instead of letting this escape IfcFile's constructor, which has no try/catch of its own and would leak everything already in byid_.
good_ = file_open_status::INVALID_SYNTAX;
logger().Error("SYN", 42, e);
break;
} }
if (byid_.find(current_id) != byid_.end()) {
std::stringstream ss;
ss << "Overwriting instance with name #" << current_id;
logger().Message(Logger::LOG_WARNING, "SYN", 18, ss.str());
}
// byidentity_[instance->identity()] = instance;
byid_.insert({(uint32_t) current_id, instance });
// @nb cannot assign to byid_;
// byid_[current_id] = instance;
max_id = (std::max)(max_id, (unsigned int) current_id);
} }
good_ = streamer.status(); good_ = streamer.status();