JoinIter::get allows mutable aliasing without the user writing any unsafe code.

[allen-marshall]

Description

Using the JoinIter::get method, it is possible for a user of specs to create two mutable references to the same component data, without the user writing any unsafe code. Here is a simple program that illustrates the issue:

use specs::{Builder, Component, DenseVecStorage, Join, World, WorldExt, WriteStorage};

#[derive(Default)]
struct TestComponent {
    value: u32,
}

impl Component for TestComponent {
    type Storage = DenseVecStorage<TestComponent>;
}

fn main() {
    let mut world = World::new();
    world.register::<TestComponent>();
    let entity = world.create_entity().with(TestComponent::default()).build();
    world.maintain();

    let mut storage: WriteStorage<TestComponent> = world.write_storage();
    let entities = world.entities();

    let mut join_iter = (&mut storage).join();
    let aliased_ref_0 = join_iter.get(entity, &entities).unwrap();
    let aliased_ref_1 = join_iter.get(entity, &entities).unwrap();

    println!("aliased_ref_0 is initially {}.", aliased_ref_0.value);
    aliased_ref_1.value += 1;
    println!(
        "After change to aliased_ref_1, aliased_ref_0 is now {}.",
        aliased_ref_0.value
    );
}

When I run this program, I get the following output:

aliased_ref_0 is initially 0.
After change to aliased_ref_1, aliased_ref_0 is now 1.

The issue doesn't seem too hard to avoid as long as I only use JoinIter for its intended purpose as an iterator, but even so, the fact that I can violate Rust's aliasing rules without writing unsafe seems like an issue. Perhaps JoinIter::get needs to be made private to the specs crate, or maybe it just needs to be marked unsafe? I imagine the same issue can occur with JoinIter::get_unchecked, though I haven't tested it.

Meta

Rust version: 1.37.0 (2018 edition)
Specs version / commit: 0.15.1
Operating system: Ubuntu 18.04.3 LTS 64-bit

Reproduction

Steps to reproduce the behavior:

1. Compile the Rust program given above.
2. Run the program.
3. See in the output that the value at aliased_ref_0 changed even though aliased_ref_0 wasn't mutated directly.

Expected behavior

I would expect the above code not to compile, at least not without modifying it to include an unsafe block.

[WaDelma]

Here is version of that where the lifetimes are explicit:

    use specs::{Builder, Component, DenseVecStorage, Join, World, WorldExt, WriteStorage, Storage, Read, Entity};
    use specs::join::JoinIter;
    use shred::FetchMut;
    use specs::storage::MaskedStorage;
    use specs::world::EntitiesRes;

    #[derive(Default)]
    struct TestComponent {
        value: u32,
    }

    impl Component for TestComponent {
        type Storage = DenseVecStorage<TestComponent>;
    }

    #[test]
    fn testi() {
        let mut world = World::new();
        world.register::<TestComponent>();
        let entity = world.create_entity().with(TestComponent::default()).build();
        world.maintain();

        let mut storage: WriteStorage<TestComponent> = world.write_storage();
        let entities = world.entities();

        fn a<'a: 'b, 'b>(
            j: JoinIter<&'b mut Storage<'a, TestComponent, FetchMut<'a, MaskedStorage<TestComponent>>>>
        ) -> JoinIter<&'b mut Storage<'a, TestComponent, FetchMut<'a, MaskedStorage<TestComponent>>>>
        {
            j
        }
        let mut join_iter = a((&mut storage).join());
        fn b<'a: 'b, 'b, 'c, 'd>(
            j: &'c mut JoinIter<&'b mut Storage<'a, TestComponent, FetchMut<'a, MaskedStorage<TestComponent>>>>,
            e: Entity,
            es: &'d Read<'a, EntitiesRes>
        ) -> &'b mut TestComponent {
            JoinIter::get(j, e, es).unwrap()
        }
        let aliased_ref_0 = b(&mut join_iter, entity, &entities);
        let aliased_ref_1 = b(&mut join_iter, entity, &entities);

        println!("aliased_ref_0 is initially {}.", aliased_ref_0.value);
        aliased_ref_1.value += 1;
        println!(
            "After change to aliased_ref_1, aliased_ref_0 is now {}.",
            aliased_ref_0.value
        );
    }

and from this the reason why it's allowed is clear: 'c isn't connected with 'b.

[WaDelma]

This basically the old streaming vs non-streaming iterator thing, but manifested in different form.

I thought I was clever before by adding this API for JoinIter as that allows easy way of getting multiple components for an entity.

To fix this we would need to either prevent using get with mutable references or create separate machinery for this stuff.

[pythonesque]

This API is very clearly unsound and needs to be removed.