From f16ba3d26e0de6f4b9e87a9bd5ad289a528fdd70 Mon Sep 17 00:00:00 2001
From: Bruno Postle <bruno@postle.net>
Date: Tue, 21 Jul 2026 14:50:36 +0100
Subject: [PATCH] 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.
---
 src/ifcparse/IfcParse.cpp | 9 +++++----
 src/ifcparse/storage.h    | 3 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/ifcparse/IfcParse.cpp b/src/ifcparse/IfcParse.cpp
index bc6df697af..07179ace5c 100644
--- a/src/ifcparse/IfcParse.cpp
+++ b/src/ifcparse/IfcParse.cpp
@@ -1661,14 +1661,15 @@ void IfcParse::impl::in_memory_file_storage::read_from_stream(IfcParse::FileRead
             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;
             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 });
+            // 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.
+            duplicate_id_instances_.emplace_back(instance);
+        }
 
         // @nb cannot assign to byid_;
         // byid_[current_id] = instance;
diff --git a/src/ifcparse/storage.h b/src/ifcparse/storage.h
index 8c46d59640..b64d609cfb 100644
--- a/src/ifcparse/storage.h
+++ b/src/ifcparse/storage.h
@@ -199,6 +199,9 @@ namespace IfcParse {
                 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;
             std::reference_wrapper<Logger> logger_;
             // IfcParse::FileReader* stream;