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

Segmentation fault when creating instance of a third-party class that has the name of a godot class but that is not in a namespace. #82812

Closed
ariesssss opened this issue Oct 4, 2023 · 10 comments · Fixed by #82973

Comments

@ariesssss
Copy link

Godot version

v4.1.1.stable.custom_build [bd6af8e]

System information

Godot v4.1.1.stable (bd6af8e) - Linux Mint 21.2 (Victoria) - Vulkan (Forward+) - integrated Intel(R) Graphics (RPL-P) () - 13th Gen Intel(R) Core(TM) i7-1360P (16 Threads)

Issue description

I'm working on a gdextension. It builds on other libraries that I have made a long time ago. One of those other libraries contains a class called Object. It is not declared in a namespace.

If in my C++ gdextension I declare a static ::Object * object {new ::Object {}}; then the godot editor, when opening a project that uses my gdextension, crashes immediately, typically with malloc(): unaligned tcache chunk. Also exporting the project from the command line fails.

Also other no-namespace classes with a name of a class in the godot namespace, like ::Node, ::Node3D, and ::CSGPolygon3D, cause segmentation faults. The class ::Vector3 is okay, however.

This issue was discusses at 36627-object-class-in-library.

Steps to reproduce

Start with for example the gdextension example from the godot docs. Add a header file Object.h like

class Object { static Object * const object; };

and a source file Object.cpp like

::Object * const ::Object::object {new ::Object {}};

Build the gdextension, open the project in the editor, the editor should crash.

Minimal reproduction project

I hope that the description of the steps to reproduce suffice.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 7, 2023

Thanks for the report!

I may be seeing similar behavior in one of my extensions.

I created a ScriptInstanceExtension class inside my GDExtension to provide an API similar to Godot's ScriptInstanceExtension because that class isn't exposed - instead you need to work through the GDExtensionScriptInstanceInfo2 struct, and this let me give it a nicer API. Anyway, I was getting a segfault when memdelete()ing an instance of this class, and when I walked through the code in the debugger, it appeared that it was calling ~ScriptInstanceExtension() for the unexposed engine class, rather than the class in my extension.

If I put my ScriptInstanceExtension in a namespace, then this stopped happening, and everything worked as expected.

This is really, really strange behavior. It's as if the dynamic linker is deciding to link the ScriptInstanceExtension class to the one in the engine?

I'm also on Linux (Ubuntu 22.04). I think the default behavior on Linux is to export all symbol, and I wonder if it's treating the Godot binary like an .so that's exporting all classes? I haven't had a chance to try this, but I wonder if we compiled Godot with -fvisibility=hidden it would help?

@dsnopek
Copy link
Contributor

dsnopek commented Oct 7, 2023

I did a little bit more research, and we could also try passing RTLD_DEEPBIND to dlopen() when loading the GDExtension. The man page says:

Place the lookup scope of the symbols in this shared object ahead of the global scope. This means that a self-contained object will use its own symbols in preference to global symbols with the same name contained in objects that have already been loaded.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 7, 2023

That last idea seems to fix it in my testing! (I didn't try the first idea, since that seemed like more work. ;-))

I created PR #82973 with the change

@ariesssss Can you try building Godot with that PR, and see if that fixes the problem for you?

@ariesssss
Copy link
Author

Thanks for your work. I'm not exactly sure where you want me to make the change to the call to dlopen(), can you give me a hint? Or alternatively, could you add a header file with

class Node
{
  public:

    explicit Node ();

    static Node * const x;
};

and a source with

#include "X.h"

Node::Node ()
{
}

Node * const Node::x {new Node {}};

When I then export the project, it hangs, it does not even output the first line:

Godot Engine v4.1.1.stable.custom_build.bd6af8e0e - https://godotengine.org

You are definitely on to something, because if I remove the constructor, then the export works fine. But I'm not convinced that this will fix my issue, because in my case the symbols are different. With nm -C I see symbols like

0000000000461fd0 T Node::Node()

for my Node and

00000000004bfe40 T godot::Node::_ready()

for ::godot::Node. I don't think RTLD_DEEPBIND can have an effect on these, can it?

@dsnopek
Copy link
Contributor

dsnopek commented Oct 7, 2023

@ariesssss:

I'm not exactly sure where you want me to make the change to the call to dlopen(), can you give me a hint?

It's the change to the Godot engine from this PR #82973 - you can apply the patch from there and recompile Godot, or you can download the build artifacts from CI (I just added some instructions to the PR description explaining how to do that).

for ::godot::Node. I don't think RTLD_DEEPBIND can have an effect on these, can it?

I'm hopeful that it'll fix the issue (it fixes a similar issue for me), but only testing will say for sure!

@ariesssss
Copy link
Author

You win, with your change it works!

However, something very strange is going on. It is as if godot is searching for a constructor of ::godot::Node, and finds a constructor for ::Node. Under normal circumstances I don't see how that can be even remotely possible. Your fix seems to me to be more like a fix of symptoms, and not a fix of the root cause of the issue.

I still think it is better to figure out what exactly goes wrong here. If not, the issue will undoubtedly come back to haunt us somewhere in the future. Just my 2 cents ...

@ariesssss
Copy link
Author

I just realised that in my case compiled code for the ::Node and ::godot::Node classes are actually in the same library. This prompted me to look for the existance of a ::Node class in the godot source code (as opposed to the godot-cpp source code). And indeed there is one, in scene/main/node.h! So now it is clear what happens. The ::godot::Node class in godot-cpp has nothing to do with it, it's the ::Node class in godot that is the issue here. And then it makes perfect sense why RTLD_DEEPBIND fixes the issue.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 8, 2023

Awesome, thanks for testing!

The ::godot::Node class in godot-cpp has nothing to do with it, it's the ::Node class in godot that is the issue here. And then it makes perfect sense why RTLD_DEEPBIND fixes the issue.

Yep! It's still weird to me, that the dynamic linker would mess with symbols that are defined - in my mental model of how it works, it only links undefined symbols, but I guess we learned something new. :-)

@ariesssss
Copy link
Author

I'm by no means an expert on dynamic linkers. But the gdextension library, opened by the dlopen call, is used to get the initialization function of the gdextension. The godot editor executable is otherwise not aware of that library. So I have a hard time understanding how symbols in the gdextension library can conflict with symbols loaded by the godot editor executable.

@ariesssss
Copy link
Author

Upon re-reading the explanation for RTLD_DEEPBIND

Place the lookup scope of the symbols in this shared object ahead of the global scope. This means that a self-contained object will use its own symbols in preference to global symbols with the same name contained in objects that have already been loaded.

This makes me think that it is actually the other way round (from what I assumed up to now, anyway): not my ::Node class interferes with the code in the godot editor, but the godot editor code interferes with my ::Node class.

I now do think that it is a bit unfortunate that the godot editor code is, by and large, not in a namespace.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants