1

Multiple questions were already asked regarding this topic:

The answers are more or less: It's not possible (without unsafe).

I tried the unsafe variant myself and want to ask if this way is safe.

The idea is that I wrap the guard in a struct that implements Iterator. Besides the guard, an iterator is stored which will be created from the stored guard:

struct MapIter<'a> {
    guard: RwLockReadGuard<'a, HashMap<i32, i32>>,
    iter:  Iter<'a, i32, i32>,
}

It's created with these lines:

impl<'a> MapIter<'a> {
    fn new(map: &'a RwLock<HashMap<i32, i32>>) -> Box<Self> {
        // create a `box Self`
        // the iterator remains uninitialized.
        let mut boxed = Box::new(Self {
            guard: map.read().expect("ToDo"),
            iter:  unsafe { mem::uninitialized() },
        });

        // create the iterator from `box Self`.
        boxed.iter = unsafe { 
            (*(&boxed.guard as *const RwLockReadGuard<'a, HashMap<i32, i32>>)).iter() 
        };
        boxed
    }
}

Now it can implement Iterator:

impl<'a> Iterator for MapIter<'a> {
    type Item = (&'a i32, &'a i32);

    fn next(&mut self) -> Option<Self::Item> {
        self.iter.next()
    }
}

Is this code safe?

See this code in action at the playground.

Additionally I get a trivial cast warning

warning: trivial cast: warning: trivial cast: `&std::sync::RwLockReadGuard<'_, std::collections::HashMap<i32, i32>>` as `*const std::sync::RwLockReadGuard<'a, std::collections::HashMap<i32, i32>>`. Cast can be replaced by coercion, this might require type ascription or a temporary variable
   |
   | unsafe { (*(&boxed.guard as *const RwLockReadGuard<'a, HashMap<i32, i32>>)).iter() };
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |

How to get around this?

Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
Tim Diekmann
  • 7,755
  • 11
  • 41
  • 69

1 Answers1

4

No, it's not safe. I can use Container to create a dangling reference in safe code:

let container = Container::new();       // create a container
let r = {
    let mut it = container.iter();
    it.next()                           // obtain a reference to part of it
};
container.map.write().unwrap().clear(); // empty the container
println!("{:?}", r);                    // oh dear.

In the playground this compiles, which isn't good, because r contains references to data that are invalidated when the HashMap is cleared.

Vladimir Matveev's answer to a similar question explains in more detail why this is unsound, and contains the following concise summary:

You cannot do this because it would allow you to circumvent runtime checks for uniqueness violations.

trent
  • 25,033
  • 7
  • 51
  • 90
  • Assuming, you only return values, i.e. clone the `Item`, would this be safe? – Tim Diekmann Aug 15 '18 at 19:59
  • @Tim If `next` returned `Option<(i32, i32)>`? I think that would be safe, but I'm not totally convinced in my own mind yet. – trent Aug 15 '18 at 20:27
  • Yup, that is actually my use case but I dropped it in order to make a [MCVE]. Aktually, I have a `(&u64, &DirEntry)` which will be converted to `(u64, PathBuf)` – Tim Diekmann Aug 15 '18 at 20:28
  • 1
    @Tim Here's what I've found so far: It seems like this should be safe by analogy to [Ref::map](https://doc.rust-lang.org/nightly/std/cell/struct.Ref.html#method.map); however, there is a subtle interaction with concurrency that makes the equivalent of `map` for `RwLockReadGuard` [unsound, or at least potentially unsound with future changes to the stdlib on Windows](https://github.com/rust-lang/rust/issues/44189). I don't understand the issue well enough to say for sure whether your proposed `MapIter` would also be unsound. – trent Aug 16 '18 at 12:11
  • However, it seems very improbable that you could trigger this soundness bug by accident. So if it's very unlikely to be triggered and you're not even sure it's there, you might find the risk acceptable. I believe the API can be implemented safely, so even if the implementation is wrong, it can be fixed in the future. At least guarantee that `iter` is dropped before `guard`, to avoid problems from that angle. – trent Aug 16 '18 at 12:16
  • I already stumbled across the issue too. I worked around the `MapIter`, it's a bit slower though, but it's cold code anyway. – Tim Diekmann Aug 16 '18 at 12:16