From e5b345553368cee3eda412b83cf20bfaf779f379 Mon Sep 17 00:00:00 2001 From: Adrien SCHVALBERG Date: Thu, 28 Apr 2022 10:33:54 +0200 Subject: [PATCH] Fix memory leak for HeaderEntity instances: the getArgumentCount() call in IfcEntityInstanceData destructor isn't virtually dispatched so it returns 0 instead of correct number of arguments => memory leak added IfcEntityInstanceData::clearArguments() which clears all arguments and resets the attributes_ field to nullptr Remove unnecessary copy in IfcEntityInstanceData::setArgument() when argument is created at the call site (no more leak of original argument) Fix memory leak in IfcFile: delete entities from both byid and entity_file_map --- src/ifcparse/IfcEntityInstanceData.h | 6 +- src/ifcparse/IfcParse.cpp | 263 ++++++++++++++------------- src/ifcparse/IfcSpfHeader.cpp | 4 + src/ifcparse/IfcSpfHeader.h | 1 + 4 files changed, 149 insertions(+), 125 deletions(-) diff --git a/src/ifcparse/IfcEntityInstanceData.h b/src/ifcparse/IfcEntityInstanceData.h index ecce5f8474..e8b62b172b 100644 --- a/src/ifcparse/IfcEntityInstanceData.h +++ b/src/ifcparse/IfcEntityInstanceData.h @@ -66,8 +66,8 @@ public: Argument* getArgument(size_t i) const; - // NB: This makes a copy of the argument - void setArgument(size_t i, Argument* a, IfcUtil::ArgumentType attr_type = IfcUtil::Argument_UNKNOWN); + // NB: This makes a copy of the argument if make_copy is set + void setArgument(size_t i, Argument* a, IfcUtil::ArgumentType attr_type = IfcUtil::Argument_UNKNOWN, bool make_copy = false); virtual size_t getArgumentCount() const { if (type_ == 0) { @@ -80,6 +80,8 @@ public: } } + void clearArguments(); + const IfcParse::declaration* type() const { return type_; } diff --git a/src/ifcparse/IfcParse.cpp b/src/ifcparse/IfcParse.cpp index 51d344033b..7366ff75a2 100644 --- a/src/ifcparse/IfcParse.cpp +++ b/src/ifcparse/IfcParse.cpp @@ -1057,15 +1057,21 @@ std::string IfcEntityInstanceData::toString(bool upper) const { return ss.str(); } -IfcEntityInstanceData::~IfcEntityInstanceData() { +void IfcEntityInstanceData::clearArguments() +{ if (attributes_ != NULL) { for (size_t i = 0; i < getArgumentCount(); ++i) { delete attributes_[i]; } delete[] attributes_; + attributes_ = NULL; } } +IfcEntityInstanceData::~IfcEntityInstanceData() { + clearArguments(); +} + unsigned IfcEntityInstanceData::set_id(boost::optional i) { if (i) { return id_ = *i; @@ -1148,7 +1154,7 @@ IfcEntityInstanceData::IfcEntityInstanceData(const IfcEntityInstanceData& e) { for (unsigned int i = 0; i < count; ++i) { attributes_[i] = 0; - this->setArgument(i, e.getArgument(i), get_argument_type(e.type(), i)); + this->setArgument(i, e.getArgument(i), get_argument_type(e.type(), i), true); } } @@ -1265,131 +1271,135 @@ public: }; -void IfcEntityInstanceData::setArgument(size_t i, Argument* a, IfcUtil::ArgumentType attr_type) { +void IfcEntityInstanceData::setArgument(size_t i, Argument* a, IfcUtil::ArgumentType attr_type, bool make_copy) { if (attributes_ == 0) { load(); } - - if (attr_type == IfcUtil::Argument_UNKNOWN) { - attr_type = a->type(); - } else if (a->isNull()) { - attr_type = IfcUtil::Argument_NULL; - } - - IfcWrite::IfcWriteArgument* copy = new IfcWrite::IfcWriteArgument(); - - switch (attr_type) { - case IfcUtil::Argument_NULL: - copy->set(boost::blank()); - break; - case IfcUtil::Argument_DERIVED: - copy->set(IfcWrite::IfcWriteArgument::Derived()); - break; - case IfcUtil::Argument_INT: - copy->set(static_cast(*a)); - break; - case IfcUtil::Argument_BOOL: - copy->set(static_cast(*a)); - break; - case IfcUtil::Argument_LOGICAL: { - boost::logic::tribool tb = *a; - copy->set(tb); - break; - } - case IfcUtil::Argument_DOUBLE: - copy->set(static_cast(*a)); - break; - case IfcUtil::Argument_STRING: - copy->set(static_cast(*a)); - break; - case IfcUtil::Argument_BINARY: { - boost::dynamic_bitset<> attr_value = *a; - copy->set(attr_value); - break; } - case IfcUtil::Argument_AGGREGATE_OF_INT: { - std::vector attr_value = *a; - copy->set(attr_value); - break; } - case IfcUtil::Argument_AGGREGATE_OF_DOUBLE: { - std::vector attr_value = *a; - copy->set(attr_value); - break; } - case IfcUtil::Argument_AGGREGATE_OF_STRING: { - std::vector attr_value = *a; - copy->set(attr_value); - break; } - case IfcUtil::Argument_AGGREGATE_OF_BINARY: { - std::vector< boost::dynamic_bitset<> > attr_value = *a; - copy->set(attr_value); - break; } - case IfcUtil::Argument_ENUMERATION: { - std::string enum_literal = a->toString(); - // Remove leading and trailing '.' - enum_literal = enum_literal.substr(1, enum_literal.size() - 2); - - const IfcParse::enumeration_type* enum_type = type()->as_enumeration_type() - ? type()->as_enumeration_type() - : type()->as_entity()->attribute_by_index(i)->type_of_attribute()-> - as_named_type()->declared_type()->as_enumeration_type(); - - std::vector::const_iterator it = std::find( - enum_type->enumeration_items().begin(), - enum_type->enumeration_items().end(), - enum_literal); - - if (it == enum_type->enumeration_items().end()) { - throw IfcParse::IfcException(enum_literal + " does not name a valid item for " + enum_type->name()); + Argument* new_attribute = a; + if (make_copy) { + if (attr_type == IfcUtil::Argument_UNKNOWN) { + attr_type = a->type(); + } else if (a->isNull()) { + attr_type = IfcUtil::Argument_NULL; } - copy->set(IfcWrite::IfcWriteArgument::EnumerationReference(it - enum_type->enumeration_items().begin(), it->c_str())); - break; } - case IfcUtil::Argument_ENTITY_INSTANCE: { - copy->set(static_cast(*a)); - break; } - case IfcUtil::Argument_AGGREGATE_OF_ENTITY_INSTANCE: { - aggregate_of_instance::ptr instances = *a; - aggregate_of_instance::ptr mapped_instances(new aggregate_of_instance); - // @todo mapped_instances are not actually mapped to the file using add(). - for (aggregate_of_instance::it it = instances->begin(); it != instances->end(); ++it) { - mapped_instances->push(*it); + IfcWrite::IfcWriteArgument* copy = new IfcWrite::IfcWriteArgument(); + + switch (attr_type) { + case IfcUtil::Argument_NULL: + copy->set(boost::blank()); + break; + case IfcUtil::Argument_DERIVED: + copy->set(IfcWrite::IfcWriteArgument::Derived()); + break; + case IfcUtil::Argument_INT: + copy->set(static_cast(*a)); + break; + case IfcUtil::Argument_BOOL: + copy->set(static_cast(*a)); + break; + case IfcUtil::Argument_LOGICAL: { + boost::logic::tribool tb = *a; + copy->set(tb); + break; } - copy->set(mapped_instances); - break; } - case IfcUtil::Argument_AGGREGATE_OF_AGGREGATE_OF_INT: { - std::vector< std::vector > attr_value = *a; - copy->set(attr_value); - break; } - case IfcUtil::Argument_AGGREGATE_OF_AGGREGATE_OF_DOUBLE: { - std::vector< std::vector > attr_value = *a; - copy->set(attr_value); - break; } - case IfcUtil::Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE: { - aggregate_of_aggregate_of_instance::ptr instances = *a; - aggregate_of_aggregate_of_instance::ptr mapped_instances(new aggregate_of_aggregate_of_instance); - for (aggregate_of_aggregate_of_instance::outer_it it = instances->begin(); it != instances->end(); ++it) { - std::vector inner; - for (aggregate_of_aggregate_of_instance::inner_it jt = it->begin(); jt != it->end(); ++jt) { - inner.push_back(*jt); + case IfcUtil::Argument_DOUBLE: + copy->set(static_cast(*a)); + break; + case IfcUtil::Argument_STRING: + copy->set(static_cast(*a)); + break; + case IfcUtil::Argument_BINARY: { + boost::dynamic_bitset<> attr_value = *a; + copy->set(attr_value); + break; } + case IfcUtil::Argument_AGGREGATE_OF_INT: { + std::vector attr_value = *a; + copy->set(attr_value); + break; } + case IfcUtil::Argument_AGGREGATE_OF_DOUBLE: { + std::vector attr_value = *a; + copy->set(attr_value); + break; } + case IfcUtil::Argument_AGGREGATE_OF_STRING: { + std::vector attr_value = *a; + copy->set(attr_value); + break; } + case IfcUtil::Argument_AGGREGATE_OF_BINARY: { + std::vector< boost::dynamic_bitset<> > attr_value = *a; + copy->set(attr_value); + break; } + case IfcUtil::Argument_ENUMERATION: { + std::string enum_literal = a->toString(); + // Remove leading and trailing '.' + enum_literal = enum_literal.substr(1, enum_literal.size() - 2); + + const IfcParse::enumeration_type* enum_type = type()->as_enumeration_type() + ? type()->as_enumeration_type() + : type()->as_entity()->attribute_by_index(i)->type_of_attribute()-> + as_named_type()->declared_type()->as_enumeration_type(); + + std::vector::const_iterator it = std::find( + enum_type->enumeration_items().begin(), + enum_type->enumeration_items().end(), + enum_literal); + + if (it == enum_type->enumeration_items().end()) { + throw IfcParse::IfcException(enum_literal + " does not name a valid item for " + enum_type->name()); } - mapped_instances->push(inner); - } - copy->set(mapped_instances); - break; } - case IfcUtil::Argument_EMPTY_AGGREGATE: - case IfcUtil::Argument_AGGREGATE_OF_EMPTY_AGGREGATE: { - IfcUtil::ArgumentType t2 = IfcUtil::from_parameter_type(type()->as_entity()->attribute_by_index(i)->type_of_attribute()); - delete copy; - copy = 0; - setArgument(i, a, t2); - break; } - default: - case IfcUtil::Argument_UNKNOWN: - throw IfcParse::IfcException(std::string("Unknown attribute encountered: '") + a->toString() + "' at index '" + boost::lexical_cast(i) + "'"); - break; - } - if (!copy) { - return; + copy->set(IfcWrite::IfcWriteArgument::EnumerationReference(it - enum_type->enumeration_items().begin(), it->c_str())); + break; } + case IfcUtil::Argument_ENTITY_INSTANCE: { + copy->set(static_cast(*a)); + break; } + case IfcUtil::Argument_AGGREGATE_OF_ENTITY_INSTANCE: { + aggregate_of_instance::ptr instances = *a; + aggregate_of_instance::ptr mapped_instances(new aggregate_of_instance); + // @todo mapped_instances are not actually mapped to the file using add(). + for (aggregate_of_instance::it it = instances->begin(); it != instances->end(); ++it) { + mapped_instances->push(*it); + } + copy->set(mapped_instances); + break; } + case IfcUtil::Argument_AGGREGATE_OF_AGGREGATE_OF_INT: { + std::vector< std::vector > attr_value = *a; + copy->set(attr_value); + break; } + case IfcUtil::Argument_AGGREGATE_OF_AGGREGATE_OF_DOUBLE: { + std::vector< std::vector > attr_value = *a; + copy->set(attr_value); + break; } + case IfcUtil::Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE: { + aggregate_of_aggregate_of_instance::ptr instances = *a; + aggregate_of_aggregate_of_instance::ptr mapped_instances(new aggregate_of_aggregate_of_instance); + for (aggregate_of_aggregate_of_instance::outer_it it = instances->begin(); it != instances->end(); ++it) { + std::vector inner; + for (aggregate_of_aggregate_of_instance::inner_it jt = it->begin(); jt != it->end(); ++jt) { + inner.push_back(*jt); + } + mapped_instances->push(inner); + } + copy->set(mapped_instances); + break; } + case IfcUtil::Argument_EMPTY_AGGREGATE: + case IfcUtil::Argument_AGGREGATE_OF_EMPTY_AGGREGATE: { + IfcUtil::ArgumentType t2 = IfcUtil::from_parameter_type(type()->as_entity()->attribute_by_index(i)->type_of_attribute()); + delete copy; + copy = 0; + setArgument(i, a, t2, make_copy); + break; } + default: + case IfcUtil::Argument_UNKNOWN: + throw IfcParse::IfcException(std::string("Unknown attribute encountered: '") + a->toString() + "' at index '" + boost::lexical_cast(i) + "'"); + break; + } + + if (!copy) { + return; + } + + new_attribute = copy; } if (attributes_[i] != 0) { @@ -1403,10 +1413,10 @@ void IfcEntityInstanceData::setArgument(size_t i, Argument* a, IfcUtil::Argument if (this->file) { register_inverse_visitor visitor(*this->file, *this); - apply_individual_instance_visitor(copy, i).apply(visitor); + apply_individual_instance_visitor(new_attribute, i).apply(visitor); } - attributes_[i] = copy; + attributes_[i] = new_attribute; } // @@ -2260,8 +2270,15 @@ IfcUtil::IfcBaseClass* IfcFile::instance_by_guid(const std::string& guid) { // FIXME: Test destructor to delete entity and arg allocations IfcFile::~IfcFile() { - for( entity_by_id_t::const_iterator it = byid.begin(); it != byid.end(); ++ it ) { - delete it->second; + std::set entities_to_delete; + for (const auto& pair : byid) { + entities_to_delete.insert(pair.second); + } + for (const auto& pair : entity_file_map) { + entities_to_delete.insert(pair.second); + } + for (auto entity : entities_to_delete) { + delete entity; } delete stream; delete tokens; diff --git a/src/ifcparse/IfcSpfHeader.cpp b/src/ifcparse/IfcSpfHeader.cpp index 779b717152..4f17ecaeee 100644 --- a/src/ifcparse/IfcSpfHeader.cpp +++ b/src/ifcparse/IfcSpfHeader.cpp @@ -46,6 +46,10 @@ HeaderEntity::HeaderEntity(const char * const datatype, size_t size, IfcFile* fi } } +HeaderEntity::~HeaderEntity() +{ + clearArguments(); +} void IfcSpfHeader::readSemicolon() { if (!TokenFunc::isOperator(file_->tokens->Next(), ';')) { diff --git a/src/ifcparse/IfcSpfHeader.h b/src/ifcparse/IfcSpfHeader.h index 84280d7ed8..c979699fbf 100644 --- a/src/ifcparse/IfcSpfHeader.h +++ b/src/ifcparse/IfcSpfHeader.h @@ -36,6 +36,7 @@ private: HeaderEntity& operator =(const HeaderEntity&); //N/A protected: HeaderEntity(const char * const datatype, size_t size, IfcParse::IfcFile* file); + virtual ~HeaderEntity(); void setValue(unsigned int i, const std::string& s) { IfcWrite::IfcWriteArgument* argument = new IfcWrite::IfcWriteArgument;