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 dangling Variants (3.2) #38119

Merged
merged 2 commits into from
Apr 24, 2020

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Apr 22, 2020

This commit addresses multiple issues with Variants that point to an Object which is later released, when it's tried to be accessed again.

Formerly, while running on the debugger the system would check if the instance id was still valid to print warnings or return special values. Some cases weren't being warned about whatsoever. Moreover, on release builds you would experiment undefined behavior; specially, crashes.

Also, a newly allocated Object could happen to be allocated at the same memory address of an old one, making cases of use hard to find and having Variants pointing to the old one magically reassigned to the new.

This commit fixes all these problems:

  • While debugging you still have instance id checks so you get warnings about trying to operate on objects that have been already released.
  • But the debug behavior on those cases is now consistent: aside from returning the most reasonable value in read operations given the circumstances, you get a printed warning in all cases and with an easier to understand message.
    - On release builds, the instance id check is not performed, so no warnings are printed, but now the validity of the Variant is checked by more optimal means, allowing the system to act in a controlled manner in those cases, as if it was null, with no crashes, etc.
    UPDATE: This PR has been updated to remove the checks from release builds. A separate proposal about how to include them for those needing maximum safety will be written, so don't worry. What is done in this PR is already good stuff.

All that said, in the name of performance there's still one possible case of undefined behavior: in multithreaded scripts there would be a race condition between a thread freeing an Object and another one trying to operate on it. The latter may not realize the Object has been freed soon enough. But that's a case of bad scripting that was never supported anyway.


Part of the credit for this goes to @reduz, since this is the result of implementing an approach he suggested to me with some additional ideas of mine.


This code is generously donated by IMVU.

@RandomShaper RandomShaper added this to the 3.2 milestone Apr 22, 2020
@RandomShaper RandomShaper requested review from reduz and hpvb April 22, 2020 17:20
@RandomShaper RandomShaper marked this pull request as ready for review April 22, 2020 17:53
Needed for the next commit. That's the only place in all the Godot code base
where that's attempted and it's not needed because Objects are not meant to
be copied that way.
@RandomShaper RandomShaper force-pushed the imvu/new_dangling_variant_fix branch 2 times, most recently from e22da10 to 31d9d24 Compare April 23, 2020 11:00
This commit addresses multiple issues with `Variant`s that point to an `Object`
which is later released, when it's tried to be accessed again.

Formerly, **while running on the debugger the system would check if the instance id was
still valid** to print warnings or return special values. Some cases weren't being
warned about whatsoever.

Also, a newly allocated `Object` could happen to be allocated at the same memory
address of an old one, making cases of use hard to find and having **`Variant`s pointing
to the old one magically reassigned to the new**.

This commit makes the engine realize all these situations **under debugging**
so you can detect and fix them. Running without a debugger attached will still
behave as it always did.

Also the warning messages have been extended and made clearer.

All that said, in the name of performance there's still one possible case of undefined
behavior: in multithreaded scripts there would be a race condition between a thread freeing
an `Object` and another one trying to operate on it. The latter may not realize the
`Object` has been freed soon enough. But that's a case of bad scripting that was never
supported anyway.
@akien-mga akien-mga merged commit 5d5848d into godotengine:3.2 Apr 24, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

@godotengine/bugsquad This likely fixes many open issues about Variant references being wrongly reassigned to a random node after free. Feel free to hunt them down and close them as fix (another fix for that issue was already merged in master a while ago).

@oeleo1
Copy link

oeleo1 commented Jun 18, 2020

That's great! Thanks for fixing this critical issue. What's the plan for release builds in 3.x ? Currently we're bound to debug builds since our release builds crash systematically. I couldn't find the mentioned new proposal for that, so could you please provide the link if it exists? Thanks!

@RandomShaper
Copy link
Member Author

I haven't had time yet, but I'll give it a higher priority in my backlog.

@oeleo1
Copy link

oeleo1 commented Jul 14, 2020

As far as I can tell, there's not much we can do as users in GDscript to avoid the issue, or is there? We don't use threads, but we do use audio which is multi-threaded under the hood. Unfortunately release builds currently seem to be trading safety for speed which is unfortunate and prohibitive. Since safety prevails, we're stuck to debug builds with the assumed cost of lower fps.

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.

None yet

4 participants