godotengine / godotengine/godot-cpp

method binding on non-MSVC compilers relies on undefined behavior

Open
#1,586 3 comments 0 reactions 0 assignees View on GitHub
discussion waiting for Godot
Dominant language
C++
Stars
2.7k
Forks
809
Avg merge
1d 3h
Merged PRs (30d)
8

Description

### Godot version

all versions since at least 3 years ago (it's as far back as I looked)

### godot-cpp version

all versions since at least 3 years ago

### System information

non-MSVC compilers

### Issue description

Method binding in godot-cpp relies on undefined behavior if `TYPED_METHOD_BIND` is not defined. It seems that it's only enabled with the MSVC compiler.

Here, `reinterpret_cast` is used on an unrelated type to cast to `MB_T`:

https://github.com/godotengine/godot-cpp/blob/4131b7f95f5a4ec660484431b175e15d95aed8db/include/godot_cpp/core/method_bind.hpp#L319-L326

Here, `reinterpret_cast` is used on unrelated member function pointers:

https://github.com/godotengine/godot-cpp/blob/4131b7f95f5a4ec660484431b175e15d95aed8db/include/godot_cpp/core/method_bind.hpp#L342-L351

`reinterpret_cast`ing here causes UB as the resulting member to function pointer cannot be used safely, as you are not casting to `unsigned char *` or something like that. Moreover, casting to a member function pointer of a non-base class is also UB. Member function pointers are implementation defined, so honestly, there is no way to rely on what the compiler will do here.

It seems like MSVC actually errors on the UB here which is really nice. I think that switching to `TYPED_METHOD_BIND` would be the right thing to do here.

If y'all are open to it, I could work on a fix. I think it would just be enabling that switch on for all builds.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.