Help reviewing short AtomicArc?

code review
#1

I'm hoping someone can check if the following code is sound?

The basic idea is to allow atomic swaps of an Arc. There's no need for a guard, since when we call load() we get an Arc itself.

use std::{intrinsics::transmute, sync::Arc};

pub struct AtomicArc<T> {
    pointer: std::sync::atomic::AtomicPtr<T>,
}

impl<T> AtomicArc<T> {
    pub fn load(&self) -> Arc<T> {
        let p = self.pointer.load(std::sync::atomic::Ordering::Relaxed);
        let arc: Arc<T> = unsafe { transmute(p) };
        let result = arc.clone();
        std::mem::forget(arc);
        result
    }
    pub fn swap(&self, other: Arc<T>) -> Arc<T> {
        let p = self.pointer.swap(
            unsafe { transmute(other) },
            std::sync::atomic::Ordering::Relaxed,
        );
        unsafe { transmute(p) }
    }
}

impl<T> From<Arc<T>> for AtomicArc<T> {
    fn from(arc: Arc<T>) -> AtomicArc<T> {
        AtomicArc {
            pointer: unsafe { transmute(arc) },
        }
    }
}

impl<T> Drop for AtomicArc<T> {
    fn drop(&mut self) {
        let _arc: Arc<T> =
            unsafe { transmute(self.pointer.load(std::sync::atomic::Ordering::Relaxed)) };
    }
}

I'd also appreciate any suggestions for better alternatives to the transmutes. That's a scary function.

#2

  1. Don't use transmute to convert Arc from/to pointers. Use from_raw/into_raw instead.

  2. Here's the use-after-free case.

thread 1: calls load()
thread 1: loads address A
thread 2: calls swap()
thread 2: address A is replaced with address B
thread 2: drops Arc from address A, which happens to be the last reference so it deallocate the Arc.
thread 1: calls Arc::clone() on Arc from address A
thread 1: trying to update reference counter on deallocated heap memory.
  1. Why don't you use the popular arc-swap crate?
5 Likes
#3

It's very much more complicated, both in terms of API and implementation.

Because I didn't read the correctly and thought they didn't do what I needed. Thanks!

Ah yes, I see the problem. I'll think about whether a solution exists... It's actually seeming doubtful and if you see one I'd love to hear it.

#4

The arc-swap crate has a more complicated implementation because it has to avoid the use-after-free that @Hyeonu mentioned.

4 Likes
#5

It was really the twisty maze of generics that I don't like about the implementation of arc-swap. It is frustrating tha the implementationt an ArcSwap is split among four different data structures divided between three modules. I can't hold all that in my head in order to understand what it's doing, andi haven't even yet located the actual code that avoids the data race.

I expect with ten minutes of effort I could manage to read and comprehend the code, but I haven't had that much uninterrupted time since starting to look into this.