Destruct CowData more graciously by avoiding accidentally exposing …

…a half-destructed buffer. This can avoid problems if any of the destructed objects tries to access the data while it's being destructed.

core/templates/cowdata.h

@@ -269,22 +269,30 @@ void CowData<T>::_unref() {
 
 	SafeNumeric<USize> *refc = _get_refcount();
 	if (refc->decrement() > 0) {
-		return; // still in use
+		// Data are still in use elsewhere.
+		_ptr = nullptr;
+		return;
 	}
-	// clean up
+	// Clean up.
+	// First, invalidate our own reference.
+	// 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.
+	// 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>
@@ -341,7 +349,6 @@ Error CowData<T>::resize(Size p_size) {
 	if (p_size == 0) {
 		// wants to clean up
 		_unref();
-		_ptr = nullptr;
 		return OK;
 	}
 
@@ -485,7 +492,6 @@ void CowData<T>::_ref(const CowData &p_from) {
 	}
 
 	_unref();
-	_ptr = nullptr;
 
 	if (!p_from._ptr) {
 		return; //nothing to do

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