bevyengine / bevyengine/bevy

UnsafeWorldCell::world_metadata is unsound given its safety contract

Open
#15,289 4 comments 0 reactions 1 assignee Claimed by @Victoronz View on GitHub
A-ECS C-Bug D-Unsafe I-Unsound S-Ready-For-Implementation
Dominant language
Rust
Stars
48.2k
Forks
4.8k
Avg merge
3d 22h
Merged PRs (30d)
161

Description

## What problem does this solve or what need does it fill?
`UnsafeWorldCell` stores a `*mut World` to avoid aliasing with any world references. However, its `world_metadata` method returns a `&World` while its safety contract reads:
> must only be used to access world metadata

This does not include ensuring that no mutable references to `World` exist before creating a `&World`. Mutable and immutable references must not alias, regardless of what is actually being accessed.

This method leads to the creation of intermediary `World` references inside various, safe metadata methods of `UnsafeWorldCell`.
`UnsafeWorldCell::storages()` also creates an intermediary reference, and while unsafe, does not state this in its safety section either.

Most UB that can be created by these methods will remain even if these intermediary references are removed, but the safety contracts and implementation can be made clearer by removing `world_metadata` and these intermediates.

~~Miri does not seem to catch this, or at least it has not until #15276, where it started failing in a multithreaded executor test, presumably because of parallel data access, though it is not yet clear why.~~

## What solution would you like?

Just like the `*mut World` `UnsafeWorldCell` stores to be safe to use while world references are live, accessing the fields on this `World` should too use pointers for all but the final reference being returned, which can be done with `addr_of!`
The `world_metadata` method should be removed, and all its uses in the metadata access methods should be replaced with the above approach. Other users of the API can use the dedicated metadata methods on `UnsafeWorldCell`, or call `UnsafeWorldCell::world()` if they can satisfy the safety contract.
`storages()` can be fixed the with `addr_of!` the same way.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.