Stack buffer overrun for enum function arguments in gdextension API, affecting at least godot-cpp

[bgie]

I ran into a stack buffer overrun, code reading extra memory that it should not, only to throw these extra bytes away.

This bug was detected with address sanitizer (GCC -fsanitize=address), and prevents anyone from using address sanitizer in combination with godot-cpp, because address sanitizer aborts on first error. Otherwise the impact of this bug is probably low.

However, allowing use of address sanitizer by contributors does seem like it will benefit the quality of the godot codebase long term. There is no realistic workaround if one wishes to use address sanitizer with godot-cpp, because any call to the godot API that involves an enum parameter will trigger the buffer overrun + abort by asan.

Bug details: the affected code receives a pointer to an enum as a void pointer, and reinterpret casts it to an int64 pointer, probably because int64 is the size used for variants. But enums are not 64 bit by default, and the majority of enums in the godot codebase use the default size, which is usually int (32 bit on most platforms), and never larger unless absolutely needed, see C++ standard on enum size
Since the code immediately does a truncating C-style cast of this int64 to the enum type, the 4 extra bytes will be discarded immediately and not result in any program logic errors.

[dsnopek]

Thanks!

While most enums will be 32-bit, some may be 64-bit (for example ArrayFormat), and we don't currently include the size of the enum in extension_api.json, the GDExtension binding can't be sure to use the same size for the enum as Godot does.

In general, GDExtension encodes all integer types as 64-bit, even if they are cast to a smaller type once they arrive on the other side. I think we should be doing the same thing here.

So, both on the Godot side and the godot-cpp side, I think we should make sure we store the enum's value into a temporary 64-bit integer and pass a pointer to that, rather than using pointer directly to the enum which could be either 32 or 64-bit.

[bgie]

I looked into the suggested alternative fix.

Assuming that fix would be similar to this example generated code from the godot-cpp API, where a copy of a bool is made:

void Node::add_child(Node *p_node, bool p_force_readable_name, Node::InternalMode p_internal) {
	static GDExtensionMethodBindPtr _gde_method_bind = internal::gdextension_interface_classdb_get_method_bind(Node::get_class_static()._native_ptr(), StringName("add_child")._native_ptr(), 3863233950);
	CHECK_METHOD_BIND(_gde_method_bind);
	int8_t p_force_readable_name_encoded;
	PtrToArg<bool>::encode(p_force_readable_name, &p_force_readable_name_encoded);
	internal::_call_native_mb_no_ret(_gde_method_bind, _owner, (p_node != nullptr ? &p_node->_owner : nullptr), &p_force_readable_name_encoded, &p_internal);
}

I found binding_generator.py in the godot-cpp repo which contains def get_encoded_arg(arg_name, type_name, type_meta)
An extra if in there to generate code to copy the enum into an int64_t would do the trick.
Only for regular enums, not BitFields, which are already stored as 64 bit.

Not eager to be messing in a python script generating C-style macros using C++ templates though...

Since this fix is in godot-cpp, should I create a bug report in the godot-cpp repo instead?

[dsnopek]

Since this fix is in godot-cpp, should I create a bug report in the godot-cpp repo instead?

That would great, thanks!

Not eager to be messing in a python script generating C-style macros using C++ templates though...

If you don't want to implement the PR to fix, that's fine, I can take a look at doing that. Investigating and reporting your findings is great contribution on its own. :-)