Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify the lifetimes of allocations returned by the Allocator trait #118890

Merged
merged 2 commits into from
Feb 14, 2024
Merged

Clarify the lifetimes of allocations returned by the Allocator trait #118890

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb5ed29
Clarify the lifetimes of allocations returned by the `Allocator` trait
Amanieu Dec 12, 2023
8e9c8dd
Add information about allocation lifetime to Allocator::allocate
Amanieu Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions library/core/src/alloc/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,10 @@ impl fmt::Display for AllocError {
/// # Safety
///
/// * Memory blocks returned from an allocator that are [*currently allocated*] must point to
/// valid memory and retain their validity while they are [*currently allocated*] and at
/// least one of the instance and all of its clones has not been dropped.
/// valid memory and retain their validity while they are [*currently allocated*] and the shorter
/// of:
/// - the borrow-checker lifetime of the allocator type itself.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reference to a borrow-checker lifetime feels pretty surprising to me. Why do we care about that lifetime specifically? The compile-time lifetimes typically don't matter for runtime safety (i.e., you can freely transmute all lifetimes between each other if you know that the code is correct, or skip borrow checking entirely if you have some other proof that the program is fine), so this is confusing to me.

It's not clear to me why the condition here cannot be "[...] and the instance the block was allocated from or a clone'd version of that instance is not dropped" -- that seems like it should be fine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the case of an allocator that allocates from a buffer on the stack (StackAlloc<'a>). Without the new wording, calling mem::forget on such an allocator would require any allocated memory to continue being valid indefinitely since the StackAlloc has not been dropped.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Feb 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a separate section for Safety for users of the Allocator API, and perhaps that means that allocate needs to be unsafe (since the returned memory is what we're bounding usage of). The implementor of the Allocator trait can't provide guarantees about downstream usage of the type (or at least, no one can legitimately say they've implemented the trait without carefully inspecting all downstream uses, and then we might as well just not make it a trait).

In particular, even with this clause StackAlloc<'a> would need to be unsafe to construct and/or never exposed outside a crate, since otherwise a completely safe crate would easily be able to have a use-after-free by using StackAlloc<'a> as you've described it.

It seems to me that a stack-based allocator needs to have Allocator for Pin<&mut StackAlloc<'a>> or so, to guarantee Drop is called. Presumably Drop would need to block the thread (e.g., loop {}) if not all memory blocks have actually been de-allocated. I don't see any other way with the current interface for Allocator for StackAlloc to be relatively safe to use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine for allocate to be a safe function since it returns a raw pointer: the user is responsible for checking pre-conditions before dereferencing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sure. But I think the clause added here should still go on that function or in some other place, it's not a precondition on the safety of the implementation of Allocator itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've copied the text into the documentation of the allocate function. The lifetime of the allocation is part of the trait contract, so both the user and implementer need to be aware of it.

/// - as long as at least one of the instance and all of its clones has not been dropped.
///
/// * copying, cloning, or moving the allocator must not invalidate memory blocks returned from this
/// allocator. A copied or cloned allocator must behave like the same allocator, and
Expand All @@ -114,6 +116,10 @@ pub unsafe trait Allocator {
/// The returned block may have a larger size than specified by `layout.size()`, and may or may
/// not have its contents initialized.
///
/// The returned block of memory remains valid as long as it is [*currently allocated*] and the shorter of:
/// - the borrow-checker lifetime of the allocator type itself.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should phrase or give a heuristic that this is equivalent to treating the allocation as borrowing from the &self argument - i.e., the returned NonNull is really closer to a &'a mut [MaybeUninit<u8>], in some sense.

But this seems OK for now.

/// - as long as at the allocator and all its clones has not been dropped.
///
/// # Errors
///
/// Returning `Err` indicates that either memory is exhausted or `layout` does not meet
Expand Down
Loading