diff --git a/core/templates/cowdata.h b/core/templates/cowdata.h index dbbb1e6b540f..d53548dd85a9 100644 --- a/core/templates/cowdata.h +++ b/core/templates/cowdata.h @@ -162,6 +162,8 @@ class CowData { return *out; } + // Decrements the reference count. Deallocates the backing buffer if needed. + // After this function, _ptr is guaranteed to be NULL. void _unref(); void _ref(const CowData *p_from); void _ref(const CowData &p_from); @@ -252,7 +254,7 @@ class CowData { Size count(const T &p_val) const; _FORCE_INLINE_ CowData() {} - _FORCE_INLINE_ ~CowData(); + _FORCE_INLINE_ ~CowData() { _unref(); } _FORCE_INLINE_ CowData(std::initializer_list p_init); _FORCE_INLINE_ CowData(const CowData &p_from) { _ref(p_from); } _FORCE_INLINE_ CowData(CowData &&p_from) { @@ -269,22 +271,30 @@ void CowData::_unref() { SafeNumeric *refc = _get_refcount(); if (refc->decrement() > 0) { - return; // still in use + // Data is still in use elsewhere. + _ptr = nullptr; + return; } - // clean up + // Clean up. + // First, invalidate our own reference. + // NOTE: It is required to do so immediately because it must not be observable outside of this + // function after refcount has already been reduced to 0. + // WARNING: It must be done before calling the destructors, because one of them may otherwise + // observe it through a reference to us. In this case, it may try to access the buffer, + // which is illegal after some of the elements in it have already been destructed, and + // may lead to a segmentation fault. + USize current_size = *_get_size(); + T *prev_ptr = _ptr; + _ptr = nullptr; if constexpr (!std::is_trivially_destructible_v) { - USize current_size = *_get_size(); - for (USize i = 0; i < current_size; ++i) { - // call destructors - T *t = &_ptr[i]; - t->~T(); + prev_ptr[i].~T(); } } // free mem - Memory::free_static(((uint8_t *)_ptr) - DATA_OFFSET, false); + Memory::free_static((uint8_t *)prev_ptr - DATA_OFFSET, false); } template @@ -339,9 +349,8 @@ Error CowData::resize(Size p_size) { } if (p_size == 0) { - // wants to clean up - _unref(); - _ptr = nullptr; + // Wants to clean up. + _unref(); // Resets _ptr to nullptr. return OK; } @@ -484,8 +493,7 @@ void CowData::_ref(const CowData &p_from) { return; // self assign, do nothing. } - _unref(); - _ptr = nullptr; + _unref(); // Resets _ptr to nullptr. if (!p_from._ptr) { return; //nothing to do @@ -496,11 +504,6 @@ void CowData::_ref(const CowData &p_from) { } } -template -CowData::~CowData() { - _unref(); -} - template CowData::CowData(std::initializer_list p_init) { Error err = resize(p_init.size()); diff --git a/tests/core/templates/test_vector.h b/tests/core/templates/test_vector.h index 4152a8258cc4..c9267cc8d79d 100644 --- a/tests/core/templates/test_vector.h +++ b/tests/core/templates/test_vector.h @@ -532,6 +532,31 @@ TEST_CASE("[Vector] Operators") { CHECK(vector != vector_other); } +struct CyclicVectorHolder { + Vector *vector = nullptr; + bool is_destructing = false; + + ~CyclicVectorHolder() { + if (is_destructing) { + // The vector must exist and not expose its backing array at this point. + CHECK_NE(vector, nullptr); + CHECK_EQ(vector->ptr(), nullptr); + } + } +}; + +TEST_CASE("[Vector] Cyclic Reference") { + // Create a stack-space vector. + Vector vector; + // Add a new (empty) element. + vector.resize(1); + // Expose the vector to its element through CyclicVectorHolder. + // This is questionable behavior, but should still behave graciously. + vector.ptrw()[0] = CyclicVectorHolder{ &vector }; + vector.ptrw()[0].is_destructing = true; + // The vector goes out of scope and destructs, calling CyclicVectorHolder's destructor. +} + } // namespace TestVector #endif // TEST_VECTOR_H