Skip to content

Fix unsound observer initialization - #25656

Merged
alice-i-cecile merged 3 commits into
bevyengine:mainfrom
chescock:unsound-observer-initialize
Sep 5, 2026
Merged

Fix unsound observer initialization#25656
alice-i-cecile merged 3 commits into
bevyengine:mainfrom
chescock:unsound-observer-initialize

Conversation

@chescock

@chescock chescock commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Objective

Fix potential UB during observer initialization that was noticed in #25507 (comment).

When calling System::initialize on an observer system, we pass &mut World while also holding a &mut to the observer system itself. This is usually fine, since none of the first-party systems or system params will do anything invalid. Even adding observers is fine because the system is stored behind a box and won't be moved if we reallocate a table. But it's valid for a system to despawn the observer during its own initialize, which would then deallocate the System while it was running initialize!

Solution

Store the system in the Observer component in an Option, and remove it before calling initialize, so that we can hold &mut World without conflict.

Note that we were already doing that for conditions, so this does not need an additional component lookup. I was even able to remove one lookup by replacing the system and conditions at the same time.

Also skip the call to register_observer if the observer was despawned before then. I didn't investigate whether that call would actually cause problems, but I can't see any way that it would be helpful in that case.

@chescock chescock added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 2, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in ECS Sep 2, 2026
@alice-i-cecile alice-i-cecile added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Uncontroversial This work is generally agreed upon labels Sep 2, 2026

@alice-i-cecile alice-i-cecile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a regression test for the pathological observer please? That seems like a nice little addition to the miri test suite. Seems like a nice fix otherwise though!

@hymm hymm added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 2, 2026
@alice-i-cecile

Copy link
Copy Markdown
Member

CI failure, then I'll merge <3

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 2, 2026
@chescock

chescock commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

CI failure, then I'll merge <3

Augh, conditional compilation again! Hopefully that push fixed it.

There is some danger that I'll make a PR soon to make fn refresh_hotpatch() always exist and just be a no-op when hotpatching is disabled, so that this doesn't bite anyone again...

@chescock chescock added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 3, 2026
@alice-i-cecile
alice-i-cecile added this pull request to the merge queue Sep 4, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 4, 2026
@alice-i-cecile
alice-i-cecile added this pull request to the merge queue Sep 5, 2026
Merged via the queue into bevyengine:main with commit dca1c42 Sep 5, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in ECS Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants