GDExtension: Allocate GDExtensionScriptInstanceInfo2 for compatibility on the heap to prevent crash

[dsnopek]

Alternative approach to fixing crash introduced in #78634

PR #81205 is the first approach

I'm honestly not sure which approach is better at this point :-)

UPDATE: Let's go with this one!

This PR has the advantage that there's only the extra copy and memory usage when using the compatibility path.

[Sauermann]

Your implementation looks nearly like mine for this approach (didn't test it):

diff --git a/core/extension/gdextension_interface.cpp b/core/extension/gdextension_interface.cpp
index 24517d71e4..08d04a3cbf 100644
--- a/core/extension/gdextension_interface.cpp
+++ b/core/extension/gdextension_interface.cpp
@@ -1043,35 +1043,36 @@ static void gdextension_ref_set_object(GDExtensionRefPtr p_ref, GDExtensionObjec
 
 #ifndef DISABLE_DEPRECATED
 static GDExtensionScriptInstancePtr gdextension_script_instance_create(const GDExtensionScriptInstanceInfo *p_info, GDExtensionScriptInstanceDataPtr p_instance_data) {
-       const GDExtensionScriptInstanceInfo2 info_2 = {
-               p_info->set_func,
-               p_info->get_func,
-               p_info->get_property_list_func,
-               p_info->free_property_list_func,
-               p_info->property_can_revert_func,
-               p_info->property_get_revert_func,
-               p_info->get_owner_func,
-               p_info->get_property_state_func,
-               p_info->get_method_list_func,
-               p_info->free_method_list_func,
-               p_info->get_property_type_func,
-               p_info->has_method_func,
-               p_info->call_func,
-               nullptr, // notification_func.
-               p_info->to_string_func,
-               p_info->refcount_incremented_func,
-               p_info->refcount_decremented_func,
-               p_info->get_script_func,
-               p_info->is_placeholder_func,
-               p_info->set_fallback_func,
-               p_info->get_fallback_func,
-               p_info->get_language_func,
-               p_info->free_func,
-       };
+       GDExtensionScriptInstanceInfo2 *info_2 = memnew(GDExtensionScriptInstanceInfo2);
+
+       info_2->set_func = p_info->set_func;
+       info_2->get_func = p_info->get_func;
+       info_2->get_property_list_func = p_info->get_property_list_func;
+       info_2->free_property_list_func = p_info->free_property_list_func;
+       info_2->property_can_revert_func = p_info->property_can_revert_func;
+       info_2->property_get_revert_func = p_info->property_get_revert_func;
+       info_2->get_owner_func = p_info->get_owner_func;
+       info_2->get_property_state_func = p_info->get_property_state_func;
+       info_2->get_method_list_func = p_info->get_method_list_func;
+       info_2->free_method_list_func = p_info->free_method_list_func;
+       info_2->get_property_type_func = p_info->get_property_type_func;
+       info_2->has_method_func = p_info->has_method_func;
+       info_2->call_func = p_info->call_func;
+       info_2->notification_func = nullptr;
+       info_2->to_string_func = p_info->to_string_func;
+       info_2->refcount_incremented_func = p_info->refcount_incremented_func;
+       info_2->refcount_decremented_func = p_info->refcount_decremented_func;
+       info_2->get_script_func = p_info->get_script_func;
+       info_2->is_placeholder_func = p_info->is_placeholder_func;
+       info_2->set_fallback_func = p_info->set_fallback_func;
+       info_2->get_fallback_func = p_info->get_fallback_func;
+       info_2->get_language_func = p_info->get_language_func;
+       info_2->free_func = p_info->free_func;
 
        ScriptInstanceExtension *script_instance_extension = memnew(ScriptInstanceExtension);
        script_instance_extension->instance = p_instance_data;
-       script_instance_extension->native_info = &info_2;
+       script_instance_extension->native_info = info_2;
+       script_instance_extension->mark_native_info_for_cleanup();
        script_instance_extension->deprecated_native_info.notification_func = p_info->notification_func;
        return reinterpret_cast<GDExtensionScriptInstancePtr>(script_instance_extension);
 }
diff --git a/core/extension/gdextension_interface.h b/core/extension/gdextension_interface.h
index e719d7337b..e8fdc63422 100644
--- a/core/extension/gdextension_interface.h
+++ b/core/extension/gdextension_interface.h
@@ -2194,7 +2194,7 @@ typedef GDExtensionScriptInstancePtr (*GDExtensionInterfaceScriptInstanceCreate)
  *
  * Creates a script instance that contains the given info and instance data.
  *
- * @param p_info A pointer to a GDExtensionScriptInstanceInfo2 struct.
+ * @param p_info A pointer to a GDExtensionScriptInstanceInfo2 struct. Can be safely freed once the function returns.
  * @param p_instance_data A pointer to a data representing the script instance in the GDExtension. This will be passed to all the function pointers on p_info.
  *
  * @return A pointer to a ScriptInstanceExtension object.
diff --git a/core/object/script_language_extension.h b/core/object/script_language_extension.h
index eca208a2bd..bfc84c89dc 100644
--- a/core/object/script_language_extension.h
+++ b/core/object/script_language_extension.h
@@ -629,6 +629,8 @@ VARIANT_ENUM_CAST(ScriptLanguageExtension::CodeCompletionKind)
 VARIANT_ENUM_CAST(ScriptLanguageExtension::CodeCompletionLocation)
 
 class ScriptInstanceExtension : public ScriptInstance {
+       bool _cleanup_native_info = false;
+
 public:
        const GDExtensionScriptInstanceInfo2 *native_info;
        struct {
@@ -637,6 +639,10 @@ public:
 
        GDExtensionScriptInstanceDataPtr instance = nullptr;
 
+       void mark_native_info_for_cleanup() {
+               _cleanup_native_info = true;
+       }
+
 // There should not be warnings on explicit casts.
 #if defined(__GNUC__) && !defined(__clang__)
 #pragma GCC diagnostic push
@@ -831,6 +837,9 @@ public:
                if (native_info->free_func) {
                        native_info->free_func(instance);
                }
+               if (_cleanup_native_info) {
+                       memdelete(native_info);
+               }
        }
 
 #if defined(__GNUC__) && !defined(__clang__)

[maiself]

I like this one better, simpler and only coping for the compatibility path. 👍

[dsnopek]

Doing it this way makes me wonder if the deprecated_native_info struct should be allocated on the heap as well? Right now it doesn't matter because it only holds one pointer, but as soon as it holds two, it'll be weighing down non-compatibility instances more than necessary.

[Sauermann]

The argumentation for putting deprecated_native_info on the heap makes sense to me. However I can't estimate, how many instance are created when running a game and how big this memory impact actually is.

[dsnopek]

Alright, then let's go with this version!

The argumentation for putting deprecated_native_info on the heap makes sense to me. However I can't estimate, how many instance are created when running a game and how big this memory impact actually is.

Yeah, I think I'm going to leave this as-is for now. When we have to add another pointer there we can re-evaluate.

[maiself]

This also fixes things! 👍

[akien-mga]

Looks fine.

[Sauermann]

The changes make sure, that the GDExtensionScriptInstanceInfo2 stay on the heap during the duration of their lifetime, so theoretically this should fix the linked issue.
I haven't replicated the crash and so didn't test it.

[akien-mga]

Thanks!