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

It is UB to use most of the library before init code is run #1046

Open
lilizoey opened this issue Feb 12, 2025 · 0 comments
Open

It is UB to use most of the library before init code is run #1046

lilizoey opened this issue Feb 12, 2025 · 0 comments
Labels
bug c: ffi Low-level components and interaction with GDExtension API ub Undefined behavior

Comments

@lilizoey
Copy link
Member

Most of the library relies on accessing the Godot binding, which is only initialized when the initialization code is run. But in most cases we only check if the initialization code is run when debug assertions are turned on.

This means that if you for instance make a unit test that accesses these APIs and run it in release mode, you'll hit UB.

Some possible solutions

  1. Check when debug assertions are turned off as well.

This would make everything safe, but would likely also incur performance penalties in situations where the initialization code will guaranteed be run, such as when actually being called by Godot.

The performance issues is why we only check when debug assertions are turned on in the first place.

  1. Track when code has been run using some kind of token in code.

This is hard to weave through every relevant part of the code, especially in a way that doesn't impact ergonomics. Additionally it will still require at least one check every time Godot calls the rust part of the code.

  1. Have an unsafe compile time flag for when the initialization code is run.

I am not entirely sure how we should decide when this flag is enabled or not. Initially i thought we could enable it when we're compiling as a cdylib but we cant actually determine that very easily, and it likely wouldn't work as expected if we could. As if you list several crate types in the Cargo.toml it'll compile against all of them at the same time.

So if for instance we had crate-type = ["rlib", "cdylib"] then anything behind cfg(crate-type = "cdylib") would also get compiled for the rlib target anyway.

See also this issue: rust-lang/rust#20267

We could also just require the user to explicitly enable this flag somehow, say a feature. But that means that suddenly people's code will become slower and it'd be an extra hurdle for making code run fast. But maybe we could add some useful warnings for when people are running the initialization code in release mode? since if the initialization code gets run in release mode they're likely in a situation where they'd want this feature enabled. We might also be able to add some other useful heuristics to give information to the user when they may be using it wrong.

  1. Make compiling in release mode unsafe

I.e we basically just say "you need to make sure you don't access the Godot API before the initialization code is run when you compile code in release mode".

This is very similar to the previous point but we basically just use debug_assertions as the relevant flag (or rather it's the inverse of the previous point, as not using this flag is what is unsafe).

However i think this is very strange from a Rust-perspective. In Rust it is generally assumed that --release wont make anything unsound, but will merely make the compiled code faster, compilation slower, and debug information worse. For beginners to the library it may be helpful, but in the long run i suspect it may be more confusing than anything else, leading to some subtle bugs.

@lilizoey lilizoey added bug c: ffi Low-level components and interaction with GDExtension API ub Undefined behavior labels Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: ffi Low-level components and interaction with GDExtension API ub Undefined behavior
Projects
None yet
Development

No branches or pull requests

1 participant