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.

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

ptr::read and ptr::write (and others?) should specify alignment requirements in their contract #36450

Closed
whitequark opened this issue Sep 13, 2016 · 17 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-low Low priority

Comments

@whitequark
Copy link
Member

whitequark commented Sep 13, 2016

Currently, these functions do not say anything about whether the raw pointer has to be aligned. They are also currently implemented with copy_nonoverlapping, which means they are safe to use with unaligned data. However, @ubsan tells me that this is an implementation detail that cannot be relied on.

In any case, these functions should explicitly specify their alignment requirements. Similarly, ptr::copy{,_nonoverlapping} also should do that, since right now they do not explicitly state that in their contract either. (They do reference C's memcpy and memmove, but I don't think that a C reference is a good way to define normative semantics.)

@strega-nil
Copy link
Contributor

ping @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Sep 13, 2016

See rust-lang/rfcs#1725

@Amanieu
Copy link
Member

Amanieu commented Sep 13, 2016

Also keep in mind that copy(_nonoverlapping) does have alignment requirements: it's based on the type of the pointer. So if you pass a *mut u32 to copy_nonoverlapping then LLVM will assume that the pointer is aligned to 4 bytes.

@whitequark
Copy link
Member Author

That then should be explicitly specified also, because it is definitely not a part of the C memcpy semantics.

@bluss
Copy link
Member

bluss commented Sep 14, 2016

Maybe it's obvious in this discussion, but *ptr (copy to value from raw pointer) requires ptr to be aligned, of course. With that as the baseline, I think it would have to be assumed that read/write use that rule if nothing else is documented -- but why not document it explicitly.

@strega-nil
Copy link
Contributor

strega-nil commented Sep 14, 2016

@bluss That's also never documented, to be clear, except in the not-right-places in the nomicon.

@tbu-
Copy link
Contributor

tbu- commented Mar 7, 2017

Seems to be fixed, the documentation says:

The pointer must be aligned; use write_unaligned if that is not the case.

and similarly for ptr::read.

@strega-nil
Copy link
Contributor

strega-nil commented Mar 8, 2017

Recommend closing as fixed.

@arielb1 arielb1 closed this as completed Mar 8, 2017
@whitequark
Copy link
Member Author

copy{,_nonoverlapping} don't have mention of alignment in the documentation still.

@arielb1
Copy link
Contributor

arielb1 commented Mar 8, 2017

@whitequark

That's because it has none.

@whitequark
Copy link
Member Author

Also keep in mind that copy(_nonoverlapping) does have alignment requirements: it's based on the type of the pointer. So if you pass a *mut u32 to copy_nonoverlapping then LLVM will assume that the pointer is aligned to 4 bytes.

@Amanieu above disagrees...

@tbu-
Copy link
Contributor

tbu- commented Mar 8, 2017

The linked RFC (by @Amanieu) states that it uses copy_nonoverlapping for {read,write}_unaligned.

@arielb1
Copy link
Contributor

arielb1 commented Mar 8, 2017

@whitequark

I had the mistaken impression that copy_nonoverlapping took a u8 parameter.

@arielb1 arielb1 reopened this Mar 8, 2017
@Mark-Simulacrum
Copy link
Member

Okay, so this appears to be a documentation issue -- we need to update ptr::copy{,_nonoverlapping} to state something about alignment.

@Mark-Simulacrum Mark-Simulacrum added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label May 27, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@steveklabnik steveklabnik added the P-low Low priority label Aug 30, 2017
@QuietMisdreavus
Copy link
Member

The docs team discussed this today. Here's the current state of the ptr module, with respect to alignment:

<ubsan> because anything in the standard library which takes a raw pointer assumes validity of the
        raw pointer, unless it's explicitly denoted `_unaligned`, in which case size must be valid,
        but alignment might not be
(...)
<ubsan> copy{_nonoverlapping}, read_volatile, probably replace, probably swap (uses "valid",
        probably could be more wordy), write_volatile, swap_nonoverlapping
<ubsan> we should also standardize this documentation across all of the functions
(...)
<ubsan> (some put their requirements under "Safety", some put it under "Undefined Behavior", etc.)

Basically, to really close this issue it would be prudent to go over every function in ptr and leave a note about the assumed alignment requirements of it.

Since #29371 is still open, i'm considering this issue part of that one.

@QuietMisdreavus QuietMisdreavus mentioned this issue Sep 13, 2017
11 tasks
@steveklabnik steveklabnik added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label May 28, 2018
@ecstatic-morse
Copy link
Contributor

@steveklabnik. This was completed in #53783. All functions in std::ptr now state their alignment requirements in the "Safety" section.

@steveklabnik
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-low Low priority
Projects
None yet
Development

No branches or pull requests

10 participants