search

bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.86k stars 3.54k forks source link

UnsafeWorldCell::world_metadata is unsound given its safety contract #15289

Open Victoronz opened 1 month ago

Victoronz commented 1 month ago

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.

SkiFire13 commented 1 month ago

I'm not sure the proposed fix will work. Even if you compute the pointer to e.g. Storages with addr_of, you'll still need read permissions to dereference it, or even read it fields without creating intermediate references. All of these ultimately will result in UB if there is a &mut World around, which IMO is the real problem.

The only issue that would avoid is if something was currently mutably borrowing a different field of World and you tried to read a different field, but AFAIK this is never the case in the current implementation.

Victoronz commented 1 month ago

Note that there has already been sentiment and an issue for refactoring to this access pattern: #12214

Victoronz commented 1 month ago

Even without mitigating most UB itself, I think improving the clarity of the safety contracts and implementation with this change is worth doing.

The only issue that would avoid is if something was currently mutably borrowing a different field of World and you tried to read a different field, but AFAIK this is never the case in the current implementation.

Even if it currently doesn't happen, this still seems like a benefit.