Arc::from(Box<T>) allocates and can panic in FallibleArc::try_new()

[brandenburg]

I was trying to understand how FallibleArc::try_new is implemented and now wonder whether it works at all.

My expectation is that FallibleArc::try_new() should return Err(TryReserveError) if the allocator runs out of memory at any point while executing FallibleArc::try_new(). However, I think it can still panic. Here's the relevant code:

    fn try_new(t: T) -> Result<Self, TryReserveError> {
        // doesn't work as the inner variable of arc are also stocked in the box
        let b = Box::try_new(t)?;
        Ok(Arc::from(b))
    }

(Aside: I do not understand what the comment "doesn't work as the inner variable of arc are also stocked in the box" is supposed to convey — my apologies if this duplicates a known issue.)

Suppose the Box::try_new() succeeds, but it exhausts the memory allocator (i.e., any future allocation attempt will fail). Then Arc::from(b) calls Arc::from_box():

fn from_box(v: Box<T>) -> Arc<T> {
        unsafe {
            let box_unique = Box::into_unique(v);
            let bptr = box_unique.as_ptr();

            let value_size = size_of_val(&*bptr);
            let ptr = Self::allocate_for_ptr(bptr);

            // Copy value as bytes
            ptr::copy_nonoverlapping(
                bptr as *const T as *const u8,
                &mut (*ptr).data as *mut _ as *mut u8,
                value_size,
            );

            // Free the allocation without dropping its contents
            box_free(box_unique);

            Self::from_ptr(ptr)
        }

Arc::from_box() calls Arc::allocate_for_ptr(), which is just a wrapper around Arc::allocate_for_layout().

    /// Allocates an `ArcInner<T>` with sufficient space for an unsized inner value.
    unsafe fn allocate_for_ptr(ptr: *const T) -> *mut ArcInner<T> {
        // Allocate for the `ArcInner<T>` using the given value.
        unsafe {
            Self::allocate_for_layout(
                Layout::for_value(&*ptr),
                |layout| Global.alloc(layout),
                |mem| set_data_ptr(ptr as *mut T, mem) as *mut ArcInner<T>,
            )
        }
    }

Let's take a look at Arc::allocate_for_layout():

    unsafe fn allocate_for_layout(
        value_layout: Layout,
        allocate: impl FnOnce(Layout) -> Result<NonNull<[u8]>, AllocError>,
        mem_to_arcinner: impl FnOnce(*mut u8) -> *mut ArcInner<T>,
    ) -> *mut ArcInner<T> {
        // Calculate layout using the given value layout.
        // Previously, layout was calculated on the expression
        // `&*(ptr as *const ArcInner<T>)`, but this created a misaligned
        // reference (see #54908).
        let layout = Layout::new::<ArcInner<()>>().extend(value_layout).unwrap().0.pad_to_align();

        let ptr = allocate(layout).unwrap_or_else(|_| handle_alloc_error(layout));

        // Initialize the ArcInner
        let inner = mem_to_arcinner(ptr.as_non_null_ptr().as_ptr());
        debug_assert_eq!(unsafe { Layout::for_value(&*inner) }, layout);

        unsafe {
            ptr::write(&mut (*inner).strong, atomic::AtomicUsize::new(1));
            ptr::write(&mut (*inner).weak, atomic::AtomicUsize::new(1));
        }

        inner
    }

Note the line allocate(layout).unwrap_or_else(|_| handle_alloc_error(layout)) — if the allocator runs out of memory at this point, the process will panic "as usual", defeating the purpose of using try_new in the first place.

Am I missing something? If not, this corner case should probably mentioned in the documentation.

Is there a good way to ensure that Arc::try_new will truly never panic? Since ArcInner is not public, I don't see a good way to construct an allocation that would be suitable for use with Arc::from_raw().

[vcombey]

You're totally right, it is really old code I forgot to remove when I published the crate. Apologies..
I have no idea neither, appart from using really unsafe code to create the pointer to give to Arc::from_raw. For the moment we should remove this method.

[brandenburg]

Thanks for the quick confirmation! That's unfortunate, though; I was hoping to use the panic-free Arc. The only option (other than evil, brittle pointer hacks) I see now is to simply re-implement Arc from scratch (or copy&paste&adjust code from alloc crate), which is a bit unfortunate.

[gz]

This can probably be closed since try_new exists on Arc nowadays (https://doc.rust-lang.org/std/sync/struct.Arc.html#method.try_new)

[kornelski]

try_new_uninit_slice is missing, so fallible Arc of slices (Arc<[_]>) is still impossible to do (https://github.com/rust-lang/rust/issues/63291)

[00xc]

Hi, doesn't this apply to Rc as well? The code path looks very similar when doing Rc::from(Box<T>) in this crate's Rc::try_new.

[kornelski]

Yes, same for Rc