CheckStorage soundness fixes #203

Aceeri · 2017-06-20T06:08:57Z

Splits CheckStorage into RestrictedStorage and CheckStorage for different usecases.

CheckStorage will just not borrow the storage, however it does not protect against the original storage having its bitset modified, so no component or entry is given for unchecked access. RestrictedStorage returns an entry and a reference back to the restricted storage every iteration. This allows restricted access to the storage to get and possibly modify components without invalidating the bitset.

WaDelma · 2017-06-20T10:06:21Z

src/storage/check.rs

+          D: 'a,
+{
+    #[inline]
+    fn assert_same_storage(&self, storage: &Storage<'a, T, D>) {


Would it be possible to use something like what this library does to get rid of these asserts? Just remembered the idea so I am not sure how feasible it would be.

Not sure... I don't really understand how it works. Another way to make the storage a unique type is by inserting a closure into it, however, that makes a god awful error that would probably just confuse people who use it.

Afaik it forces creation of new lifetime for the index and then ensures that that lifetimed index can only be used with right container. You actually need closure for it too. I didn't remember that was the case.

Maybe it isn't that great idea to use it.

torkleyy · 2017-07-01T03:41:12Z

src/storage/check.rs

-/// component.
-pub struct CheckStorage<'a, T, D> {
+/// Allows iterating over a storage without borrowing the storage itself.
+pub struct CheckStorage {


Looks good overall, but as far as I see it it requires a version bump, right?

torkleyy

Nice, thanks! Not sure about Entry yet, see below.

torkleyy · 2017-07-01T07:25:54Z

src/storage/restrict.rs

+{
+    /// Attempts to get the component related to the entity mutably.
+    ///
+    /// Functions similar to the normal `Storage::get_mut` implementation.


Maybe we should add a note that it only works for restrict_type::Normal?

torkleyy · 2017-07-01T07:27:40Z

src/storage/restrict.rs

+
+    /// Builds a parallel `RestrictedStorage`, does not allow mutably getting other components
+    /// aside from the current iteration.
+    pub fn par_restrict<'rf>(&'rf self)


Do we really need to methods for immutable restriction storages? Because they both don't allow mutable access, I don't see how their functionality is different.

Probably not necessary.

torkleyy · 2017-07-01T07:29:15Z

src/storage/restrict.rs

+}
+
+/// An entry to a storage.
+pub struct Entry<'rf, T>


At this point, we should evaluate which of Send / Sync this type conforms to.

I think it is safe for it to both be Send/Sync? It lives as long as the reference to the storage it belongs to, and only works in a Join, however I don't see how much use it has in those scenarios.

torkleyy · 2017-07-02T15:14:13Z

examples/full.rs

@@ -50,6 +50,7 @@ struct Sum(usize);
 struct IntAndBoolData<'a> {
    comp_int: ReadStorage<'a, CompInt>,
    comp_bool: WriteStorage<'a, CompBool>,
+    entities: Entities<'a>,


I don't think we need this.

Yeah, that was mainly for quick testing reasons.

torkleyy · 2017-07-02T15:16:14Z

src/storage/restrict.rs

+/// Generic flags for the restricted storage to determine
+/// the type of restriction.
+pub mod restrict_type {
+    pub struct Normal;


docs missing

torkleyy · 2017-07-06T07:30:24Z

src/lib.rs

-                  HashMapStorage, InsertResult, NullStorage, ReadStorage, Storage,
-                  UnprotectedStorage, VecStorage, WriteStorage};
+pub use storage::{BTreeStorage, CheckStorage, DenseVecStorage, DistinctStorage, Entry, FlaggedStorage,
+                  HashMapStorage, InsertResult, NullStorage, ReadStorage, restrict_type, RestrictedStorage,


Let's move restrict_type to the end of the list.

torkleyy · 2017-07-06T07:32:27Z

src/storage/tests.rs

            }
        }
    }

    #[test]
-    #[should_panic]
+    //#[should_panic]


err..is there anything wrong with the check or did you fix it already?

Forgot about that, just needed to add insertions to the s2 storage.

torkleyy · 2017-07-07T19:07:12Z

src/storage/restrict.rs

+{
+    bitset: B,
+    data: R,
+    entities: &'rf Fetch<'st, EntitiesRes>,


Could just use Entities here.

torkleyy · 2017-07-07T19:11:37Z

src/storage/mod.rs

@@ -18,6 +19,7 @@ use shred::Fetch;
 use {Component, EntitiesRes, Entity, Index, Join, ParJoin};

 mod check;
+mod restrict;


Could you move the module declaration down by one, please?

Ah sorry, forgot about the alphabetical order for modules.

torkleyy · 2017-07-21T11:55:51Z

I'm ready to merge this one, but I'd like to have another review.

kvark · 2017-07-23T18:13:15Z

src/storage/restrict.rs

+    /// Builds an immutable `RestrictedStorage` out of a `Storage`. Allows restricted
+    /// access to the inner components without allowing invalidating the
+    /// bitset for iteration in `Join`.
+    pub fn restrict<'rf>(&'rf self)


Is there a point in this read-only restricted storage? I'd assume that a regular MaskedStorage is equivalent since it doesn't allow adding/removing the components (and thus modifying the bitmask) either.

There isn't too much difference, this one just defers the "getting" of the component to when the user wants to get it, I'm not sure how useful that is with just being able to get an immutable reference though, so it probably doesn't have much of a point being immutable.

kvark · 2017-07-23T18:13:53Z

src/storage/check.rs

-/// component.
-pub struct CheckStorage<'a, T, D> {
+/// Allows iterating over a storage without borrowing the storage itself.
+pub struct CheckStorage {


What's a good use case for this new CheckStorage?

If you need to insert/remove components from the storage, this allows you to do that with a single loop rather than loop over all of them, find the ones you need to remove, store them in a Vec, then loop over those and remove them.

If you need to insert/remove components from the storage, this allows you to do that with a single loop rather than loop over all of them, find the ones you need to remove, store them in a Vec, then loop over those and remove them.

What is the performance difference with this single loop over the loop/deleteloop style?

@OvermindDL1 I'm not entirely sure? It would probably be faster if the compiler didn't optimize anything out, but in this case it probably is, so this is more for just ease of use along with making it easier to slide around the borrow checker.

kvark · 2017-07-24T15:10:13Z

Alright, the use case for CheckStorage is a bit weak, but it's there.
Read-only RestrictedStorage doesn't seem to have one, so I'd prefer this concept limited to mutability only.

Aceeri · 2017-07-25T00:15:31Z

Alright, removed the immutable restricted storage and just made the restrict_mut into restrict instead.

kvark · 2017-07-25T14:35:51Z

src/storage/restrict.rs

+    /// Functions similar to the normal `Storage::get_mut` implementation.
+    ///
+    /// Note: This only works if this is a parallel and mutable `RestrictedStorage`.
+    /// You can get one by using the `par_restrict_mut` method instead of the normal


documentation is outdated

kvark · 2017-07-25T14:36:29Z

src/storage/restrict.rs

+    ///
+    /// Functions similar to the normal `Storage::get_mut` implementation.
+    ///
+    /// Note: This only works if this is a parallel and mutable `RestrictedStorage`.


Note says this is for parallel, but the actual implementation is for Normal. Where is the truth?

Ah, I probably meant to put "non-parallel" there.

kvark · 2017-07-25T14:37:30Z

src/storage/restrict.rs

+/// the type of restriction.
+pub mod restrict_type {
+    /// Specifies that the `RestrictedStorage` cannot run in parallel.
+    pub struct Normal;


enums are better phantom types, and no need for a separate module, really.

enum NormalRestriction {} enum ParallelRestriction {}

True, didn't know about never types before the asset management stuff.

kvark · 2017-07-25T14:37:58Z

src/storage/restrict.rs

+    /// Specifies that the `RestrictedStorage` cannot run in parallel.
+    pub struct Normal;
+    /// Specifies that the `RestrictedStorage` can run in parallel.
+    pub struct Parallel;


could you explain a bit on how these types help you exactly?

I have to prevent get_mut on it when you use rayon, since that would mean you could get 2 mutable references to the same entity.

Basically, I'm enforcing that you can do this:

(&comp_int, &mut comp_float.par_restrict()) .par_join() .for_each(|(i, (entry, restricted))| { let f = restricted.get_mut_unchecked(&entry); });

But you can't do this:

(&comp_int, &mut comp_float.par_restrict()) .par_join() .for_each(|(i, (entry, restricted))| { let f = restricted.get_mut(&entity); // Some entity stored somewhere. });

kvark · 2017-07-26T01:52:30Z

src/storage/restrict.rs

+}
+
+
+unsafe impl<'rf, 'st: 'rf, B, T, R, ParallelRestriction> ParJoin for &'rf RestrictedStorage<'rf, 'st, B, T, R, ParallelRestriction>


I don't think you want ParallelRestriction in the impl block here

ParallelRestriction was the correct one, since it removes the get_mut method for the RestrictedStorage.

I read it as: you put ParallelRestriction into the impl<...> section, which defines a new local generic type with the same name as your phantom type. This is not what you want here.

Oh misread what you put, I thought you meant it should be a NormalRestriction. I must've done a s/RT/ParallelRestriction instead of just replacing the RestrictedStorage one.

kvark · 2017-07-26T01:52:38Z

src/storage/restrict.rs

+          B: Borrow<BitSet> + 'rf,
+{ }
+
+unsafe impl<'rf, 'st: 'rf, B, T, R, ParallelRestriction> ParJoin for &'rf mut RestrictedStorage<'rf, 'st, B, T, R, ParallelRestriction>


same here... what's the difference between these blocks anyway?

It does the same thing as the ParJoin implementation for Storage.

kvark · 2017-07-26T14:10:46Z

src/storage/restrict.rs

+    /// access to the inner components without allowing invalidating the
+    /// bitset for iteration in `Join`.
+    pub fn restrict<'rf>(&'rf mut self)
+        -> RestrictedStorage<'rf, 'st, &'rf BitSet, T, &'rf mut T::Storage, NormalRestriction>


I think lifetime elision could be used here to omit rf entirely?

kvark · 2017-07-26T14:12:03Z

src/storage/restrict.rs

+{
+    /// Gets the component related to the current entry without checking whether
+    /// the storage has it or not.
+    pub fn get_mut_unchecked(&mut self, entry: &Entry<'rf, T>) -> &mut T {


shouldn't this be unsafe?

This is actually safe since it requires an Entry, which can't live longer than the iteration.

torkleyy · 2017-07-27T14:52:16Z

src/storage/restrict.rs

+/// to only getting and modifying the components. That means nothing that would
+/// modify the inner bitset so the iteration cannot be invalidated. For example,
+/// no insertion or removal is allowed.
+pub struct RestrictedStorage<'rf, 'st: 'rf, B, T, R, RT>


Could we have an example usage?

torkleyy · 2017-08-09T16:07:23Z

@Aceeri Please rebase so we can merge this PR.

Remove immutable parallel restrict Impl Send/Sync for Entry

Aceeri · 2017-08-11T21:38:22Z

@torkleyy Rebased so it should be fine now.

torkleyy · 2017-08-11T21:51:29Z

Nice, thank you!

bors r+

203: CheckStorage soundness fixes r=torkleyy Splits `CheckStorage` into `RestrictedStorage` and `CheckStorage` for different usecases. `CheckStorage` will just not borrow the storage, however it does not protect against the original storage having its bitset modified, so no component or entry is given for unchecked access. `RestrictedStorage` returns an entry and a reference back to the restricted storage every iteration. This allows restricted access to the storage to get and possibly modify components without invalidating the bitset.

bors · 2017-08-11T21:57:06Z

Build succeeded

continuous-integration/travis-ci/push

204: Update dependency versions. r=azriel91 a=azriel91 Closes amethyst#203. Co-authored-by: Azriel Hoh <[email protected]>

WaDelma reviewed Jun 20, 2017

View reviewed changes

Aceeri force-pushed the check branch 4 times, most recently from 0a998cc to 77ee78c Compare June 25, 2017 00:45

Aceeri changed the title ~~[WIP] CheckStorage soundness fixes~~ CheckStorage soundness fixes Jun 25, 2017

Aceeri force-pushed the check branch 2 times, most recently from 7ff5e07 to ba4fef2 Compare July 1, 2017 01:55

torkleyy reviewed Jul 1, 2017

View reviewed changes

Aceeri force-pushed the check branch from 1cd84f0 to 96770db Compare July 1, 2017 03:50

torkleyy added the in progress label Jul 1, 2017

torkleyy self-requested a review July 1, 2017 03:55

torkleyy mentioned this pull request Jul 1, 2017

Missing Send / Sync and ParJoin for CheckStorage #191

Closed

torkleyy reviewed Jul 1, 2017

View reviewed changes

Aceeri force-pushed the check branch 2 times, most recently from ee356c5 to b5e1e1e Compare July 2, 2017 13:13

torkleyy added ready and removed in progress labels Jul 2, 2017

torkleyy requested changes Jul 2, 2017

View reviewed changes

Aceeri force-pushed the check branch 4 times, most recently from 407e0e2 to 16f44f2 Compare July 2, 2017 15:42

torkleyy reviewed Jul 6, 2017

View reviewed changes

Aceeri force-pushed the check branch 3 times, most recently from 24e327d to 695a71c Compare July 6, 2017 15:16

torkleyy reviewed Jul 7, 2017

View reviewed changes

Aceeri force-pushed the check branch 2 times, most recently from 0adda8d to 602ab4b Compare July 8, 2017 01:53

torkleyy requested a review from a team July 21, 2017 11:56

kvark reviewed Jul 23, 2017

View reviewed changes

Aceeri force-pushed the check branch from d6ec0e3 to b203d46 Compare July 25, 2017 00:16

kvark requested changes Jul 25, 2017

View reviewed changes

kvark requested changes Jul 26, 2017

View reviewed changes

Aceeri force-pushed the check branch from b97a4c6 to e259662 Compare July 26, 2017 03:52

kvark reviewed Jul 26, 2017

View reviewed changes

Aceeri force-pushed the check branch from e259662 to f108e49 Compare July 26, 2017 23:01

torkleyy reviewed Jul 27, 2017

View reviewed changes

torkleyy self-assigned this Aug 9, 2017

Aceeri added 8 commits August 9, 2017 20:01

Separate RestrictedStorage and CheckStorage for different purposes.

9296b60

Mutable restricted storage

36b46e1

Split storages into separate files

c0a63ba

Better documentation for restrict

c8f49d2

Remove immutable parallel restrict Impl Send/Sync for Entry

Bump version to 0.10

a27f2a1

Remove immutable restricted storage

d04954d

Documentation fixes, ParJoin implementation instead of Distinct

d1d4a8d

Example usage for RestrictedStorage

a7bae7c

Aceeri force-pushed the check branch from f4304c3 to a7bae7c Compare August 10, 2017 03:24

bors bot merged commit a7bae7c into amethyst:master Aug 11, 2017

WaDelma mentioned this pull request Aug 19, 2017

CheckedStorage cannot be used with par_iter #190

Closed

xMAC94x pushed a commit to xMAC94x/specs that referenced this pull request Mar 10, 2021

Merge amethyst#204

09e47bc

204: Update dependency versions. r=azriel91 a=azriel91 Closes amethyst#203. Co-authored-by: Azriel Hoh <[email protected]>

		}


		unsafe impl<'rf, 'st: 'rf, B, T, R, ParallelRestriction> ParJoin for &'rf RestrictedStorage<'rf, 'st, B, T, R, ParallelRestriction>

CheckStorage soundness fixes #203

CheckStorage soundness fixes #203

Conversation

Aceeri commented Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Aceeri Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

WaDelma Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkleyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkleyy commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aceeri Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

kvark commented Jul 24, 2017

Aceeri commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aceeri Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkleyy commented Aug 9, 2017

Aceeri commented Aug 11, 2017

torkleyy commented Aug 11, 2017

bors bot commented Aug 11, 2017

Build succeeded

Aceeri commented Jun 20, 2017 •

edited

Loading

Aceeri Jun 21, 2017 •

edited

Loading

WaDelma Jun 21, 2017 •

edited

Loading

Aceeri Jul 25, 2017 •

edited

Loading

Aceeri Jul 26, 2017 •

edited

Loading