Box::leak will not call allocator destruction function

[yuhao-su]

I tried this code:

#![feature(btreemap_alloc)]
#![feature(allocator_api)]

use std::collections::BTreeMap;
use std::sync::Arc;
use std::alloc::Allocator;
use std::ptr::{self, NonNull};

#[derive(Clone)]
pub struct AllocWrapper<T>(pub Arc<T>);

unsafe impl<T: Allocator> Allocator for AllocWrapper<T> {
    fn allocate(
        &self,
        layout: std::alloc::Layout,
    ) -> Result<std::ptr::NonNull<[u8]>, std::alloc::AllocError> {
        self.0.allocate(layout)
    }

    unsafe fn deallocate(&self, ptr: std::ptr::NonNull<u8>, layout: std::alloc::Layout) {
        self.0.deallocate(ptr, layout)
    }

    fn allocate_zeroed(
        &self,
        layout: std::alloc::Layout,
    ) -> Result<std::ptr::NonNull<[u8]>, std::alloc::AllocError> {
        self.0.allocate_zeroed(layout)
    }

    unsafe fn grow(
        &self,
        ptr: std::ptr::NonNull<u8>,
        old_layout: std::alloc::Layout,
        new_layout: std::alloc::Layout,
    ) -> Result<std::ptr::NonNull<[u8]>, std::alloc::AllocError> {
        self.0.grow(ptr, old_layout, new_layout)
    }

    unsafe fn grow_zeroed(
        &self,
        ptr: std::ptr::NonNull<u8>,
        old_layout: std::alloc::Layout,
        new_layout: std::alloc::Layout,
    ) -> Result<std::ptr::NonNull<[u8]>, std::alloc::AllocError> {
        self.0.grow_zeroed(ptr, old_layout, new_layout)
    }

    unsafe fn shrink(
        &self,
        ptr: std::ptr::NonNull<u8>,
        old_layout: std::alloc::Layout,
        new_layout: std::alloc::Layout,
    ) -> Result<std::ptr::NonNull<[u8]>, std::alloc::AllocError> {
        self.0.shrink(ptr, old_layout, new_layout)
    }
}

fn main() {
    let allocator = AllocWrapper(Arc::new(std::alloc::System));
    println!("count = {}", Arc::strong_count(&allocator.0));
    let x = Box::new_in(5, allocator.clone());
    println!("count = {}", Arc::strong_count(&allocator.0));
    let _ = NonNull::from(Box::leak(x));
    println!("count = {}", Arc::strong_count(&allocator.0));
}

I expected to see this happen:

• When the x get dropped, the ref count decreased to 1

Instead, this happened:

• When the x get dropped, the ref count dose not change

The current implementation of Box::leak will ManuallyDrop the whole Box but only leak the data pointer. The destruction function of the allocator will not be called. I'm not sure if it the expected behavior.

    pub const fn leak<'a>(b: Self) -> &'a mut T
    where
        A: 'a,
    {
        unsafe { &mut *mem::ManuallyDrop::new(b).0.as_ptr() }
    }

Meta

rustc --version --verbose:

rustc 1.68.0-nightly (bdb07a8ec 2022-12-11)
binary: rustc
commit-hash: bdb07a8ec8e77aa10fb84fae1d4ff71c21180bb4
commit-date: 2022-12-11
host: aarch64-apple-darwin
release: 1.68.0-nightly
LLVM version: 15.0.6

Backtrace

<backtrace>

related to : #106203

[jschwe]

When the x get dropped, the ref count dose not change

x is never dropped though - it is leaked.

The current implementation of Box::leak will ManuallyDrop the whole Box but only leak the data pointer.

The Box is wrapped in ManuallyDrop. This does not mean that the value is dropped! Instead, it means that the value will not be dropped automatically by the compiler. The destructor not running seems like the intended behaviour to me.

[compiler-errors]

Hm, seems a bit inconsistent if we compare to the into_raw implementation, which does drop the allocator: https://doc.rust-lang.org/src/alloc/boxed.rs.html#1057

[fuyufjh]

The current implementation of Box::leak will ManuallyDrop the whole Box but only leak the data pointer.

The Box is wrapped in ManuallyDrop. This does not mean that the value is dropped! Instead, it means that the value will not be dropped automatically by the compiler. The destructor not running seems like the intended behaviour to me.

pub struct Box<
    T: ?Sized,
    #[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global,
>(Unique<T>, A);

https://doc.rust-lang.org/src/alloc/boxed.rs.html#197-200

Yeah, obviously the destructor of value (T) should not be called, but how about the memory allocator (A)? Especially in cases of a customized allocator wrapped in Arc.

[yuhao-su]

This does not mean that the value is dropped! Instead, it means that the value will not be dropped automatically by the compiler.

Sorry for using the wrong phrase drop. What I want to say is leak ask the compiler not to automatically drop the Boxed value and the allocator, but it only returns the pointer to the value while the allocator is ignored. Maybe we need something like leak_with_allocator?

[thomcc]

I think this failing to drop the allocator is pretty surprising and is inconsistent with most uses of Box::leak.

Really, it's hard for me to see a case where this is the desired behavior...

[BugenZhao]

Hm, seems a bit inconsistent if we compare to the into_raw implementation, which does drop the allocator: https://doc.rust-lang.org/src/alloc/boxed.rs.html#1057

While Vec::into_raw_parts does not drop the allocator as well. 🤯 I guess we may need to clarify the semantics of these into_raws when the allocator API is supported.

https://doc.rust-lang.org/stable/src/alloc/vec/mod.rs.html#818

[jschwe]

I think this failing to drop the allocator is pretty surprising and is inconsistent with most uses of Box::leak.
Really, it's hard for me to see a case where this is the desired behavior...

In case of a multi-tiered allocator approach, where the last tier is something super simple, e.g. a bump allocator, I could see a use case where a child allocator returns the memory owned by itself to a parent allocator when its last instance is dropped. This obviously would be unsound if allocator instances can be dropped when leaking memory.

The Safety comment of the allocator trait says (emphasis mine):

Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped,

The safety comment could of course be adjusted to put the onus on the implementor to check for any leaks when the alst instance is dropped, but that prevents very simple allocator designs, since some book-keeping is needed.

[thomcc]

So, I think I was completely mistaken here. Or rather, I think it is surprising behavior, and I think it will usually not be desired, but it's also probably the correct behavior if we're going to offer this function for Box<T, A>.

My guess is that there are going to be plenty of As which are Global-like (that is, ZSTs with no drop) where this causes no problems, that Box::leak is worth having, but we should probably call out this behavior in the documentation with some sort of caveat.

[Amanieu]

The correct behavior is actually not to leak the allocator, otherwise leak is unsound. This is because the Allocator trait only guarantees that allocations remain valid until the allocator is dropped.

#141219 moves into_raw to only be usable with the global allocator, requiring the use of into_raw_with_alloc when using custom allocators. This avoids any confusing by requiring users of into_raw to always explicitly handle the allocator.

Since this is behaving as intended, I'm going to close this issue.