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

Jump to bottom

update the safety preconditions of from_raw_parts #129483

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+8 −8
Open

update the safety preconditions of from_raw_parts #129483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 2 additions & 3 deletions library/alloc/src/string.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,10 +913,9 @@ impl String {
/// This is highly unsafe, due to the number of invariants that aren't
/// checked:
///
/// * The memory at `buf` needs to have been previously allocated by the
/// same allocator the standard library uses, with a required alignment of exactly 1.
/// * unless `capacity` is 0, `buf` must have been allocated using the global allocator with an alignment of 1 and a capacity of `capacity`.
Copy link
Member

Choose a reason for hiding this comment

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

So do we really want to promise that any non-null pointer is okay if capacity is 0?

Also, "allocated using the global allocator with [...] a capacity" is odd -- the allocation ABI doesn't talk about "capacity", it talks about "size". So this should probably say "and a size of capacity".

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I kinda think we don't, and should specify the NonNull::dangling() pointer if we accept any.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the point about using "size" rather than "capacity"; note that this is consistent with some of the current language on Vec::from_raw_parts "capacity needs to be the capacity that the pointer was allocated with". Maybe that should be changed as well then?

Regarding the requirement on String::from_raw_parts; what about doing the same as in Vec::from_raw_parts, making the alignment requirement always present? That would allow making your own dangling pointers, but I don't see the motivation for restricting the creation of dangling pointers to a specific API here.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the requirement on String::from_raw_parts; what about doing the same as in Vec::from_raw_parts, making the alignment requirement always present? That would allow making your own dangling pointers, but I don't see the motivation for restricting the creation of dangling pointers to a specific API here.

So that we can change the value that we're expecting as the dangling pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that we can change the value that we're expecting as the dangling pointer.

NonNull::dangling does specifically specify that it should never be used as a sentinel value, as it may also be a valid pointer.

what about doing the same as in Vec::from_raw_parts, making the alignment requirement always present?

is it possible to have a pointer that isn't byte aligned? I considered saying "the pointer must have an alignment of 1", but isn't that a given? maybe it can change the behavior of allocators? but, that shouldn't matter for a dangling pointer that is never freed.

Copy link
Member

Choose a reason for hiding this comment

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

NonNull::dangling does specifically specify that it should never be used as a sentinel value, as it may also be a valid pointer.

See prior comments about the standard library being allowed to write unfair contracts and rely on its own internal implementation details in ways you cannot.

Copy link
Member

Choose a reason for hiding this comment

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

I may be being a bit silly here, but it just still doesn't seem like a good idea to bless all pointers thusly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See prior comments about the standard library being allowed to write unfair contracts and rely on its own internal implementation details in ways you cannot.

except that the behavior of allocators is not an internal implementation detail of the standard library. even if it was, NonNull::dangling can be manipulated into taking any value by changing the size of T. therefore, even with the special privilege of being the standard library, using NonNull::dangling as a sentinel value is still a logic error.

/// * `buf` must not be null.
/// * `length` needs to be less than or equal to `capacity`.
/// * `capacity` needs to be the correct value.
/// * The first `length` bytes at `buf` need to be valid UTF-8.
///
/// Violating these may cause problems like corrupting the allocator's
Expand Down
11 changes: 6 additions & 5 deletions library/alloc/src/vec/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,9 @@ impl<T> Vec<T> {
/// This is highly unsafe, due to the number of invariants that aren't
/// checked:
///
/// * `ptr` must have been allocated using the global allocator, such as via
/// * unless `capacity` is 0 or `T` has size 0, `ptr` must have been allocated using the global allocator, such as via
/// the [`alloc::alloc`] function.
/// * `ptr` must not be null.
/// * `T` needs to have the same alignment as what `ptr` was allocated with.
/// (`T` having a less strict alignment is not sufficient, the alignment really
/// needs to be equal to satisfy the [`dealloc`] requirement that memory must be
Expand All @@ -514,12 +515,12 @@ impl<T> Vec<T> {
/// alignment, [`dealloc`] must be called with the same layout `size`.)
/// * `length` needs to be less than or equal to `capacity`.
/// * The first `length` values must be properly initialized values of type `T`.
/// * `capacity` needs to be the capacity that the pointer was allocated with.
/// * `capacity` needs to be the capacity that the pointer was allocated with, or 0 in the case of a dangling pointer.
/// * The allocated size in bytes must be no larger than `isize::MAX`.
/// See the safety documentation of [`pointer::offset`].
///
/// These requirements are always upheld by any `ptr` that has been allocated
/// via `Vec<T>`. Other allocation sources are allowed if the invariants are
/// via `Vec<T>`. Note that a `Vec` of capacity 0 does not allocate. Other allocation sources are allowed if the invariants are
/// upheld.
///
/// Violating these may cause problems like corrupting the allocator's
Expand Down Expand Up @@ -724,7 +725,7 @@ impl<T, A: Allocator> Vec<T, A> {
/// This is highly unsafe, due to the number of invariants that aren't
/// checked:
///
/// * `ptr` must be [*currently allocated*] via the given allocator `alloc`.
/// * unless `capacity` is 0, `ptr` must be [*currently allocated*] via the given allocator `alloc`.
/// * `T` needs to have the same alignment as what `ptr` was allocated with.
/// (`T` having a less strict alignment is not sufficient, the alignment really
/// needs to be equal to satisfy the [`dealloc`] requirement that memory must be
Expand All @@ -739,7 +740,7 @@ impl<T, A: Allocator> Vec<T, A> {
/// See the safety documentation of [`pointer::offset`].
///
/// These requirements are always upheld by any `ptr` that has been allocated
/// via `Vec<T, A>`. Other allocation sources are allowed if the invariants are
/// via `Vec<T, A>`. Note that a `Vec` of capacity 0 does not allocate. Other allocation sources are allowed if the invariants are
/// upheld.
///
/// Violating these may cause problems like corrupting the allocator's
Expand Down
Loading