From bf9f288c7dc1f03232077408a5ac49b88a5e0718 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Verschelde?= <rverschelde@gmail.com>
Date: Wed, 5 May 2021 10:26:34 +0200
Subject: [PATCH] Fix crash with user-defined `ResourceFormatLoader.load`

There's still some fishy recursive relationship between `load_interactive` and
`load` which needs to be investigated here, but this patch solves the crash
when returning an error code in user-defined `load`.

Fixes #48463.
---
 core/io/resource_loader.cpp | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/core/io/resource_loader.cpp b/core/io/resource_loader.cpp
index 1c23e419eed1..01afa3a9e5c1 100644
--- a/core/io/resource_loader.cpp
+++ b/core/io/resource_loader.cpp
@@ -131,18 +131,6 @@ class ResourceInteractiveLoaderDefault : public ResourceInteractiveLoader {
 	ResourceInteractiveLoaderDefault() {}
 };
 
-Ref<ResourceInteractiveLoader> ResourceFormatLoader::load_interactive(const String &p_path, const String &p_original_path, Error *r_error) {
-	//either this
-	Ref<Resource> res = load(p_path, p_original_path, r_error);
-	if (res.is_null()) {
-		return Ref<ResourceInteractiveLoader>();
-	}
-
-	Ref<ResourceInteractiveLoaderDefault> ril = Ref<ResourceInteractiveLoaderDefault>(memnew(ResourceInteractiveLoaderDefault));
-	ril->resource = res;
-	return ril;
-}
-
 bool ResourceFormatLoader::exists(const String &p_path) const {
 	return FileAccess::exists(p_path); //by default just check file
 }
@@ -160,16 +148,33 @@ void ResourceFormatLoader::get_recognized_extensions(List<String> *p_extensions)
 	}
 }
 
+// Warning: Derived classes must override either `load` or `load_interactive`. The base code
+// here can trigger an infinite recursion otherwise, since `load` calls `load_interactive`
+// vice versa.
+
+Ref<ResourceInteractiveLoader> ResourceFormatLoader::load_interactive(const String &p_path, const String &p_original_path, Error *r_error) {
+	// Warning: See previous note about the risk of infinite recursion.
+	Ref<Resource> res = load(p_path, p_original_path, r_error);
+	if (res.is_null()) {
+		return Ref<ResourceInteractiveLoader>();
+	}
+
+	Ref<ResourceInteractiveLoaderDefault> ril = Ref<ResourceInteractiveLoaderDefault>(memnew(ResourceInteractiveLoaderDefault));
+	ril->resource = res;
+	return ril;
+}
+
 RES ResourceFormatLoader::load(const String &p_path, const String &p_original_path, Error *r_error) {
+	// Check user-defined loader if there's any. Hard fail if it returns an error.
 	if (get_script_instance() && get_script_instance()->has_method("load")) {
 		Variant res = get_script_instance()->call("load", p_path, p_original_path);
 
-		if (res.get_type() == Variant::INT) {
+		if (res.get_type() == Variant::INT) { // Error code, abort.
 			if (r_error) {
 				*r_error = (Error)res.operator int64_t();
 			}
-
-		} else {
+			return RES();
+		} else { // Success, pass on result.
 			if (r_error) {
 				*r_error = OK;
 			}
@@ -177,7 +182,7 @@ RES ResourceFormatLoader::load(const String &p_path, const String &p_original_pa
 		}
 	}
 
-	//or this must be implemented
+	// Warning: See previous note about the risk of infinite recursion.
 	Ref<ResourceInteractiveLoader> ril = load_interactive(p_path, p_original_path, r_error);
 	if (!ril.is_valid()) {
 		return RES();