utopia-rise / godot-kotlin-jvm

Godot Kotlin JVM Module
MIT License
613 stars 44 forks source link

Current Memory Manager can cause RefCounted instances to be freed incorrectly. #616

Closed CedNaru closed 4 months ago

CedNaru commented 5 months ago

This is quite complex to explain so hang in there. The following example is extreme, but its purpose is to show that incorrect behaviours are possible with the way we handle memory within the binding. Take the following Kotlin Script:

@RegisterClass
class MyScript {

  private var ref: RefCounted? = RefCounted()

  @RegisterFunction
  fun getRef(): RefCounted{
    val ret = ref
    ref = null
    return ret
  }
}

In this configuration, ref is the only reference to the RefCounted instance in both the JVM and C++. Let's say that some time happens between the creation of this script and the call to getRef()so we are sure that ref is bound by the MemoryManage

Now we have this GDScript (but it could be the internals of any Godot native type as well).


var ref: Refcounted
var my_script := MyScript.new()

func _enter_tree() -> void
    ref = my_script.get_ref()

We prepare an instance of this gdscript and add it later to the scene tree (so MyScript is created, and its RefCounted as well). When getRef()get called, we set ref to null and return. The native pointer of the RefCounted is going to be placed in the SharedBuffer but once we return from the JVM, there will be no reference left of the RefCounted (ref is null and the local variable ret is no longer in the scope) It means that between the moment we return from the JVM and the moment the pointer of the RefCounted instance is parsed from the buffer and converted back to a Variant or Ref<>, there will be nothing keeping it alive. Its counter won't immediately reach 0 because it can only go from 1 to 0 when the MemoryManager calls CPP, which will happen. When you return from the JVM, all reference to ref are gone, and so the instance will be collected and the MemoryManager will use the GC callback to call CPP and decrement the counter.

At this moment, 2 things can happen depending on the timing of the 2 threads (the one running the scripts and the one running the MemoryManager):

In both case, it leads to corrupting the memory in one of the 2 sides. The situation is complex, but the root cause is simple: We allow for a JVM reference to be collected when returning to cpp.