From 058604f5b845812a8b75a8014a1b243115ad65c7 Mon Sep 17 00:00:00 2001 From: Maxim Kulkin Date: Fri, 17 Feb 2023 05:18:58 -0500 Subject: [PATCH] Fix crash when saving resources with circular references When saving resources, marking of already seen resources was done too late, causing infinite loop traversing referenced resources and eventual stack overflow. The change marks traversed resource before descending to it's children, thus when this resource is encountered again, it is already marked as seen and traversal stops. --- core/io/resource_format_binary.cpp | 3 +- scene/resources/resource_format_text.cpp | 5 ++- tests/core/io/test_resource.h | 52 ++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/core/io/resource_format_binary.cpp b/core/io/resource_format_binary.cpp index 3037f603c4f..adae468d931 100644 --- a/core/io/resource_format_binary.cpp +++ b/core/io/resource_format_binary.cpp @@ -1960,6 +1960,8 @@ void ResourceFormatSaverBinaryInstance::_find_resources(const Variant &p_variant return; } + resource_set.insert(res); + List property_list; res->get_property_list(&property_list); @@ -1983,7 +1985,6 @@ void ResourceFormatSaverBinaryInstance::_find_resources(const Variant &p_variant } } - resource_set.insert(res); saved_resources.push_back(res); } break; diff --git a/scene/resources/resource_format_text.cpp b/scene/resources/resource_format_text.cpp index dc0240859e1..c02431efa80 100644 --- a/scene/resources/resource_format_text.cpp +++ b/scene/resources/resource_format_text.cpp @@ -1877,6 +1877,8 @@ void ResourceFormatSaverTextInstance::_find_resources(const Variant &p_variant, return; } + resource_set.insert(res); + List property_list; res->get_property_list(&property_list); @@ -1908,8 +1910,7 @@ void ResourceFormatSaverTextInstance::_find_resources(const Variant &p_variant, I = I->next(); } - resource_set.insert(res); //saved after, so the children it needs are available when loaded - saved_resources.push_back(res); + saved_resources.push_back(res); // Saved after, so the children it needs are available when loaded } break; case Variant::ARRAY: { diff --git a/tests/core/io/test_resource.h b/tests/core/io/test_resource.h index 20d76c88946..8fc2a2f040f 100644 --- a/tests/core/io/test_resource.h +++ b/tests/core/io/test_resource.h @@ -109,6 +109,58 @@ TEST_CASE("[Resource] Saving and loading") { loaded_child_resource_text->get_name() == "I'm a child resource", "The loaded child resource name should be equal to the expected value."); } + +TEST_CASE("[Resource] Breaking circular references on save") { + Ref resource_a = memnew(Resource); + resource_a->set_name("A"); + Ref resource_b = memnew(Resource); + resource_b->set_name("B"); + Ref resource_c = memnew(Resource); + resource_c->set_name("C"); + resource_a->set_meta("next", resource_b); + resource_b->set_meta("next", resource_c); + resource_c->set_meta("next", resource_b); + + const String save_path_binary = OS::get_singleton()->get_cache_path().path_join("resource.res"); + const String save_path_text = OS::get_singleton()->get_cache_path().path_join("resource.tres"); + ResourceSaver::save(resource_a, save_path_binary); + ResourceSaver::save(resource_a, save_path_text); + + const Ref &loaded_resource_a_binary = ResourceLoader::load(save_path_binary); + CHECK_MESSAGE( + loaded_resource_a_binary->get_name() == "A", + "The loaded resource name should be equal to the expected value."); + const Ref &loaded_resource_b_binary = loaded_resource_a_binary->get_meta("next"); + CHECK_MESSAGE( + loaded_resource_b_binary->get_name() == "B", + "The loaded child resource name should be equal to the expected value."); + const Ref &loaded_resource_c_binary = loaded_resource_b_binary->get_meta("next"); + CHECK_MESSAGE( + loaded_resource_c_binary->get_name() == "C", + "The loaded child resource name should be equal to the expected value."); + CHECK_MESSAGE( + !loaded_resource_c_binary->get_meta("next"), + "The loaded child resource circular reference should be NULL."); + + const Ref &loaded_resource_a_text = ResourceLoader::load(save_path_text); + CHECK_MESSAGE( + loaded_resource_a_text->get_name() == "A", + "The loaded resource name should be equal to the expected value."); + const Ref &loaded_resource_b_text = loaded_resource_a_text->get_meta("next"); + CHECK_MESSAGE( + loaded_resource_b_text->get_name() == "B", + "The loaded child resource name should be equal to the expected value."); + const Ref &loaded_resource_c_text = loaded_resource_b_text->get_meta("next"); + CHECK_MESSAGE( + loaded_resource_c_text->get_name() == "C", + "The loaded child resource name should be equal to the expected value."); + CHECK_MESSAGE( + !loaded_resource_c_text->get_meta("next"), + "The loaded child resource circular reference should be NULL."); + + // Break circular reference to avoid memory leak + resource_c->remove_meta("next"); +} } // namespace TestResource #endif // TEST_RESOURCE_H