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 crash when creating thread #53059

Merged
merged 1 commit into from
Oct 2, 2021
Merged

Conversation

timothyqiu
Copy link
Member

Fixes #53046

There are two different crashes:

  1. Did not check MethodBind * for null: it crashes when the target method does not exist.
  2. Did not validate target instance: if the target instance is freed before the new thread is started, it's undefined behavior to call its methods.
    • ObjectDB::instance_validate() is gone on master, so I changed target_instance from plain old Object * to ObjectID in order to validate it.

@akien-mga
Copy link
Member

Needs a rebase after #53053.

@RandomShaper
Copy link
Member

Just for the records, it's worth noting that, by doing the check inside _start_func() instead of start(), this is also protecting against a race condition where the original thread (or some other) frees the object just after having created the new thread, before the latter has had a chance to obtain the object pointer. That part of the protection will only take us so far, but I don't think it's a bad thing either. The important fact is that the important timespan covered by this protection, the same that we would get by having the check in start(), is still there, only extended to account for a potential race condition.

@akien-mga akien-mga merged commit 90f8eb7 into godotengine:master Oct 2, 2021
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the thread-obj branch October 3, 2021 07:08
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.

Godot crash when creating thread
3 participants