Only check for Godot 4.0 if the pointer is aligned how it would be for the legacy interface.

[BenLubar]

Not checking the function pointer before reading it like a struct causes ubsan to detect this as a problem.

[dsnopek]

Can you explain this a little more? What is the specific error from UBSAN? Why is the value of the pointer guaranteed to be evenly divisible by the alignment of the (partial) struct in the case that we are passed a pointer to the struct?

Looking at the current code, I'm realizing this isn't 100% safe for 32-bit architectures. On a 64-bit system, we know the value will definitely have two elements when cast to uint32_t *, regardless of if it's a struct or a function pointer, but on a 32-bit system it won't if it's a function pointer, and we'll be poking into random memory. So, I suppose on 32-bit systems we should only check the first element.

Are you running UBSAN on a 32-bit build?

[BenLubar]

I was compiling with platform=web on a custom export template that had assertions and safe heap on, so I think it was actually safe heap that was causing it to complain about the unaligned access.

The idea of the check is that the struct will be aligned at least to pointer size (since the partial version of the struct contains pointers) and the function was at an address ending with 3 so there's no reason to read from the function memory to see if it's Godot 4.0.

I don't think there's any risk of the check reading past memory bounds because the function would have to start with (in x86)

04 00    add al, 0
00 00    add byte ptr [eax], al

and the start of the function isn't going to be less than 4 bytes from the end of memory even if it's a thunk.

[dsnopek]

The idea of the check is that the struct will be aligned at least to pointer size (since the partial version of the struct contains pointers) and the function was at an address ending with 3 so there's no reason to read from the function memory to see if it's Godot 4.0.

Is the struct guaranteed to have the same alignof() for the struct in both Godot and the GDExtension?

One of my worries is that Godot could be built with one compiler and set of options that would align the struct on, for example, 4 bytes, and the extension could be built with another compiler or options and it would align the struct on 8 bytes, and then we could get a false negative.

Since the test is actually accessing the data as uint32_t *, maybe we should checking the alignment of uint32_t * instead?

I don't think there's any risk of the check reading past memory bounds [...]

Yeah, practically speaking, reading an extra 4 bytes into memory is probably fine, but it could certainly trigger sanitizers like ASAN

[Bromeon]

Maybe interesting, Rust simply does this:
https://github.com/godot-rust/gdext/blob/master/godot-ffi/src/interface_init.rs#L30-L30

But I don't check for alignment either. Maybe I should 🤔 even though I haven't seen any problems yet...

[BenLubar]

Is the struct guaranteed to have the same alignof() for the struct in both Godot and the GDExtension?

The Godot alignof might be a multiple of the GDExtension alignof if there's some field that has a more-than-8-byte alignment, but as long as there's no preprocessor #pragma pack stuff going on, the check shouldn't have any false negatives.

[dsnopek]

The Godot alignof might be a multiple of the GDExtension alignof if there's some field that has a more-than-8-byte alignment

Ok! So long as we can be sure that, if they aren't the same, that the Godot alignof() would be a multiple of the GDExtension alignof() and not the other way around, then this seems fine.

I've tested this PR with an extension built using GCC on Linux, and the official builds of Godot 4.0, 4.0.1, 4.0.2 and 4.0.3, and it correctly detects that it's being loaded by Godot 4.0

[dsnopek]

Cherry-picked for 4.4 in PR #1836