Wrong atomic ordering in Drop impl for RwLockWriteGuard and buggy lock acquisition failure in RwLock::try_read

[64]

Multiple issues in the RwLock implementation.

The first (and the worst) one is here:

impl<'rwlock, T: ?Sized> Drop for RwLockWriteGuard<'rwlock, T> {
    fn drop(&mut self) {
        self.lock.store(0, Ordering::Relaxed);
    }
}

Use of Ordering::Relaxed is incorrect. The compiler is free to reorder a write behind this, which could lead to two mutable refs being used at the same time (UB). It should be Ordering::Release instead (maybe this was just a typo?). Other parts of this code use the needlessly strict SeqCst ordering, but that's not a soundness issue.

The second that I've found is a bug in the try_read function:

    pub fn try_read(&self) -> Option<RwLockReadGuard<T>>
    {
        let old = (!USIZE_MSB) & self.lock.load(Ordering::Relaxed);
		// readers variable could be incremented here
        let new = old + 1;
        if self.lock.compare_and_swap(old,
                                      new,
                                      Ordering::SeqCst) == old
        {
            Some(RwLockReadGuard {
                lock: &self.lock,
                data: unsafe { & *self.data.get() },
            })
        } else {
            None
        }
    }

The problem is that a different thread could increment the readers count on the indicated line, making the value that the CAS checks for incorrect - it might fail even if there are no writers at all. For example:

    let lock = Arc::new(spin::RwLock::new(0));

    {
        let lock = lock.clone();
        thread::spawn(move || {
            loop {
                lock.try_read().unwrap();
            }
        });
    }

    thread::spawn(move || {
        loop {
            // Increment and decrement readers count in a loop
            lock.read();
        }
    }).join();

This code panics for me, even though it there are no writers so try_read should always succeed.

To be frank, I think it would be better to simply rewrite this whole implementation. The original blog that this was implemented from uses the writers bit to allow for recursive write locking (which this crate doesn't) - I see no reason not to just use some sentinel value like -1 to indicate a writer is holding the lock.

[Ericson2314]

Your first bug makes sense; blame that was my error years ago. Thanks for catching. I am confused by your second bug; if readers is incremented than the compare won't match and just another loop will happen: seems sub-optimal but not unsound?

Feel free to take a stab at a rewrite.

[64]

The problem is that there is an implicit expectation that try_read will succeed if there are no writers. Or at least, you need to make it abundantly clear in the docs that this is not the case - i.e that you can't rely on try_read() succeeding even if there are no writers.

Working on a fix - would you be okay with an implementation which includes upgradeable locks? I'll try and do a benchmark and show that performance doesn't suffer as a result.

[64]

forced_write_unlock also incorrectly uses Ordering::Relaxed.

[Ericson2314]

Oh right! There is no loop. Yeah I think I just took the code from the original to make the try version, not realizing about that difference in expectations.

Upgradable is fine. I think std might even do that now?