godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.95k stars 21.07k forks source link

Potential use after free on GDScript callable methods; methods do not capture self object when used as callables. #97523

Open mathgeniuszach opened 1 week ago

mathgeniuszach commented 1 week ago

Tested versions

System information

Godot v4.3.stable (77dcf97d8) - NixOS #1-NixOS SMP PREEMPT_DYNAMIC Wed Sep 18 17:24:10 UTC 2024 - X11 - GLES3 (Compatibility)

Issue description

Consider the following code:

extends Node

class Test:
    func print_hi():
        print("hi")

func other() -> Callable:
    return Test.new().print_hi

func _ready():
    other().call()

The expected result would be that it prints "hi" to the console; however, this instead pushes the following error:

Attempt to call function 'null::print_hi (Callable)' on a null instance.

Calling print_hi() from within other() works fine, but the self object seems to be cleaned up after the callable is returned, resulting in calling it on a null instance. I believe the callable should capture its self object when bound like this.

Steps to reproduce

  1. Create a new function.
  2. Create new object inside of that function and return a bound callable method from it.
  3. Call the function from some other code, and call .call() or .callv() on the returned callable.
  4. Observe the editor error, which says that the initial object is null.

Minimal reproduction project (MRP)

test.zip

jinyangcruise commented 1 week ago

I guess the reason is that in Test.new().print_hi, Test.new() returns a RefCounted object which you don't have a reference of that making the object is freed automatically. So it became null. If you define class Test extends Object, then the error will disappear but it may cause memory leaking if you don't hold a reference of this object and free it manually when you don't need it.

mathgeniuszach commented 1 week ago

Yes, I am aware of that. I am simply saying that the callable should increment the reference count by one until it goes out of scope.

jinyangcruise commented 1 week ago

Thank you, I am aware of that. I am simply saying that the callable should increment the reference count by one until it goes out of scope.

Oops, I just noticed that you mentioned I believe the callable should capture its self object when bound like this.

AThousandShips commented 1 week ago

Might have been fixed by:

mathgeniuszach commented 1 week ago

@AThousandShips Tested with https://godotengine.github.io/godot-commit-artifacts/ under https://github.com/godotengine/godot/commit/ae45d19ad69d4a23146ab04c85adca00188a0d94 on a vm, the issue still seems to exist there. The Linux build failed to compile through nix.