Skip to content

Fix leaks caused by implicitly-called copy constructors#355

Merged
akien-mga merged 1 commit intogodotengine:masterfrom
sheepandshepherd:variantleak
Dec 4, 2019
Merged

Fix leaks caused by implicitly-called copy constructors#355
akien-mga merged 1 commit intogodotengine:masterfrom
sheepandshepherd:variantleak

Conversation

@sheepandshepherd
Copy link
Contributor

Previously, Variants were unpacked like this:

Variant::operator PoolVector2Array() const {
	godot_pool_vector2_array s = godot::api->godot_variant_as_pool_vector2_array(&_godot_variant);
	return *(PoolVector2Array *)&s;
}

The cast implicitly calls the C++ type's copy constructor. (You can find it in the gdb disassembly.) This increases the ref-count twice instead of just once through godot_variant_as_*. For PoolArrays, it caused an error message on exiting Godot:

ERROR: cleanup: There are still MemoryPool allocs in use at exit!
   At: core/pool_vector.cpp:69.

All of the ref-counted, RAII-managed core types are affected. The fix is to use a local variable and return it instead of casting.

@zhangshiqian1214
Copy link

zhangshiqian1214 commented Dec 4, 2019

I test this fix is ok and can fix #318, your solution is beautiful, only use a local variable fix ref-count

@akien-mga
Copy link
Member

Thanks!

@sheepandshepherd
Copy link
Contributor Author

Looks like this still leaks for Array and Dictionary. I forgot to take into account that their default constructors, unlike PoolArrays, allocate a small empty array that is now getting leaked instead of the one in the Variant.

I don't know of any way to prevent this default constructor call in C++. Instead of trying more hackish casts, I think I'll just add a safe, clean constructor to Array and Dictionary: Array(godot_array p_array), unless anyone has another idea for how to convert the C types without calling either default or copy constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants