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

server/headless: pure virtual method called with --gdnative-generate-json-api #36582

Closed
Tracked by #39196
lyuma opened this issue Feb 26, 2020 · 12 comments · Fixed by #48081
Closed
Tracked by #39196

server/headless: pure virtual method called with --gdnative-generate-json-api #36582

lyuma opened this issue Feb 26, 2020 · 12 comments · Fixed by #48081

Comments

@lyuma
Copy link
Contributor

lyuma commented Feb 26, 2020

Godot version:
3.2-stable

OS/device including version:
Linux
$ lsb_release -r
Release: 18.04
$ uname -a
Linux server 4.15.0-72-generic #81-Ubuntu SMP Tue Nov 26 12:20:02 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

Issue description:
Running a platform=server build with --gdnative-generate-json-api results in a "pure virtual method called" segfault in AudioDriverDummy::thread_func()

$ bin/godot_server.x11.opt.tools.64 --gdnative-generate-json-api api.json
Godot Engine v3.2.stable.custom_build.4e7d75ccd - https://godotengine.org
 
pure virtual method called
terminate called without an active exception
Aborted (core dumped)

GDB stacktrace:

pure virtual method called
terminate called without an active exception

Thread 4 "godot_server.x1" received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff7e64700 (LWP 3900)]
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff6cc2801 in __GI_abort () at abort.c:79
#2  0x00007ffff76b5957 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007ffff76bbab6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007ffff76bbaf1 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007ffff76bc8bf in __cxa_pure_virtual () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00005555577dbabf in AudioDriverDummy::thread_func () at servers/audio/audio_driver_dummy.cpp:68
#7  0x000055555614b3ec in ThreadPosix::thread_callback () at drivers/unix/thread_posix.cpp:74
#8  0x00007ffff7bbd6db in start_thread (arg=0x7ffff7e64700) at pthread_create.c:463
#9  0x00007ffff6da388f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb) 

Steps to reproduce:

  1. git checkout 3.2-stable
  2. /usr/bin/scons platform=server target=release_debug -j4
  3. bin/godot_server.x11.opt.tools.64 --gdnative-generate-json-api api.json

Minimal reproduction project:
N/A

@twaritwaikar
Copy link
Contributor

twaritwaikar commented Feb 27, 2020

Could be caused by #36367 but it does really look like that from the stack trace. The PR removes driver/dummy/audio_driver_dummy.h which isn't being used anywhere. The stack trace points to servers/audio/audio_driver_dummy.h instead

@akien-mga
Copy link
Member

@IronicallySerious that PR is not in 3.2-stable, so it's something else.

@lyuma
Copy link
Contributor Author

lyuma commented Feb 27, 2020

There is a AudioDriverDummy declared in static initialization as part of AudioDriverManager in servers/audio_server.h:
static AudioDriverDummy AudioDriverManager::dummy_driver;

The init function is called from:

(gdb) bt
#0  AudioDriverDummy::init () at servers/audio/audio_driver_dummy.cpp:36
#1  0x00005555575a43fc in AudioDriverManager::initialize () at servers/audio_server.cpp:191
#2  0x00005555557aa5f0 in OS_Server::initialize () at platform/server/os_server.cpp:91
#3  0x00005555557c59bb in Main::setup2 () at main/main.cpp:1201
#4  0x00005555557cb2ff in Main::setup () at main/main.cpp:1146
#5  0x00005555557a66b4 in main () at platform/server/godot_server.cpp:38

The destructor is called from:

(gdb) bt
#0  AudioDriverDummy::~AudioDriverDummy () at servers/audio/audio_driver_dummy.cpp:134
#1  0x00007ffff6cc5041 in __run_exit_handlers (status=0, listp=0x7ffff706d718 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, 
    run_dtors=run_dtors@entry=true) at exit.c:108
#2  0x00007ffff6cc513a in __GI_exit (status=<optimized out>) at exit.c:139
#3  0x0000555555caf322 in NativeScriptLanguage::init () at modules/gdnative/nativescript/nativescript.cpp:1085
#4  0x0000555557ba0b03 in ScriptServer::init_languages () at core/script_language.cpp:177
#5  0x00005555557c6af0 in Main::setup2 () at main/main.cpp:1364
#6  0x00005555557cb2ff in Main::setup () at main/main.cpp:1146
#7  0x00005555557a66b4 in main () at platform/server/godot_server.cpp:38

Hence, this pure virtual function call is a use-after-free caused by the thread continuing to run during the C++ static destruction phase.

Because exit() is not being called from a conventional place, it means that ::finish() is never called on any components that were previously ::init()ed. The destructor does not clean up the created threads, so this could be a plain old race condition.

I am able to avert this crash with the following workaround:

--- servers/audio/audio_driver_dummy.cpp	2020-02-26 22:21:54.194735819 +0000
+++ servers/audio/audio_driver_dummy.cpp	2020-02-27 13:00:48.436284974 +0000
@@ -132,5 +132,5 @@
 };
 
 AudioDriverDummy::~AudioDriverDummy(){
-
+	finish();
 };

However, I do not wish to submit this as a PR, as this goes against the existing style for servers, where deallocation of resources is done inside ::finish() rather than the destructor. It is also generally frowned upon to do cleanup of static objects in the destructor because the order is unspecified and may differ across machines or platforms.

@akien-mga
Copy link
Member

There's already code in AudioDriver::finish() that takes care of calling finish() on each driver:

godot/servers/audio_server.cpp

Lines 1072 to 1076 in ac44657

void AudioServer::finish() {
for (int i = 0; i < AudioDriverManager::get_driver_count(); i++) {
AudioDriverManager::get_driver(i)->finish();
}

So not sure while it's not called/not sufficient here.

@kuruk-mm
Copy link
Contributor

There's already code in AudioDriver::finish() that takes care of calling finish() on each driver:

godot/servers/audio_server.cpp

Lines 1072 to 1076 in ac44657

void AudioServer::finish() {
for (int i = 0; i < AudioDriverManager::get_driver_count(); i++) {
AudioDriverManager::get_driver(i)->finish();
}

So not sure while it's not called/not sufficient here.

Seems the destructor of AudioDriverDummy is beeing called before this.

As @lyuma it's being destroyed when static objects are deleted

Maybe dummy_driver is should be an singleton and non static?

@kuruk-mm
Copy link
Contributor

kuruk-mm commented Feb 27, 2020

I made dummy_driver a static pointer to have control of his destruction
It seems good for me and it's working as expected, what do you think? This is the right solution? @akien-mga @IronicallySerious

kuruk-mm@00a7eaa

@KoBeWi
Copy link
Member

KoBeWi commented Dec 21, 2020

Can anyone still reproduce this bug in Godot 3.2.3 or any later release?

@luispadron
Copy link

@KoBeWi Yes, I'm on a custom build v3.2.4.beta.custom_build.7d45e6764 and when using the server build and running the --gdnative-generate-json-api i get the same error as OP

@luispadron
Copy link

Looks like also reported more recently in #43953

@Calinou Calinou changed the title server: pure virtual method called with --gdnative-generate-json-api server/headless: pure virtual method called with --gdnative-generate-json-api Jan 3, 2021
@luispadron
Copy link

luispadron commented Jan 3, 2021

Just tested @kuruk-mm's fix and while it does fix the virtual function crash the command does not seem to generate anything. Instead it simply runs godot.

Output:

./bin/godot_server.osx.opt.debug.64 --gdnative-generate-json-api api.json
Godot Engine v3.2.4.beta.custom_build.1f5bf8551 - https://godotengine.org

Running outside of a directory with a project file produces this:

./bin/godot_server.osx.opt.debug.64 --gdnative-generate-json-api api.json
Error: Couldn't load project data at path ".". Is the .pck file missing?
If you've renamed the executable, the associated .pck file should also be renamed to match the executable's name (without the extension).
ALERT: ALERT!: Error: Couldn't load project data at path ".". Is the .pck file missing?
If you've renamed the executable, the associated .pck file should also be renamed to match the executable's name (without the extension).

Is --gdnative-generate-json-api not intended for headless/server builds? I ask because I think it would make dockerizing a server/headless build with the Godot C++ module a lot simpler since that module requires generating the latest api.json. Which only seems possible with a normal build of godot.

Edit:

Seems --gdnative-generate-json-api command requires tools=yes to be set. Which means a server build cannot generate api.json. Looking at the source it seems a lot of the code required for this command are not defined when tools=no. Would this be possible to change 🤔Either way the solution proposed above adds functionality that was previously throwing errors (for headless builds).

@Bromeon
Copy link
Contributor

Bromeon commented Dec 21, 2021

This still seems to be a problem for Godot versions <= 3.3.
I haven't experienced the issue for >= 3.3.1, but I haven't tested very extensively, either.
Could it be that this has been fixed in the meantime (maybe unknowingly)?

We discovered the issue during CI pipelines based on GitHub Actions, which use the headless version of Godot. The pure virtual method call occurs only sporadically, but this makes each CI run non-deterministic.

@akien-mga
Copy link
Member

This was fixed by #48081.

@akien-mga akien-mga modified the milestones: 4.0, 3.4 Dec 21, 2021
bors bot added a commit to godot-rust/gdnative that referenced this issue Jan 1, 2022
833: Feature flag for custom Godot version r=Bromeon a=Bromeon

Enables the use of custom Godot builds in a straightforward way.
This includes older versions such as Godot 3.2, for which `api.json` is no longer shipped along.

Previous process (see also [book](https://godot-rust.github.io/book/advanced-guides/custom-godot.html)):
* create a local copy of godot-rust
* Run `godot --gdnative-generate-json-api`
* replace `api.json` inside `gdnative-binding`

New process:
* enable feature `custom-godot`
* make sure `godot` executable is in path **OR** set `GODOT_BIN` env var
* done

This supersedes the previous `bindings` feature, which is now removed.

I also added CI support for previous Godot version 3.3.1, so far only for integration tests. Originally I wanted to use Godot 3.2 (the oldest supported version), however godotengine/godot#36582 prevented headless versions from generating `api.json` reliably without crashing. This has been fixed for versions >= 3.3.1.

There is still testing to be done; feedback is always appreciated! 🙂 

Closes #640. Thanks for the great ideas in that issue.

Co-authored-by: Jan Haller <bromeon@gmail.com>
KarimHamidou added a commit to KarimHamidou/godot-rust that referenced this issue Feb 6, 2023
833: Feature flag for custom Godot version r=Bromeon a=Bromeon

Enables the use of custom Godot builds in a straightforward way.
This includes older versions such as Godot 3.2, for which `api.json` is no longer shipped along.

Previous process (see also [book](https://godot-rust.github.io/book/advanced-guides/custom-godot.html)):
* create a local copy of godot-rust
* Run `godot --gdnative-generate-json-api`
* replace `api.json` inside `gdnative-binding`

New process:
* enable feature `custom-godot`
* make sure `godot` executable is in path **OR** set `GODOT_BIN` env var
* done

This supersedes the previous `bindings` feature, which is now removed.

I also added CI support for previous Godot version 3.3.1, so far only for integration tests. Originally I wanted to use Godot 3.2 (the oldest supported version), however godotengine/godot#36582 prevented headless versions from generating `api.json` reliably without crashing. This has been fixed for versions >= 3.3.1.

There is still testing to be done; feedback is always appreciated! 🙂 

Closes #640. Thanks for the great ideas in that issue.

Co-authored-by: Jan Haller <bromeon@gmail.com>
GuilhermeOrceziae added a commit to GuilhermeOrceziae/godot-rust that referenced this issue Feb 9, 2023
833: Feature flag for custom Godot version r=Bromeon a=Bromeon

Enables the use of custom Godot builds in a straightforward way.
This includes older versions such as Godot 3.2, for which `api.json` is no longer shipped along.

Previous process (see also [book](https://godot-rust.github.io/book/advanced-guides/custom-godot.html)):
* create a local copy of godot-rust
* Run `godot --gdnative-generate-json-api`
* replace `api.json` inside `gdnative-binding`

New process:
* enable feature `custom-godot`
* make sure `godot` executable is in path **OR** set `GODOT_BIN` env var
* done

This supersedes the previous `bindings` feature, which is now removed.

I also added CI support for previous Godot version 3.3.1, so far only for integration tests. Originally I wanted to use Godot 3.2 (the oldest supported version), however godotengine/godot#36582 prevented headless versions from generating `api.json` reliably without crashing. This has been fixed for versions >= 3.3.1.

There is still testing to be done; feedback is always appreciated! 🙂 

Closes #640. Thanks for the great ideas in that issue.

Co-authored-by: Jan Haller <bromeon@gmail.com>
hesuteia added a commit to hesuteia/godot-rust that referenced this issue Feb 11, 2023
833: Feature flag for custom Godot version r=Bromeon a=Bromeon

Enables the use of custom Godot builds in a straightforward way.
This includes older versions such as Godot 3.2, for which `api.json` is no longer shipped along.

Previous process (see also [book](https://godot-rust.github.io/book/advanced-guides/custom-godot.html)):
* create a local copy of godot-rust
* Run `godot --gdnative-generate-json-api`
* replace `api.json` inside `gdnative-binding`

New process:
* enable feature `custom-godot`
* make sure `godot` executable is in path **OR** set `GODOT_BIN` env var
* done

This supersedes the previous `bindings` feature, which is now removed.

I also added CI support for previous Godot version 3.3.1, so far only for integration tests. Originally I wanted to use Godot 3.2 (the oldest supported version), however godotengine/godot#36582 prevented headless versions from generating `api.json` reliably without crashing. This has been fixed for versions >= 3.3.1.

There is still testing to be done; feedback is always appreciated! 🙂 

Closes #640. Thanks for the great ideas in that issue.

Co-authored-by: Jan Haller <bromeon@gmail.com>
ecobiubiu added a commit to ecobiubiu/open-rust that referenced this issue Mar 30, 2023
833: Feature flag for custom Godot version r=Bromeon a=Bromeon

Enables the use of custom Godot builds in a straightforward way.
This includes older versions such as Godot 3.2, for which `api.json` is no longer shipped along.

Previous process (see also [book](https://godot-rust.github.io/book/advanced-guides/custom-godot.html)):
* create a local copy of godot-rust
* Run `godot --gdnative-generate-json-api`
* replace `api.json` inside `gdnative-binding`

New process:
* enable feature `custom-godot`
* make sure `godot` executable is in path **OR** set `GODOT_BIN` env var
* done

This supersedes the previous `bindings` feature, which is now removed.

I also added CI support for previous Godot version 3.3.1, so far only for integration tests. Originally I wanted to use Godot 3.2 (the oldest supported version), however godotengine/godot#36582 prevented headless versions from generating `api.json` reliably without crashing. This has been fixed for versions >= 3.3.1.

There is still testing to be done; feedback is always appreciated! 🙂 

Closes #640. Thanks for the great ideas in that issue.

Co-authored-by: Jan Haller <bromeon@gmail.com>
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.

8 participants