Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash on exit when using OpenXRExtensionWrapperExtension #88613

Closed
Malcolmnixon opened this issue Feb 21, 2024 · 2 comments · Fixed by #88689
Closed

Crash on exit when using OpenXRExtensionWrapperExtension #88613

Malcolmnixon opened this issue Feb 21, 2024 · 2 comments · Fixed by #88689

Comments

@Malcolmnixon
Copy link
Contributor

Tested versions

  • Reproducible in 4.3-dev [fb10e67]

System information

Windows 11, gl_compatibility, NVidia RTX 3070 TI

Issue description

When terminating a Godot application which uses any OpenXRExtensionWrapperExtension objects, the application terminates in a call of memdelete(extension_wrapper) with a pointer that isn't a valid block of heap memory.

The problem is caused by the inheritance structure of OpenXRExtensionWrapperExtension:

class OpenXRExtensionWrapperExtension : public Object, public OpenXRExtensionWrapper, public OpenXRCompositionLayerProvider {

The problem occurs when OpenXRExtensionWrapperExtension::register_extension_wrapper() registers this class with OpenXRAPI::register_extension_wrapper() AS AN OpenXRExtensionWrapper:

void OpenXRExtensionWrapperExtension::register_extension_wrapper() {
OpenXRAPI::register_extension_wrapper(this);
}

The OpenXRAPI extension wrappers saves OpenXRExtensionWrapper instances in a Vector<OpenXRExtensionWrapper *> and later deletes them using memdelete.

The problem is that the memory layout of OpenXRExtensionWrapperExtension is as follows (sizes specific to Windows/X64):

  • Object base: 408 bytes
  • OpenXRExtensionWrapper base: 8 bytes
  • OpenXRCompositionLayerProvider base: 8 bytes
  • OpenXRExtensionWrapperExtension members: 688 bytes
  • Total size = 1112 bytes

When the OpenXRExtensionWrapperExtension::register_extension_wrapper() registers itself, it has to cast its this pointer to an OpenXRExtensionWrapper* which involves adding 408 to the pointer value to jump over the Object. When memdelete() is called on this pointer, it fails because the pointer isn't a block of allocated memory.

This pattern is fully supported in C++ using the normal new/delete, as the compiler will generate a "virtual deleting destructor" in the objects VTABLE, so a delete by any class type will correctly call the real destructor, then adjust the pointer to the start of the object and delete the memory.

Godot's use of memdelete() appears to bypass this machinery preventing safe deletion of multiple-inheritance classes.

Steps to reproduce

Run Godot with the godot_openxr_vendors extension (or any extension providing OpenXR extensions) and then terminate the application.

Minimal reproduction project (MRP)

N/A

@dsnopek
Copy link
Contributor

dsnopek commented Feb 22, 2024

I just posted PR #88688 which provides a way to fix memdelete() in this case. However, I don't know if a general solution that modifies core for such an uncommon pattern is really the right way to go... :-/

I'm going to make an alternative PR that just fixes this for OpenXRExtensionWrapper, in case that doesn't turn out to be popular.

@dsnopek
Copy link
Contributor

dsnopek commented Feb 22, 2024

And here's PR #88689 which is just a targeted fix for this specific bug :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment