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

InstanceId can be used to bypass Gd not being Send #886

Open
lilizoey opened this issue Sep 4, 2024 · 2 comments
Open

InstanceId can be used to bypass Gd not being Send #886

lilizoey opened this issue Sep 4, 2024 · 2 comments
Labels
bug c: core Core components ub Undefined behavior

Comments

@lilizoey
Copy link
Member

lilizoey commented Sep 4, 2024

Currently Gd does not implement Send, because it is unsound to send to another thread. However you can easily bypass this restriction by using InstanceId and calling Gd::try_from_instance_id().

Until we have a proper solution for multi-threaded code, this loophole should probably be available so that people who desperately need it can use it. But when we are going to support multi-threaded code, this loophole will need to be closed to ensure soundness.

My suggestion would be:

  • Make InstanceId not implement Send or Sync. This will make it impossible to bypass multi-threading by merely passing an InstanceId to another thread.
  • Make it impossible or unsafe to construct an InstanceId from a u64. This makes it so that you cant bypass the previous restriction by just passing the integer representation of an InstanceId to another thread.

This means we're effectively adding strict provenance to InstanceId. Where it is only safe to obtain an InstanceId which has a known provenance, and any attempt to create an InstanceId out of thin air would be library UB since it can potentially violate arbitrary restrictions.

I am not sure if it is possible to call try_from_instance_id another way, such as through Object::call(). If that is possible then this suggestion would likely mean that that would need to be made unsafe as well. That would be non-ideal, but if we are going to impose any extra soundness criteria on top of Godot it may be unavoidable.

@lilizoey lilizoey added c: core Core components ub Undefined behavior bug labels Sep 4, 2024
@Yarwin
Copy link
Contributor

Yarwin commented Sep 5, 2024

Note – in some cases InstanceId can be used as, well, InstanceId, used to recognize given instance while passing message from the thread. While it is not a huge deal (InstanceId stil can be converted to u64) by itself, proper deprecation warnings would be much appreciated.

Bigger issue might be some thread-safe APIs (according to docs - godot's servers – https://docs.godotengine.org/en/stable/tutorials/performance/thread_safe_apis.html#global-scope) that operate with InstanceIds. Example:
https://docs.godotengine.org/en/stable/classes/class_physicsserver3d.html#class-physicsserver3d-method-area-attach-object-instance-id
https://docs.godotengine.org/en/stable/classes/class_physicsserver3d.html#class-physicsserver3d-method-area-get-object-instance-id

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

No branches or pull requests

3 participants