mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-09-22 21:52:26 +00:00
Fix leak of entities on duplicate #id
byid_.insert() at IfcParse.cpp is a no-op when the id already exists (unlike operator[], it doesn't overwrite), even though the adjacent log message claims the entity is being overwritten. The newly instantiated duplicate-id entity was therefore never stored anywhere and leaked. It can't simply be deleted on insert failure: by that point it's already been registered in byguid_ (if IfcRoot) and bytype_excl_'s per-type aggregate, both earlier in the same loop iteration, so discarding it would leave those maps holding dangling pointers. Route it into a new owned list instead (same pattern already used for read_simple_type_instances) so it stays alive for the file's lifetime but still gets freed on destruction. Found by fuzzing (fuzzer-3su); repro is crashes/0f486942362e in the fuzzer repo, minimized to 2 duplicate-id entities out of ~256 total. Generated with the assistance of an AI coding tool.
This commit is contained in:
@@ -1661,14 +1661,15 @@ void IfcParse::impl::in_memory_file_storage::read_from_stream(IfcParse::FileRead
|
|||||||
bytype_excl_[ty]->push(instance);
|
bytype_excl_[ty]->push(instance);
|
||||||
}
|
}
|
||||||
|
|
||||||
if (byid_.find(current_id) != byid_.end()) {
|
// byidentity_[instance->identity()] = instance;
|
||||||
|
if (!byid_.insert({(uint32_t) current_id, instance }).second) {
|
||||||
std::stringstream ss;
|
std::stringstream ss;
|
||||||
ss << "Overwriting instance with name #" << current_id;
|
ss << "Overwriting instance with name #" << current_id;
|
||||||
logger().Message(Logger::LOG_WARNING, "SYN", 18, ss.str());
|
logger().Message(Logger::LOG_WARNING, "SYN", 18, ss.str());
|
||||||
}
|
|
||||||
|
|
||||||
// byidentity_[instance->identity()] = instance;
|
// insert() is a no-op on a duplicate key; instance is already referenced by byguid_/bytype_excl_ above, so keep it owned here instead of leaking it.
|
||||||
byid_.insert({(uint32_t) current_id, instance });
|
duplicate_id_instances_.emplace_back(instance);
|
||||||
|
}
|
||||||
|
|
||||||
// @nb cannot assign to byid_;
|
// @nb cannot assign to byid_;
|
||||||
// byid_[current_id] = instance;
|
// byid_[current_id] = instance;
|
||||||
|
|||||||
@@ -199,6 +199,9 @@ namespace IfcParse {
|
|||||||
return std::move(read_simple_type_instances);
|
return std::move(read_simple_type_instances);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Entities rejected by byid_.insert() on a duplicate #id; kept alive here since byguid_/bytype_excl_ already reference them.
|
||||||
|
std::vector<std::unique_ptr<IfcUtil::IfcBaseClass>> duplicate_id_instances_;
|
||||||
|
|
||||||
IfcParse::IfcSpfLexer* tokens;
|
IfcParse::IfcSpfLexer* tokens;
|
||||||
std::reference_wrapper<Logger> logger_;
|
std::reference_wrapper<Logger> logger_;
|
||||||
// IfcParse::FileReader* stream;
|
// IfcParse::FileReader* stream;
|
||||||
|
|||||||
Reference in New Issue
Block a user