Possible heap corruption issue

[lightyears1998]

If my understanding is correct, the current implementation of memnew_arr_template and memdelete_arr could lead to heap corruption.

godot-cpp/include/godot_cpp/core/memory.hpp

Lines 142 to 146 in be7ed4c

	size_t len = sizeof(T) * p_elements;
	uint64_t *mem = (uint64_t *)Memory::alloc_static(len);
	T *failptr = nullptr; // Get rid of a warning.
	ERR_FAIL_COND_V(!mem, failptr);
	*(mem - 1) = p_elements;

Line 146 *(mem - 1) = p_elements; assumes that godot-cpp version of Memory::alloc_static has allocated extra bytes before actual array, which is not true. Memory::alloc_static internally calls mem_alloc from gd extension interface. mem_alloc function is implemented in godot/core/extension/gdnative_interface.cpp as a call to a macro memalloc(p_size), which expands to the godot upstream version of Memory::alloc_static(m_size), and godot upstream version of Memory::alloc_static doesn't pre-allocate extras bytes before the actual array.

Suggested fix: master...lightyears1998:godot-cpp:fix-heap-corruption

[lightyears1998]

It should be quite esay to create a demo project to verify the above idea based on the offical test project and the fact that String::utf8 method is using memnew_arr_template internally.

Steps reproduce to heap corruption issue

1. Modify the code of the official test project to make a function call to String::utf8. For exmaple:

diff --git a/test/src/example.cpp b/test/src/example.cpp
index 8375111..7f58e3a 100644
--- a/test/src/example.cpp
+++ b/test/src/example.cpp
@@ -155,6 +155,7 @@ void Example::simple_const_func() const {

 String Example::return_something(const String &base) {
        UtilityFunctions::print("  Return something called.");
+       auto char_string(base.utf8());
        return base;
 }

2. Compile the test project with scons target=release. (Optionally scons target=debug. Be caution, as it may or may not crash your editor.)

3. Open the test project in godot editor and export the game without "export with debug" option.

4. Run the exported game and it crashes.

Screenshot from Visual Studio debugger

Please pay attention to the call stack. Heap is corrupted during memdelete_arr inside the destructor of CharString.

[Zylann]

Here is upstream alloc_static:

https://github.com/godotengine/godot/blob/24115beb3c69e648b1d48f969785b9b0729e1be8/core/os/memory.cpp#L68-L95

You may notice prepad. When that variable is true, there will actually be 16 bytes allocated before the pointer returned by alloc_static. The first 8 bytes are set to the size of the requested size. The next 8 bytes are apparently not used. Then after that, starts the block that is returned.

Now prepad is always true with DEBUG_ENABLED. Otherwise, it depends on the p_pad_align argument.
https://github.com/godotengine/godot/blob/4ba934bf3d1e697d8f332b5e8cfd694cdf49a7ba/core/os/memory.h#L54
By default, that argument is false. And it turns out, memalloc does not fill that argument.
https://github.com/godotengine/godot/blob/4ba934bf3d1e697d8f332b5e8cfd694cdf49a7ba/core/os/memory.h#L82

Which means, in DEBUG_ENABLED builds, the code works fine, writing to the 8 unused bytes (it's almost like they were made for that purpose, but there is no comments at all so can only speculate).
But will corrupt the heap otherwise.

In any case, I believe GodotCpp has no reason to store anything before the pointer... data structures exploiting that are core types that GodotCpp exposes already, I dont see a reason to exploit such an intrinsic low level detail in the bindings library. The sole fact it's not always present should mean it's not actually necessary.
If it is, it should really be commented better. Then either the presence of padding must match with the engine depending on the compile target, which is risky and quite an opinionated choice, or GodotCpp has to add its own padding, without assuming what alloc_static does (approach done by @lightyears1998). It increases the size of dynamic memory blocks though, which is why I question its usefulness.

[cridenour]

I've been running with @lightyears1998 quick fix for a while now, but it doesn't seem like its the right long term solution? We're getting ready to showcase a demo next month and I'd like to feel confident in our release builds if we can.

Who would have the expertise here - maybe @vnen?

I appreciate all the leg work so far and for the quick fix as well.

[vnen]

I honestly have no idea. If I wrote this it was quite some time ago and together with a lot of other changes so it's very possible I misunderstood something.

It seems @Zylann nailed the cause though, so it seems just a matter of opening the PR to remove the subtraction.

[lyuma]

I am able to confirm the crash with my compiled editor build (TOOLS_ENABLED but not DEBUG_ENABLED using my PR godotengine/godot#73668) with the test extension included in godot-cpp and the test/demo project, and I am able to confirm that the proposed change by @lightyears1998 solves it.

@lightyears1998 any chance you could submit your change as a PR? It looks good, and I can confirm it fixes the issue.

@Zylann, godot-cpp currently uses the extra 8 bytes so that non-trivially-destructable array types can be properly destructed. This is similar to how the delete[] operator works in C++.

As for why godot-cpp cannot simply pass true for the second argument, it is because Memory::alloc_static in godot-cpp actually calls into GDExtensionInterface::mem_alloc, which is defined as void *(*mem_alloc)(size_t p_bytes);
There is therefore no way to pass true for p_pad_align without changing the GDExtension ABI in a possibly breaking way.

As a result, I recommend the fix proposed by lightyears1998.