godotengine / godotengine/godot
Crash calling function on null pointer of already freed GDExtension
- Dominant language
- C++
- Stars
- 117k
- Forks
- 26.8k
- PR merge metrics
- PR metrics pending
Description
### Tested versions
4.2.2, 4.3-stable, 4.3 6699ae7897658e44efc3cfb2cba91c11a8f5aa6a
Master untested but the problem is visible in the code.
godot-cpp 4.2-cherrypicks-7
### System information
Windows 11/64, RTX 3070, Vulkan
### Issue description
I believe there are two bugs here:
1. Somewhere in gdextension `Dictionary[String] -> TypedArray` it isn't properly freeing the GDextResource when it is reassigned to a new dictionary pointer with `dict = Dictionary();` That's triggering the engine to go down an unexpected code path on cleanup triggering the second bug:
2. In the engine, ObjectDB::cleanup() code and Resource::GDCLASS macro assume `_extension` is a null pointer. However under the above circumstances and maybe others this assumption is wrong and the engine attempts to call `is_class("Node")` on a gdextension resource where the extension has already been freed. The function call on a null pointer causes a crash.
The second problem is here in this sequence and call stack:
```
register_core_types.cpp::unregister_core_types() {
...
memdelete(gdextension_manager);
...
ObjectDB::cleanup(); // crash
}
ObjectDB::cleanup() {
...
if (OS::get_singleton()->is_stdout_verbose()) {
...
if (obj->is_class("Node")) // crash
}
Resource::GDClass macro extended inline
virtual bool is_class(const String &p_class) const override {
if (_get_extension() &&
_get_extension()->is_class(p_class)) { // crash
return true;
}
return (p_class == (#m_class)) ? true : m_inherits::is_class(p_class);
}
```
In the Resource macro, _get_extension() returns a non-null pointer to already freed memory. It was freed in unregister_core_types(). Since it's freed, it's very difficult to figure out what object was being queried, but with other debugging I've determined this situation is a Terrain3DResource. So, Godot freed the library, _extension is not-null and invalid, then the Resource macro attempts to call a function from the null pointer resulting in the crash.
Deleting gdextensions before everything is shutdown has caused more than one issue. Related https://github.com/godotengine/godot/issues/95310. The fix for this didn't solve the fundamental problem.


-------
Here's more information about this. It may be a gdextension bug.
Using godot-cpp 4.2-cherrypicks-7
* The object it's crashing on querying is a Terrain3DRegion, which is just a basic custom resource with only native data types. A region stores our data and image maps for a geographical section of the terrain. https://github.com/TokisanGames/Terrain3D/blob/main/src/terrain_3d_region.h
* The engine crashes in this code block https://github.com/godotengine/godot/blob/4.3/core/object/object.cpp#L2287-L2305
* If --verbose is specified (but also when I run it in my msvc debugger) this code runs. Terrain3D has already been unloaded at this point. The Terrain3DRegion object still has a pointer to it in _extension. is_class() runs the Resource::GDCLASS macro which calls a function from the null pointer.
* Of note, whichever objects that are triggered in these conditions, `object_slots[i].validator` is true. The instances haven't been cleared yet. Not all of my gdextension resources or even Terrain3DRegions fall under these conditions. I can load up regions from disk and not have an issue. The conditions are set only once we start editing data, which means region duplicates, EditorUndoRedoManager and so on.
* I have cleared the undo history on exit before the offending code runs but it still crashes.
* Testing each component, I've determined the cause for the conditions comes down to this:
```c++
void Terrain3DEditor::start_operation(const Vector3 &p_global_position) {
//_undo_data = Dictionary(); // ** causes latent crash **
_undo_data.clear(); // workaround
```
We've been using the first line to get a new pointer and make our backup work properly. This alone sets up the conditions for a crash. If I replace the first with the second line it no longer crashes.
We also have this other bit of code where `_undo_data` is used that can also be adjusted to prevent the crash:
```c++
void Terrain3DEditor::_store_undo() {
...
Dictionary redo_data;
_undo_data["edited_regions"] = _original_regions; // ** causes latent crash **
redo_data["edited_regions"] = _edited_regions;
```
I can `return` right before the _undo_data line or comment it out and it won't crash. If I return right after or leave it uncommented, it will crash.
`_original_regions` and `_edited_regions` are both `TypedArray`.
`_undo_data` and `redo_data` are both Dictionaries. They store essentially the same type of data. The only fundamental differences between them are the first one is a class member, and it gets reset by assignment.
* I was able to fix the problem on our end by using `_undo_data.clear()` in the first block. And use `_undo_data.duplicate()` in the second block where we pass the data to the undo/redo manager.
### Steps to reproduce
* Download the artifact below to a new project.
* Open and restart twice.
* Run the demo in a debug engine build or in a debugger.
* Click Terrain3D, then the foliage brush and start painting. Cover a few 32x32 areas. Keep it loaded at least 5 seconds.
On quit it should crash in Resource, on the GDClass macro.
### Minimal reproduction project (MRP)
Artifact https://github.com/TokisanGames/Terrain3D/actions/runs/11317731831
Contributor guide
Research direction
Start with the shutdown sequence in register_core_types.cpp, ObjectDB::cleanup() in core/object/object.cpp, and the Resource::GDCLASS macro. Reproduce the crash with the linked Terrain3D artifact, then trace GDExtension lifetime through Dictionary reassignment and gdextension_manager cleanup. Done means the regression is understood and covered by a test or reproduction that no longer crashes during shutdown.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, godot
- Domain
- game-dev
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100