Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mathgeniuszach opened this issue Sep 27, 2024 · 5 comments

Comments

@mathgeniuszach
Copy link

Tested versions

  • Reproducible in 4.4.dev2, 4.3.stable, 4.2.2.stable, 4.1.4.stable, and 4.0.4.stable

System information

Godot v4.3.stable (77dcf97) - 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
Copy link

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
Copy link
Author

mathgeniuszach commented Sep 27, 2024

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
Copy link

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
Copy link
Member

@mathgeniuszach
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants