Is this cyclic Arc constructor sound?

I am currently missing the ability to construct an Arc in a potentially cyclic fashion. Arc::new_cyclic does not fulfill my needs as I am using futures + Results.

I found that there was a proposal to add this to the standard library, but that did not realize yet, and I need it now in stable rust.

The code:

I understand that I am relying on some internals of Arc. However, none of these feel unsound to rely on, as they are the same as those used by new_cyclic and wouldn't make sense to be changed to me.

Notably:

  • Decrementing a strong count while a Weak<T> exists does not drop the backing storage
  • Incrementing a strong count after initializing T with a strong count of 0 is sound
  • Using a pointer returned from Arc::<MaybeUninit<T>>::into_raw in Arc::<T>::from_raw is sound

I am fairly new to writing unsafe rust, so I would love comments beyond the soundness of this piece of code!

The code is problematic on several counts. I don't know whether these can actually cause unsoundness.

  • mem::forget() in general is a red flag. You should probably be using ManuallyDrop instead. I don't understand exactly why the forget is needed here, but if you rely on it for soundness, then you've got a problem: panic safety. If the code panics before the forget, then it will unwind and forget won't get called at all. The better alternative is to wrap the thing to be forgotten in a drop guard containing a ManuallyDrop, and clean it up when/if needed, ensuring that a panic only leads to a leak but not to UB.
  • You seem to be asserting all sorts of conditions related to the reference count, but not that you have 0 weaks at the end of the build method. That seems like a useful addition.
  • SmartPointer::from_raw() should be paired with SmartPointer::into_raw(), not with as_ptr(). The former indicates giving up ownership, the latter doesn't, and from_raw() on std smart pointer types definitely does assume ownership. You might have a double-free bug here.
  • The type of the stored Weak should probably be MaybeUninit<T> instead pf T. It doesn't point to a valid T, and so it's too easy to upgrade and dereference it, causing UB (which is most likely to materialize as the dropping of an uninitialized value if you assign through the reference, but technically, creating the reference in the first place would already be UB if you were to accidentally do that).
  • The endless #[allow(unsafe_code)] annotations are a code smell. You don't gain anything by annotating every unsafe use site twice. If you have a crate-level #![deny(unsafe_code)], and you want to allow unsafe in this single module, then put a a module-level #![allow(unsafe_code)] at the top of the file. The individual annotations are completely redundant, because the very purpose of unsafe {} blocks is to mark unsafe code.
4 Likes

There recently was this thread. IIRC, the conclusion was that it’s not really possible to do such a things soundly on stable Rust. Though maybe your API is actually different from that case. I’ll have a look shortly.

To recap the “main problem”: The documentation of increment_strong_count reads

Safety

The pointer must have been obtained through Arc::into_raw, and the associated Arc instance must be valid (i.e. the strong count must be at least 1) for the duration of this method.

which might at the moment only directly lead to UB in Rc, but it’s clearly at least library-UB.

I also raised concerns in this thread about the Relaxed ordering inherent to this implementation being insufficient. I’m curios if I’ll be able to build some concrete miri-detectable UB through that issue, but either way the issue above should be sufficient already to say conclude “can’t do it soundly this way”.

3 Likes

It is possible for miri to detect a data race (so that’s UB), e.g. with code like

fn main() {
    let mut x = CyclicArcBuilder::new();
    let w = x.weak();
    let _t = std::thread::spawn(move || x.build(Box::<str>::from("hello")));
    loop {
        if let Some(arc) = w.upgrade() {
            let _s = &*arc;
            break;
        }
    }
}
$ cargo miri run
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/small_pg`
error: Undefined Behavior: Data race detected between (1) Write on thread `<unnamed>` and (2) Read on thread `main` at alloc916+0x10. (2) just happened here
   --> ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:395:18
    |
395 |         unsafe { &*self.as_ptr().cast_const() }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) Write on thread `<unnamed>` and (2) Read on thread `main` at alloc916+0x10. (2) just happened here
    |
help: and (1) occurred earlier here
   --> src/main.rs:47:13
    |
47  |             std::ptr::write(loc, data);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE (of the first span):
    = note: inside `std::ptr::NonNull::<alloc::sync::ArcInner<std::boxed::Box<str>>>::as_ref::<'_>` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:395:18: 395:46
    = note: inside `std::sync::Arc::<std::boxed::Box<str>>::inner` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:1741:18: 1741:35
    = note: inside `<std::sync::Arc<std::boxed::Box<str>> as std::ops::Deref>::deref` at ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:2051:10: 2051:22
note: inside `main`
   --> src/main.rs:129:23
    |
129 |             let _s = &*arc;
    |                       ^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error
2 Likes

Well it was worth a shot. I'll have to see if I can make this work in another way...

Since in my usage, there is a Mutex in the Arc, I guess I can do some Option shenanigans and just unwrap everywhere else until something like this is supported.

Thank you a lot @H2CO3. Those are some very good pointers! I'll keep those in mind for my next foray into unsafe stuff.

You seem to be asserting all sorts of conditions related to the reference count, but not that you have 0 weaks at the end of the build method. That seems like a useful addition.

I could only assert that I have >0 weaks, since an Arc also has a a weak ref, and there could have been made any amount before build is called.

@steffahn, thanks for the link! That seems completely related and I'm happy to see that I'm not the only one hitting this snag...

Is there any way to try and move it along? Since I am not super-knowledgeable on atomics etc... I feel like I'd mostly be noise, but if I can do anything I'd love to try and help!

In this case, I'd advise you to forgo all unsafe, and initialize a valid (i.e., non-dangling) weak pointer, backed by a strong pointer pointing to a (not necessarily meaningful) default value, which you can subsequently replace.

Here's how.

3 Likes

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.