Fix sparse_set invalid state when component constructor throws (#176)
Fix sparse_set invalid state when component constructor throws Previously, a component's constructor that throws would cause the sparse set to think that the entity still exists in the set. This is because the underlying sparse set that stores the entities will have its entry added before the component is added to the component set. This could cause a number of invalid memory access problems such as the following: 1) Exception triggers destructor of enclosing object that then tries to remove the component that was just added. SparseSet<entity_t, Component>::has() will return true for the entity but when destroy() is called "instances" will be empty so instances.back() will be invalid. 2) If the exception is handled then calling get(entity) for the same entity identifier that initially threw the exception will give a position for that entity even though it was not added. This can cause an invalid memory access or accessing the data of a different stored component.
This commit is contained in:
committed by
Michele Caini
parent
00f1f6d86b
commit
9f997fe1fa
@@ -914,9 +914,8 @@ public:
|
||||
*/
|
||||
template<typename... Args>
|
||||
object_type & construct([[maybe_unused]] const entity_type entity, [[maybe_unused]] Args &&... args) {
|
||||
underlying_type::construct(entity);
|
||||
|
||||
if constexpr(std::is_empty_v<object_type>) {
|
||||
underlying_type::construct(entity);
|
||||
return instances;
|
||||
} else {
|
||||
if constexpr(std::is_aggregate_v<object_type>) {
|
||||
@@ -925,6 +924,8 @@ public:
|
||||
instances.emplace_back(std::forward<Args>(args)...);
|
||||
}
|
||||
|
||||
// entity goes after component in case constructor throws
|
||||
underlying_type::construct(entity);
|
||||
return instances.back();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
#include <memory>
|
||||
#include <exception>
|
||||
#include <algorithm>
|
||||
#include <unordered_set>
|
||||
#include <gtest/gtest.h>
|
||||
@@ -1054,3 +1055,23 @@ TEST(SparseSetWithType, CloneMoveOnlyComponent) {
|
||||
entt::sparse_set<std::uint64_t, std::unique_ptr<int>> set;
|
||||
ASSERT_EQ(set.clone(), nullptr);
|
||||
}
|
||||
|
||||
TEST(SparseSetWithType, ConstructorExceptionDoesNotAddToSet) {
|
||||
struct throwing_component {
|
||||
struct constructor_exception: std::exception {};
|
||||
|
||||
throwing_component() { throw constructor_exception{}; }
|
||||
|
||||
// necessary to avoid the short-circuit construct() logic for empty objects
|
||||
int data;
|
||||
};
|
||||
|
||||
entt::sparse_set<std::uint64_t, throwing_component> set;
|
||||
|
||||
try {
|
||||
set.construct(0);
|
||||
FAIL() << "Expected constructor_exception to be thrown";
|
||||
} catch (const throwing_component::constructor_exception &) {
|
||||
ASSERT_TRUE(set.empty());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user