From 9f997fe1fa8e17e3da6dd77ae57009c5d42c286e Mon Sep 17 00:00:00 2001
From: corystegel <cory.stegelmeier@thalmic.com>
Date: Fri, 25 Jan 2019 16:36:30 -0500
Subject: [PATCH] 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.
---
 src/entt/entity/sparse_set.hpp  |  5 +++--
 test/entt/entity/sparse_set.cpp | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/entt/entity/sparse_set.hpp b/src/entt/entity/sparse_set.hpp
index 74f841026..8da2e321d 100644
--- a/src/entt/entity/sparse_set.hpp
+++ b/src/entt/entity/sparse_set.hpp
@@ -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();
         }
     }
diff --git a/test/entt/entity/sparse_set.cpp b/test/entt/entity/sparse_set.cpp
index 0add9d30c..c03dbfcea 100644
--- a/test/entt/entity/sparse_set.cpp
+++ b/test/entt/entity/sparse_set.cpp
@@ -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());
+    }
+}