Skip to content

Fix Variant -> Gd conversions not taking into account dead objects #1033

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

Merged
merged 5 commits into from
Feb 2, 2025

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Feb 2, 2025

Variant::to(), Variant::try_to() and Gd::from_variant() did not recognize when the object living inside the variant was dead. This sometimes caused a panic "constructed RawGd weak pointer with instance ID 0", but generally caused UB, since the object pointer inside the variant was already dangling at that point.


Also improves error messages when converting variants of other types to object.
Old:

Gd cannot be null: null

new:

cannot convert from INT to OBJECT: 123


Furthermore, this fixes Debug impl hitting infinite recursion upon encountering a dead object.
Now prints <Freed Object> instead.

@Bromeon Bromeon added bug c: core Core components ub Undefined behavior labels Feb 2, 2025
/// Does not check again that the variant has type `OBJECT`.
pub(crate) fn is_object_dead(&self) -> bool {
// There is no dedicated API, however the string repr has a special value for dead objects.
self.stringify() == "<Freed Object>".into()
Copy link
Contributor

@Yarwin Yarwin Feb 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about is_instance_valid?

Note: It is deprecated for public use, but reserving it for internal use (pub(crate) won't allow to use it even in itest) seems to be better idea than relying on repr for nullptr checks 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, great idea! Totally forgot that that one takes variant as arguments 😅 who knew that would come in handy one day...

@Bromeon Bromeon force-pushed the bugfix/variant-dead-objects branch 2 times, most recently from 6e7043c to 25552db Compare February 2, 2025 15:22
@Bromeon Bromeon force-pushed the bugfix/variant-dead-objects branch from 25552db to 981f29a Compare February 2, 2025 15:57
@Bromeon Bromeon force-pushed the bugfix/variant-dead-objects branch from f1ec846 to 6f5383e Compare February 2, 2025 16:19
@Bromeon Bromeon added this pull request to the merge queue Feb 2, 2025
Merged via the queue into master with commit b9b3f9f Feb 2, 2025
15 checks passed
@Bromeon Bromeon deleted the bugfix/variant-dead-objects branch February 2, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: core Core components ub Undefined behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants