Prevent exclusive systems from being used as observers #19033
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alice-i-cecile
added
C-Bug
|
Is there any chance this can be backported to 0.16.1? Given it's a soundness issue I was thinking |
hukasu
approved these changes
May 4, 2025
|
I realized this might require a migration guide if anyone is doing this today... but then I thought it would be even better to put the guide in the error message so that anyone trying this in the future also gets guidance. So I did that instead! |
|
having it on the error is very smart, but i don't think it removes the need of the migration guide |
alice-i-cecile
approved these changes
May 5, 2025
alice-i-cecile
added
S-Ready-For-Final-Review
Agreed, we should tackle this in follow-up at some point. |
mockersf
approved these changes
May 5, 2025
andrewzhurov
pushed a commit
to andrewzhurov/bevy
that referenced
this pull request
May 17, 2025
) # Objective Prevent using exclusive systems as observers. Allowing them is unsound, because observers are only expected to have `DeferredWorld` access, and the observer infrastructure will keep pointers that are invalidated by the creation of `&mut World`. See https://github.com/bevyengine/bevy/actions/runs/14778342801/job/41491517847?pr=19011 for a MIRI failure in a recent PR caused by an exclusive system being used as an observer in a test. ## Solution Have `Observer::new` panic if `System::is_exclusive()` is true. Document that method, and methods that call it, as panicking. (It should be possible to express this in the type system so that the calls won't even compile, but I did not want to attempt that.) ## Testing Added a unit test that calls `World::add_observer` with an exclusive system.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Prevent using exclusive systems as observers. Allowing them is unsound, because observers are only expected to have
DeferredWorldaccess, and the observer infrastructure will keep pointers that are invalidated by the creation of&mut World.See https://github.com/bevyengine/bevy/actions/runs/14778342801/job/41491517847?pr=19011 for a MIRI failure in a recent PR caused by an exclusive system being used as an observer in a test.
Solution
Have
Observer::newpanic ifSystem::is_exclusive()is true. Document that method, and methods that call it, as panicking.(It should be possible to express this in the type system so that the calls won't even compile, but I did not want to attempt that.)
Testing
Added a unit test that calls
World::add_observerwith an exclusive system.