Merge pull request #100694 from Ivorforce/cowdata-destruct-graciously

Destruct `CowData` more graciously by avoiding accidentally exposing a half-destructed buffer.

core/templates/cowdata.h

@@ -162,6 +162,8 @@ private:
 		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 @@ public:
 	Size count(const T &p_val) const;
 
 	_FORCE_INLINE_ CowData() {}
-	_FORCE_INLINE_ ~CowData();
+	_FORCE_INLINE_ ~CowData() { _unref(); }
 	_FORCE_INLINE_ CowData(std::initializer_list<T> p_init);
 	_FORCE_INLINE_ CowData(const CowData<T> &p_from) { _ref(p_from); }
 	_FORCE_INLINE_ CowData(CowData<T> &&p_from) {
@@ -269,22 +271,30 @@ void CowData<T>::_unref() {
 
 	SafeNumeric<USize> *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<T>) {
-		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 <typename T>
@@ -339,9 +349,8 @@ Error CowData<T>::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<T>::_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<T>::_ref(const CowData &p_from) {
 	}
 }
 
-template <typename T>
-CowData<T>::~CowData() {
-	_unref();
-}
-
 template <typename T>
 CowData<T>::CowData(std::initializer_list<T> p_init) {
 	Error err = resize(p_init.size());

tests/core/templates/test_vector.h

@@ -532,6 +532,31 @@ TEST_CASE("[Vector] Operators") {
 	CHECK(vector != vector_other);
 }
 
+struct CyclicVectorHolder {
+	Vector<CyclicVectorHolder> *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<CyclicVectorHolder> 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