Internal: stack overwrite on _call_native_mb_ret with godot::Error return value

[derammo]

Godot version

custom

System information

Windows 10 Visual Studio 2019

Issue description

for R is godot::Error, a 4-byte enum:

template <class R, class... Args>
R _call_native_mb_ret(const GDNativeMethodBindPtr mb, void *instance, const Args &...args) {
	R ret;
	std::array<const GDNativeTypePtr, sizeof...(Args)> mb_args = { { (const GDNativeTypePtr)args... } };
	internal::gdn_interface->object_method_bind_ptrcall(mb, instance, mb_args.data(), &ret);
	return ret;
}

before object_method_bind_ptrcall:

after object_method_bind_ptrcall:

It has written 8 bytes and triggered stack guard detection in Visual Studio. I believe this is real and it may be caused by VARIANT_ENUM_CAST assuming all enum are int64_t and thus overwriting the 4-byte stack variable on encode. reduz would probably know instantly if this is a real issue.

If it becomes relevant, I can try to reproduce this with an official build, but it is hard to do right now because of problems in GDExtension that require me to run various patches. In any case this would be confirmed and fixed by inspection if it is previously unknown.

Steps to reproduce

Debugging extensions code, and by inspection.

Minimal reproduction project

No response

[derammo]

I put this here for tracking if this is not a known issue. I can't really provide a good repro for it, which would also be a waste of time. Either this is a code bug, or it is not, and @reduz would know the difference pretty much instantly.

[RiederAlex]

I've also hit the issue while playing around with godot-cpp for 4.0.
For godot-cpp: I've fixed it locally by explicitly specifying the underlying enum types for the generated enumerations (global_constants.hpp), so they are all of type int64_t enum Error -> enum Error : int64_t, which could also be a potential solution for the enums in this repository.

[derammo]

Yes, that would be a work-around. Could it be combined with a static assertion to make sure it isn't being used that way anywhere else?

[derammo]

By the way, I strongly suggest this should be fixed by correcting the encoding. I am just trying to confirm that based on what I saw in the code, the work-around would work.

[derammo]

Code that forces the enums to be assumed wide is here:
godotengine/godot@95088f6

Unfortunately, this means that the encoding blindly writes int64_t into the void*, even if the underlying type is a narrower enum.

[derammo]

godotengine/godot#50931

[RiederAlex]

If this issue causes the enums to have explicit types I would also propose to think if int64_t is really needed in this case and if a smaller data-type would suffice - as an enum with this much entries does not really make sense (except maybe for really huge bitflags?) plus encoding/decoding/saving/loading/network all use double the amount of data needed.

[DNKpp]

If this issue causes the enums to have explicit types I would also propose to think if int64_t is really needed in this case and if a smaller data-type would suffice - as an enum with this much entries does not really make sense (except maybe for really huge bitflags?) plus encoding/decoding/saving/loading/network all use double the amount of data needed.

In my opinion such assumptions are always the wrong direction. Its well defined assigning a 2^20 to an enum type, as long as it has enough bits, even if that enum (class) type has no named constant values. I don't see why those enum types shouldn't use the corresponding integer with equal bit size.

[DNKpp]

Even if its not pretty, but this should already be sufficent.

template <class TEnum>
using int_rep_t = std::conditional_t<
    sizeof(TEnum) == 1,
    std::uint8_t,
    std::conditional_t<
        sizeof(TEnum) == 2,
        std::uint16_t,
        std::conditional_t<
            sizeof(TEnum) == 4,
            std::uint32_t,
            std::uint64_t
            >
        >
    >;