GDScript segmentation fault cyclic reference

[AndreaCatania]

Godot version:
f733746

OS/device including version:
Fedora 32

Issue description:
When you have a cyclic reference using class_name and one of the file is a Resource a segmentation fault is raised.

handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib64/libc.so.6(+0x3ca70) [0x7ffff7716a70] (??:0)
[2] void call_with_variant_args_retc_dv<__UnexistingClass, bool, Object const*>(__UnexistingClass*, bool (__UnexistingClass::*)(Object const*) const, Variant const**, int, Variant&, Callable::CallError&, Vector<Variant> const&) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/./core/variant/binder_common.h:424)
[3] MethodBindTRC<bool, Object const*>::call(Object*, Variant const**, int, Callable::CallError&) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/./core/object/method_bind.h:554)
[4] GDScriptDataType::operator PropertyInfo() const (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/modules/gdscript/gdscript_function.h:145 (discriminator 1))
[5] GDScriptCompiler::_parse_class_level(GDScript*, GDScriptParser::ClassNode const*, bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/modules/gdscript/gdscript_compiler.cpp:2006)
[6] GDScriptCompiler::compile(GDScriptParser const*, GDScript*, bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/modules/gdscript/gdscript_compiler.cpp:2318 (discriminator 4))
[7] GDScript::reload(bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/modules/gdscript/gdscript.cpp:643)
[8] GDScriptCache::get_full_script(String const&, Error&, String const&) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/modules/gdscript/gdscript_cache.cpp:211)
[9] ResourceFormatLoaderGDScript::load(String const&, String const&, Error*, bool, float*, bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/modules/gdscript/gdscript.cpp:2104)
[10] ResourceLoader::_load(String const&, String const&, String const&, bool, Error*, bool, float*) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/io/resource_loader.cpp:190 (discriminator 6))
[11] ResourceLoader::_thread_load_function(void*) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/io/resource_loader.cpp:217 (discriminator 4))
[12] ResourceLoader::load(String const&, String const&, bool, Error*) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/io/resource_loader.cpp:581)
[13] ResourceLoaderText::load() (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/scene/resources/resource_format_text.cpp:459)
[14] ResourceFormatLoaderText::load(String const&, String const&, Error*, bool, float*, bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/scene/resources/resource_format_text.cpp:1271)
[15] ResourceLoader::_load(String const&, String const&, String const&, bool, Error*, bool, float*) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/io/resource_loader.cpp:190 (discriminator 6))
[16] ResourceLoader::load(String const&, String const&, bool, Error*) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/io/resource_loader.cpp:593 (discriminator 3))
[17] EditorNode::load_scene(String const&, bool, bool, bool, bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/editor/editor_node.cpp:3344 (discriminator 2))
[18] EditorNode::_load_open_scenes_from_config(Ref<ConfigFile>, String const&) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/editor/editor_node.cpp:4497 (discriminator 4))
[19] EditorNode::_load_docks() (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/editor/editor_node.cpp:4287 (discriminator 5))
[20] EditorNode::_sources_changed(bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/editor/editor_node.cpp:851)
[21] void call_with_variant_args_helper<EditorNode, bool, 0ul>(EditorNode*, void (EditorNode::*)(bool), Variant const**, Callable::CallError&, IndexSequence<0ul>) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/./core/variant/binder_common.h:217 (discriminator 4))
[22] void call_with_variant_args<EditorNode, bool>(EditorNode*, void (EditorNode::*)(bool), Variant const**, int, Callable::CallError&) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/./core/variant/binder_common.h:301)
[23] CallableCustomMethodPointer<EditorNode, bool>::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/./core/object/callable_method_pointer.h:98)
[24] Callable::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/variant/callable.cpp:55)
[25] Object::emit_signal(StringName const&, Variant const**, int) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/object/object.cpp:1067)
[26] Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/object/object.cpp:1122)
[27] EditorFileSystem::_notification(int) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/editor/editor_file_system.cpp:1129 (discriminator 3))
[28] EditorFileSystem::_notificationv(int, bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/editor/editor_file_system.h:107 (discriminator 14))
[29] Object::notification(int, bool) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/core/object/object.cpp:798)
[30] SceneTree::_notify_group_pause(StringName const&, int) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/scene/main/scene_tree.cpp:818)
[31] SceneTree::idle(float) (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/scene/main/scene_tree.cpp:447 (discriminator 2))
[32] Main::iteration() (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/main/main.cpp:2431)
[33] OS_LinuxBSD::run() (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/platform/linuxbsd/os_linuxbsd.cpp:261)
[34] /home/andrea/Workspace/godot_projects/wander/source/engine/godot/bin/godot.linuxbsd.tools.64(main+0x135) [0x1c8b99b] (/home/andrea/Workspace/godot_projects/wander/source/engine/godot/platform/linuxbsd/godot_linuxbsd.cpp:60)
[35] /lib64/libc.so.6(__libc_start_main+0xf2) [0x7ffff7701042] (??:0)
[36] /home/andrea/Workspace/godot_projects/wander/source/engine/godot/bin/godot.linuxbsd.tools.64(_start+0x2e) [0x1c8b7ae] (??:?)
-- END OF BACKTRACE --

Steps to reproduce:
Just launch this: TestCrash.zip

[AndreaCatania]

Some IRC chat, that may be useful to fix this issue:

odino: vnen, During the parsing of the member, that has the resource type, the following line is executed: https://github.com/godotengine/godot/blob/master/modules/gdscript/gdscript_compiler.cpp#L131
That function returns a Ref of the script, though it take out the pointer from it (see .ptr at the end of such line).
If we look into get_shallow_script to see what it does, we see that it creates the Reference but stores just the pointer and returns the reference: https://github.com/godotengine/godot/blob/master/modules/gdscript/gdscript_cache.cpp#L182-L188
Since the reference is never stored, the pointer memory is invalidated just after that function is done.
So that shallow_gdscript_cache https://github.com/godotengine/godot/blob/master/modules/gdscript/gdscript_cache.cpp#L188 and then script_type here https://github.com/godotengine/godot/blob/master/modules/gdscript/gdscript_compiler.cpp#L131
Have invalid pointers which may be the cause of the segmentation fault raised in this line: https://github.com/godotengine/godot/blob/master/modules/gdscript/gdscript_compiler.cpp#L2007
Which is the line just after the function _gdtype_from_datatype that triggers all the above mechanism.

Even if that's not the cause, I still think that the reference should be stored anyway (at least into shallow_gdscript_cache)

I've just converted shallow_gdscript_cache to store the Ref and the issue seems gone.
Here the diff:

diff --git a/modules/gdscript/gdscript.cpp b/modules/gdscript/gdscript.cpp
index 6b74abf15d..e557fda522 100644
--- a/modules/gdscript/gdscript.cpp
+++ b/modules/gdscript/gdscript.cpp
@@ -604,7 +604,7 @@ Error GDScript::reload(bool p_keep_state) {
 		if (!source_path.empty()) {
 			MutexLock lock(GDScriptCache::singleton->lock);
 			if (!GDScriptCache::singleton->shallow_gdscript_cache.has(source_path)) {
-				GDScriptCache::singleton->shallow_gdscript_cache[source_path] = this;
+				GDScriptCache::singleton->shallow_gdscript_cache[source_path] = Ref<GDScript>(this);
 			}
 		}
 	}
diff --git a/modules/gdscript/gdscript_cache.cpp b/modules/gdscript/gdscript_cache.cpp
index 95d24a8b08..b1df599195 100644
--- a/modules/gdscript/gdscript_cache.cpp
+++ b/modules/gdscript/gdscript_cache.cpp
@@ -185,7 +185,7 @@ Ref<GDScript> GDScriptCache::get_shallow_script(const String &p_path, const Stri
 	script->set_script_path(p_path);
 	script->load_source_code(p_path);
 
-	singleton->shallow_gdscript_cache[p_path] = script.ptr();
+	singleton->shallow_gdscript_cache[p_path] = script;
 	return script;
 }
 
@@ -213,7 +213,7 @@ Ref<GDScript> GDScriptCache::get_full_script(const String &p_path, Error &r_erro
 		return script;
 	}
 
-	singleton->full_gdscript_cache[p_path] = script.ptr();
+	singleton->full_gdscript_cache[p_path] = script;
 	singleton->shallow_gdscript_cache.erase(p_path);
 
 	return script;
@@ -222,7 +222,7 @@ Ref<GDScript> GDScriptCache::get_full_script(const String &p_path, Error &r_erro
 Error GDScriptCache::finish_compiling(const String &p_owner) {
 	// Mark this as compiled.
 	Ref<GDScript> script = get_shallow_script(p_owner);
-	singleton->full_gdscript_cache[p_owner] = script.ptr();
+	singleton->full_gdscript_cache[p_owner] = script;
 	singleton->shallow_gdscript_cache.erase(p_owner);
 
 	Set<String> depends = singleton->dependencies[p_owner];
diff --git a/modules/gdscript/gdscript_cache.h b/modules/gdscript/gdscript_cache.h
index 90c5884985..36a8112d44 100644
--- a/modules/gdscript/gdscript_cache.h
+++ b/modules/gdscript/gdscript_cache.h
@@ -71,8 +71,8 @@ public:
 class GDScriptCache {
 	// String key is full path.
 	HashMap<String, GDScriptParserRef *> parser_map;
-	HashMap<String, GDScript *> shallow_gdscript_cache;
-	HashMap<String, GDScript *> full_gdscript_cache;
+	HashMap<String, Ref<GDScript>> shallow_gdscript_cache;
+	HashMap<String, Ref<GDScript>> full_gdscript_cache;
 	HashMap<String, Set<String>> dependencies;
 
 	friend class GDScript;

reduz: scripts can get hooked up to the refcounting, we already use this for mono
reduz: so if you want to check that the cache is the sole owner of a gdscript resource, it should be possible without much effort
odino: assuming the script is always stored into the cache, my idea was to do: bool cache_only = refcount == 1 do you though to something similar?
reduz: I haven't checked much in detail vnen's work, but if that's the case, you can override Script::refcount_increment and Script::refcount_decrement in GDScript to do this check, you most likely don't need changes to the engine for that
odino: That's good to know, I was thinking more of a cache_validation() function into the script parser, thought handle it as you propose seems a much convenient thing.
odino: vnen, tell me if you want me to open a PR with such changes or in alternative I can put the above discoveries into the issue so you can work on it later.

[lyuma]

I'm unable to reproduce this bug on Godot master ( 85186bc )

@AndreaCatania if you are unable to reproduce this issue, we should close it.

[YuriSizov]

Assuming fixed based on the comment above (if not, it's likely reported with a new issue from a more recent version).