UnsafeWorldCell::world_metadata is unsound given its safety contract

[Victoronz]

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]

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]

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

[Victoronz]

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.

[chescock]

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.

I believe the UnsafeWorldCell itself is proof that no mutable references to World exist.

It can only be created from either &World or &mut World, either of which prevent other &mut World references that don't come from the UnsafeWorldCell. And the only way to get a &mut World from the UnsafeWorldCell is world_mut, but that is an unsafe fn whose safety requirements prevent it from being called while other code is calling world_metadata.