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

cast_ptr_alignment is acceptable in a narrow set of contexts #2881

Closed
Cocalus opened this issue Jun 29, 2018 · 12 comments · Fixed by #8632
Closed

cast_ptr_alignment is acceptable in a narrow set of contexts #2881

Cocalus opened this issue Jun 29, 2018 · 12 comments · Fixed by #8632
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience.

Comments

@Cocalus
Copy link

Cocalus commented Jun 29, 2018

Clippy may not be able to see when this is acceptable but if so there are several functions that can handle unaligned ptrs. If the unaligned pointer is only used as an input to those then it ideally wouldn't be flagged.

The obvious
std::ptr::read_unaligned
std::ptr::write_unaligned

Anything with memcpy or memmove semantics
std::ptr::copy
std::ptr::copy_nonoverlapping

I'm unsure about std::ptr::swap and std::ptr::swap_nonoverlapping

And in
std::arch::x86_64
std::arch::x86
storeu
loadu
lddqu

There might be some others.

@oli-obk oli-obk added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages labels Jun 29, 2018
@danburkert
Copy link

Example from something I'm working on:

error: casting from `*const u8` to a more-strictly-aligned pointer (`*const i16`)
  --> src/<>.rs:<>:<>
   |
69 |                 (data as *const i16).read_unaligned().to_le()
   |

which is obviously fine. It seems appropriate to at least mention this in the 'Known problems' section of cast_ptr_alignment.

@Valloric
Copy link

I've only ever seen cast_ptr_alignment lint being pretty useless; I have similar code to @danburkert.

error: casting from `*mut u8` to a more-strictly-aligned pointer (`*mut u16`)
   --> src/btree/raw_page.rs:160:13
    |
160 |             (key_entry_start.as_ptr() as *mut u16).read_unaligned();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#cast_ptr_alignment

and also

error: casting from `*mut u8` to a more-strictly-aligned pointer (`*mut u16`)
   --> src/btree/raw_page.rs:220:28
    |
220 |       ptr::write_unaligned(entry_start as *mut u16, key_bytes_to_copy);
    |                            ^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#cast_ptr_alignment

@strega-nil
Copy link

This also breaks in the context of allocation functions - malloc always returns a 16-byte aligned pointer, iirc, so there's no need to lint the cast of the return value.

@Cocalus
Copy link
Author

Cocalus commented Oct 8, 2018

Malloc returns pointers that aligned to size of the allocation (rounding upto the next highest power of 2) or sufficiently large enough max alignment (often 16 bytes). Most allocators have a minimum alignment as well. Rust's default allocator jemalloc has a minimum alignment of 8 bytes by default.

@strega-nil
Copy link

@Cocalus oh, also, to be clear, the only functions in rust which can deal with unaligned pointers are those that end in _unaligned - copy and copy_nonoverlapping do expect aligned pointers.

@Cocalus
Copy link
Author

Cocalus commented Oct 8, 2018

If that's true then the documentation is wrong, or the name should be fixed.

From https://doc.rust-lang.org/beta/std/ptr/fn.copy_nonoverlapping.html

copy_nonoverlapping is semantically equivalent to C's memcpy

Memcpy just move bytes from one place to another. And is generally the standards correct way to realign data in C.

I'm actively using it in unsafe code to fix the alignment, of a few types. So If that's wrong what is the correct way of copying a bunch of bytes to a particular alignment.

@strega-nil
Copy link

@Cocalus note: https://doc.rust-lang.org/nightly/std/intrinsics/fn.copy_nonoverlapping.html#safety

The correct way is to either 1) use read_unaligned and write_unaligned in a loop, or 2) use copy_nonoverlapping::<u8>

@strega-nil
Copy link

I guess this has been a learning experience, so this is likely a good warning. I'd like to do the following:

  • Change from error -> warning
  • Add a note about the std::ptr functions

are people okay with this?

@Cocalus
Copy link
Author

Cocalus commented Oct 8, 2018

Thanks, the alignment requirement has been added to the nightly docs since I last checked. I think just casting both sides to *u8 and doing the sizeof calculation by hand should work for my needs.

Adding the note that there are some functions that support unaligned is a definite improvement. If clippy can't white list the functions directly, then listing some/all of them should be helpful. I wonder if the pointer types could be extended to have a unaligned type. Then there could be "safe" casts that verify alignment.

For my needs I still want the alignment check in general, since it's easy to mess up in unsafe code. I've been adding allow(cast_ptr_alignment) to the individual calls that I hand verify. Since I don't allow clippy warnings the change from error to warning doesn't really affect my workflow. I'm fine leaving it as an error with a description of the rare exceptions, since it seems far more likely to misused. But I don't know what clippy's standards are for that.

@dtolnay
Copy link
Member

dtolnay commented Apr 13, 2019

+1 for recognizing the specific pattern of ptr::read_unaligned(ptr as *const T) as correct. Example that came up in Chrome OS and required suppressing this lint: platform/crosvm/crosvm_plugin/src/lib.rs

vks added a commit to vks/rust-clippy that referenced this issue Jul 9, 2019
bors added a commit that referenced this issue Jul 9, 2019
cast_ptr_alignment: Mention legal use under known problems

Refs #2881.

changelog: Mention know problems for cast_ptr_alignment
@vks
Copy link
Contributor

vks commented Jul 9, 2019

A note about the std::ptr functions was added, but the lint was not converted to a warning yet.

@kornelski
Copy link
Contributor

Same here. I've discovered it because I had a real bug (yay!), but the warning didn't go away when I used read_unaligned.

weiznich added a commit to weiznich/diesel that referenced this issue Feb 19, 2020
Clippy (rightful) complained that we read from a potential unaligned pointer,
which would be undefined behaviour.
The problem is: Clippy continues to complain about this, even if we
use `read_unaligned()` here. This seems to be a clippy bug, see
rust-lang/rust-clippy#2881
weiznich added a commit to weiznich/diesel that referenced this issue May 11, 2020
Clippy (rightful) complained that we read from a potential unaligned pointer,
which would be undefined behaviour.
The problem is: Clippy continues to complain about this, even if we
use `read_unaligned()` here. This seems to be a clippy bug, see
rust-lang/rust-clippy#2881
weiznich added a commit to weiznich/diesel that referenced this issue Jun 3, 2020
Clippy (rightful) complained that we read from a potential unaligned pointer,
which would be undefined behaviour.
The problem is: Clippy continues to complain about this, even if we
use `read_unaligned()` here. This seems to be a clippy bug, see
rust-lang/rust-clippy#2881
@bors bors closed this as completed in 9fd1cde Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants