From f6f2be7577f0f952c446064252339b7142109ea7 Mon Sep 17 00:00:00 2001 From: Ignacio Etcheverry Date: Sat, 1 Dec 2018 02:23:55 +0100 Subject: [PATCH] Fix crash due to ~CSharpInstance() being called on freed instance This would be the case when calling SetScript on an object with a C# script. --- modules/mono/csharp_script.cpp | 9 ++++++--- modules/mono/csharp_script.h | 3 +++ modules/mono/glue/base_object_glue.cpp | 20 ++++++++++++-------- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp index e3df0246ab9..3c818898e64 100644 --- a/modules/mono/csharp_script.cpp +++ b/modules/mono/csharp_script.cpp @@ -993,7 +993,7 @@ void CSharpLanguage::set_language_index(int p_idx) { void CSharpLanguage::release_script_gchandle(Ref &p_gchandle) { - if (!p_gchandle->is_released()) { // Do not locking unnecessarily + if (!p_gchandle->is_released()) { // Do not lock unnecessarily SCOPED_MUTEX_LOCK(get_singleton()->script_gchandle_release_mutex); p_gchandle->release(); } @@ -1003,7 +1003,7 @@ void CSharpLanguage::release_script_gchandle(MonoObject *p_expected_obj, Refis_released()) { // Do not locking unnecessarily + if (!p_gchandle->is_released()) { // Do not lock unnecessarily SCOPED_MUTEX_LOCK(get_singleton()->script_gchandle_release_mutex); MonoObject *target = p_gchandle->get_target(); @@ -1754,7 +1754,8 @@ CSharpInstance::CSharpInstance() : base_ref(false), ref_dying(false), unsafe_referenced(false), - predelete_notified(false) { + predelete_notified(false), + destructing_script_instance(false) { } CSharpInstance::~CSharpInstance() { @@ -1771,7 +1772,9 @@ CSharpInstance::~CSharpInstance() { if (mono_object) { MonoException *exc = NULL; + destructing_script_instance = true; GDMonoUtils::dispose(mono_object, &exc); + destructing_script_instance = false; if (exc) { GDMonoUtils::set_pending_exception(exc); diff --git a/modules/mono/csharp_script.h b/modules/mono/csharp_script.h index 269d7ebe424..501e0d9d6d2 100644 --- a/modules/mono/csharp_script.h +++ b/modules/mono/csharp_script.h @@ -198,6 +198,7 @@ class CSharpInstance : public ScriptInstance { bool ref_dying; bool unsafe_referenced; bool predelete_notified; + bool destructing_script_instance; Ref script; Ref gchandle; @@ -218,6 +219,8 @@ class CSharpInstance : public ScriptInstance { public: MonoObject *get_mono_object() const; + _FORCE_INLINE_ bool is_destructing_script_instance() { return destructing_script_instance; } + virtual bool set(const StringName &p_name, const Variant &p_value); virtual bool get(const StringName &p_name, Variant &r_ret) const; virtual void get_property_list(List *p_properties) const; diff --git a/modules/mono/glue/base_object_glue.cpp b/modules/mono/glue/base_object_glue.cpp index d718c3cc616..58916c5283a 100644 --- a/modules/mono/glue/base_object_glue.cpp +++ b/modules/mono/glue/base_object_glue.cpp @@ -54,8 +54,10 @@ void godot_icall_Object_Disposed(MonoObject *p_obj, Object *p_ptr) { if (p_ptr->get_script_instance()) { CSharpInstance *cs_instance = CAST_CSHARP_INSTANCE(p_ptr->get_script_instance()); if (cs_instance) { - cs_instance->mono_object_disposed(p_obj); - p_ptr->set_script_instance(NULL); + if (!cs_instance->is_destructing_script_instance()) { + cs_instance->mono_object_disposed(p_obj); + p_ptr->set_script_instance(NULL); + } return; } } @@ -82,12 +84,14 @@ void godot_icall_Reference_Disposed(MonoObject *p_obj, Object *p_ptr, bool p_is_ if (ref->get_script_instance()) { CSharpInstance *cs_instance = CAST_CSHARP_INSTANCE(ref->get_script_instance()); if (cs_instance) { - bool r_owner_deleted; - cs_instance->mono_object_disposed_baseref(p_obj, p_is_finalizer, r_owner_deleted); - if (!r_owner_deleted && !p_is_finalizer) { - // If the native instance is still alive and Dispose() was called - // (instead of the finalizer), then we remove the script instance. - ref->set_script_instance(NULL); + if (!cs_instance->is_destructing_script_instance()) { + bool r_owner_deleted; + cs_instance->mono_object_disposed_baseref(p_obj, p_is_finalizer, r_owner_deleted); + if (!r_owner_deleted && !p_is_finalizer) { + // If the native instance is still alive and Dispose() was called + // (instead of the finalizer), then we remove the script instance. + ref->set_script_instance(NULL); + } } return; }