Fixed leaks if allocation of ctrl block fails

Author: cschreib

include/oup/observable_unique_ptr.hpp

@@ -564,13 +564,19 @@ class basic_observable_ptr final {
      * \note Do *not* manually delete this raw pointer after the
      * @ref observable_unique_ptr is created. If possible, prefer
      * using @ref make_observable() instead of this constructor.
+     * \note If the allocation of the control block fails, the
+     * pointer `value` will be deleted, and no memory will leak.
      */
     template<
         typename U,
         typename enable =
             std::enable_if_t<std::is_convertible_v<U*, T*> && queries::owner_allow_release()>>
-    explicit basic_observable_ptr(U* value) :
-        basic_observable_ptr(get_block_from_object_(value), value) {}
+    explicit basic_observable_ptr(U* value) try :
+        basic_observable_ptr(get_or_create_block_from_object_(value), value) {
+    } catch (...) {
+        // Allocation of control block failed, delete input pointer and rethrow
+        ptr_deleter(value);
+    }
 
     /**
      * \brief Explicit ownership capture of a raw pointer, with customer deleter.
@@ -579,13 +585,19 @@ class basic_observable_ptr final {
      * \note Do *not* manually delete this raw pointer after the
      * @ref basic_observable_ptr is created. If possible, prefer
      * using @ref make_observable() instead of this constructor.
+     * \note If the allocation of the control block fails, the
+     * pointer `value` will be deleted, and no memory will leak.
      */
     template<
         typename U,
         typename enable =
             std::enable_if_t<std::is_convertible_v<U*, T*> && queries::owner_allow_release()>>
-    explicit basic_observable_ptr(U* value, Deleter del) :
-        basic_observable_ptr(get_block_from_object_(value), value, std::move(del)) {}
+    explicit basic_observable_ptr(U* value, Deleter del) try :
+        basic_observable_ptr(get_or_create_block_from_object_(value), value, std::move(del)) {
+    } catch (...) {
+        // Allocation of control block failed, delete input pointer and rethrow
+        del(value);
+    }
 
     /**
      * \brief Transfer ownership by implicit casting
@@ -666,9 +678,10 @@ class basic_observable_ptr final {
 
     /**
      * \brief Replaces the managed object.
-     * \param ptr The new object to manage (can be `nullptr`, then this is equivalent to
-     *      @ref reset())
+     * \param ptr The new object to manage (can be null, then this is equivalent to @ref reset())
      * \note This function is enabled only if `Policy::is_sealed` is false.
+     * \note If the allocation of the control block fails, the pointer `ptr` will be deleted, and
+     * no memory will leak.
      */
     template<
         typename U,
@@ -680,8 +693,14 @@ class basic_observable_ptr final {
         control_block_type* old_block = block;
 
         // Assign the new one
-        block            = get_block_from_object_(ptr);
-        ptr_deleter.data = ptr;
+        try {
+            block            = get_or_create_block_from_object_(ptr);
+            ptr_deleter.data = ptr;
+        } catch (...) {
+            // Allocation of control block failed, delete input pointer and rethrow
+            ptr_deleter(ptr);
+            throw;
+        }
 
         // Delete the old pointer
         // (this follows `std::unique_ptr` specs)

tests/runtime_tests.cpp

@@ -9,9 +9,10 @@
 constexpr std::size_t max_allocations = 20'000;
 void*                 allocations[max_allocations];
 void*                 allocations_array[max_allocations];
-std::size_t           num_allocations = 0u;
-std::size_t           double_delete   = 0u;
-bool                  memory_tracking = false;
+std::size_t           num_allocations          = 0u;
+std::size_t           double_delete            = 0u;
+bool                  memory_tracking          = false;
+bool                  force_allocation_failure = false;
 
 #if defined(OUP_PLATFORM_OSX)
 // Getting weird errors on MacOS when overriding operator new and delete,
@@ -27,6 +28,10 @@ void* allocate(std::size_t size, bool array) {
         throw std::bad_alloc();
     }
 
+    if (force_allocation_failure) {
+        throw std::bad_alloc();
+    }
+
     void* p = std::malloc(size);
     if (!p) {
         throw std::bad_alloc();
@@ -277,6 +282,24 @@ TEST_CASE("owner acquiring constructor", "[owner_construction]") {
     REQUIRE(mem_track.double_del() == 0u);
 }
 
+TEST_CASE("owner acquiring constructor bad alloc", "[owner_construction]") {
+    memory_tracker mem_track;
+
+    {
+        test_object* raw_ptr = new test_object;
+        try {
+            force_allocation_failure = true;
+            test_ptr{raw_ptr};
+        } catch (...) {
+        }
+        force_allocation_failure = false;
+    }
+
+    REQUIRE(instances == 0);
+    REQUIRE(mem_track.leaks() == 0u);
+    REQUIRE(mem_track.double_del() == 0u);
+}
+
 TEST_CASE("owner acquiring constructor with deleter", "[owner_construction]") {
     memory_tracker mem_track;
 
@@ -294,6 +317,24 @@ TEST_CASE("owner acquiring constructor with deleter", "[owner_construction]") {
     REQUIRE(mem_track.double_del() == 0u);
 }
 
+TEST_CASE("owner acquiring constructor with deleter bad alloc", "[owner_construction]") {
+    memory_tracker mem_track;
+
+    {
+        test_object* raw_ptr = new test_object;
+        try {
+            force_allocation_failure = true;
+            test_ptr_with_deleter{raw_ptr, test_deleter{42}};
+        } catch (...) {
+        }
+        force_allocation_failure = false;
+    }
+
+    REQUIRE(instances == 0);
+    REQUIRE(mem_track.leaks() == 0u);
+    REQUIRE(mem_track.double_del() == 0u);
+}
+
 TEST_CASE("owner acquiring constructor null", "[owner_construction]") {
     memory_tracker mem_track;
 
@@ -1278,6 +1319,28 @@ TEST_CASE("owner reset to new", "[owner_utility]") {
     REQUIRE(mem_track.double_del() == 0u);
 }
 
+TEST_CASE("owner reset to new bad alloc", "[owner_utility]") {
+    memory_tracker mem_track;
+
+    {
+        test_object* raw_ptr1 = new test_object;
+        test_object* raw_ptr2 = new test_object;
+        test_ptr     ptr(raw_ptr1);
+        try {
+            force_allocation_failure = true;
+            ptr.reset(raw_ptr2);
+        } catch (...) {
+        }
+        force_allocation_failure = false;
+        REQUIRE(instances == 1);
+        REQUIRE(ptr.get() == raw_ptr1);
+    }
+
+    REQUIRE(instances == 0);
+    REQUIRE(mem_track.leaks() == 0u);
+    REQUIRE(mem_track.double_del() == 0u);
+}
+
 TEST_CASE("owner reset to new with deleter", "[owner_utility]") {
     memory_tracker mem_track;