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

C#: Fix crash with DisposablesTracker_OnGodotShuttingDown #78157

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

RedworkDE
Copy link
Member

@RedworkDE RedworkDE commented Jun 12, 2023

Fixes #77486
Also Fixes #77305

As described in the issue, because DisposablesTracker_OnGodotShuttingDown is called after getting rid of all script bindings, if deleting an object also deletes other objects that are later in the queue of managed objects to delete, the later managed object isn't marked as deleted and later a use-after-free occurs.
Thus move the DisposablesTracker_OnGodotShuttingDown call to before deleting the script bindings.
(Also the finalizing member is probably no longer needed, but I didn't test that)
MRP for the original issue: Test_77486_min.zip (ASAN only, simply start the project and close it again)

While I don't think this ha any side-effects beyond a slightly degraded shutdown performance, @neikeq should probably review this as there may be reasons to do things in the previous order that I missed.

Alternatively the root cause is that the managed shutdown deletes objects that are not owned by the managed side, but there is no way to check this.

@RedworkDE RedworkDE requested a review from a team as a code owner June 12, 2023 21:22
@RedworkDE RedworkDE force-pushed the net-shutting-down-abruptly branch from 7039a99 to 5fde287 Compare June 12, 2023 22:08
@raulsntos raulsntos added this to the 4.x milestone Jun 13, 2023
@raulsntos raulsntos requested a review from neikeq June 13, 2023 01:31
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

The change makes sense to me, but I couldn't reproduce the issue.

As for why the call to OnGodotShuttingDown was in the GDMono destructor. The DisposablesTracker was added in f88d890 to replace the old Mono cleanup.

I don't think the previous order was important, I think OnGodotShutdown just happend to be added there because we were replacing the Mono cleanup which naturally happened in the GDMono destructor. So it's probably fine to change the order.

Since I couldn't reproduce the issue it makes me feel uneasy about approving, and normally I'd prefer to move it to 4.3 so we can get more testing, but it seems we may need this PR and #84013 to fix an important regression in 4.2 (#82279). Also, I trust that @RedworkDE has tested it. So I'll approve the PR.

@akien-mga akien-mga merged commit 6afd320 into godotengine:master Oct 31, 2023
@akien-mga
Copy link
Member

Thanks!

@RedworkDE RedworkDE deleted the net-shutting-down-abruptly branch November 1, 2023 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants