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

Fix unsafe uses of Callable.is_null() #91247

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Apr 27, 2024

Callable.is_null() is not equivalent to !Callable.is_valid() and doesn't guarantee the call is valid. It might not cause issues generally but there are cases when this would cause issues, so best to be safe. One major case when this will fail is when the object has been freed.

_FORCE_INLINE_ bool is_null() const {
return method == StringName() && object == 0;
}

bool Callable::is_valid() const {
if (is_custom()) {
return get_custom()->is_valid();
} else {
return get_object() && get_object()->has_method(get_method());
}
}

@AThousandShips AThousandShips added this to the 4.3 milestone Apr 27, 2024
@AThousandShips AThousandShips requested review from a team as code owners April 27, 2024 09:59
@AThousandShips AThousandShips force-pushed the callable_fix branch 3 times, most recently from f4cef10 to 2894b4d Compare April 27, 2024 14:17
@AThousandShips
Copy link
Member Author

AThousandShips commented Apr 27, 2024

Restored some Object related ones that should use is_valid but can't due to classes not being registered yet, missed a comment about it in the code

Added a clarifying note to these two instances to prevent confusion

`Callable.is_null()` is not equivalent to `!Callable.is_valid()` and
doesn't guarantee the call is valid.
@akien-mga akien-mga merged commit 947f5a8 into godotengine:master Apr 29, 2024
16 checks passed
@AThousandShips AThousandShips deleted the callable_fix branch April 29, 2024 08:16
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants