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

Make non-power-of-two alignments a validity error in Layout #95361

Merged
merged 1 commit into from
Apr 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 5 additions & 6 deletions library/core/src/alloc/layout.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate::cmp;
use crate::fmt;
use crate::mem;
use crate::num::NonZeroUsize;
use crate::mem::{self, ValidAlign};
use crate::ptr::NonNull;

// While this function is used in one place and its implementation
Expand Down Expand Up @@ -40,7 +39,7 @@ pub struct Layout {
//
// (However, we do not analogously require `align >= sizeof(void*)`,
// even though that is *also* a requirement of `posix_memalign`.)
align_: NonZeroUsize,
align_: ValidAlign,
}

impl Layout {
Expand Down Expand Up @@ -97,8 +96,8 @@ impl Layout {
#[must_use]
#[inline]
pub const unsafe fn from_size_align_unchecked(size: usize, align: usize) -> Self {
// SAFETY: the caller must ensure that `align` is greater than zero.
Layout { size_: size, align_: unsafe { NonZeroUsize::new_unchecked(align) } }
// SAFETY: the caller must ensure that `align` is a power of two.
Layout { size_: size, align_: unsafe { ValidAlign::new_unchecked(align) } }
}

/// The minimum size in bytes for a memory block of this layout.
Expand All @@ -117,7 +116,7 @@ impl Layout {
without modifying the layout"]
#[inline]
pub const fn align(&self) -> usize {
self.align_.get()
self.align_.as_nonzero().get()
}

/// Constructs a `Layout` suitable for holding a value of type `T`.
Expand Down
6 changes: 6 additions & 0 deletions library/core/src/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ mod maybe_uninit;
#[stable(feature = "maybe_uninit", since = "1.36.0")]
pub use maybe_uninit::MaybeUninit;

mod valid_align;
// For now this type is left crate-local. It could potentially make sense to expose
// it publicly, as it would be a nice parameter type for methods which need to take
// alignment as a parameter, such as `Layout::padding_needed_for`.
pub(crate) use valid_align::ValidAlign;

#[stable(feature = "rust1", since = "1.0.0")]
#[doc(inline)]
pub use crate::intrinsics::transmute;
Expand Down
240 changes: 240 additions & 0 deletions library/core/src/mem/valid_align.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
use crate::convert::TryFrom;
use crate::num::NonZeroUsize;
use crate::{cmp, fmt, mem, num};

/// A type storing a `usize` which is a power of two, and thus
/// represents a possible alignment in the rust abstract machine.
///
/// Note that particularly large alignments, while representable in this type,
/// are likely not to be supported by actual allocators and linkers.
#[derive(Copy, Clone)]
#[repr(transparent)]
pub(crate) struct ValidAlign(ValidAlignEnum);

// ValidAlign is `repr(usize)`, but via extra steps.
const _: () = assert!(mem::size_of::<ValidAlign>() == mem::size_of::<usize>());
const _: () = assert!(mem::align_of::<ValidAlign>() == mem::align_of::<usize>());

impl ValidAlign {
/// Creates a `ValidAlign` from a power-of-two `usize`.
///
/// # Safety
///
/// `align` must be a power of two.
///
/// Equivalently, it must be `1 << exp` for some `exp` in `0..usize::BITS`.
/// It must *not* be zero.
#[inline]
pub(crate) const unsafe fn new_unchecked(align: usize) -> Self {
debug_assert!(align.is_power_of_two());

// SAFETY: By precondition, this must be a power of two, and
// our variants encompass all possible powers of two.
unsafe { mem::transmute::<usize, ValidAlign>(align) }
}

#[inline]
pub(crate) const fn as_nonzero(self) -> NonZeroUsize {
// SAFETY: All the discriminants are non-zero.
unsafe { NonZeroUsize::new_unchecked(self.0 as usize) }
}

/// Returns the base 2 logarithm of the alignment.
///
/// This is always exact, as `self` represents a power of two.
#[inline]
pub(crate) fn log2(self) -> u32 {
self.as_nonzero().trailing_zeros()
}
}

impl fmt::Debug for ValidAlign {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{:?} (1 << {:?})", self.as_nonzero(), self.log2())
}
}

impl TryFrom<NonZeroUsize> for ValidAlign {
type Error = num::TryFromIntError;

#[inline]
fn try_from(align: NonZeroUsize) -> Result<ValidAlign, Self::Error> {
if align.is_power_of_two() {
// SAFETY: Just checked for power-of-two
unsafe { Ok(ValidAlign::new_unchecked(align.get())) }
} else {
Err(num::TryFromIntError(()))
}
}
}

impl TryFrom<usize> for ValidAlign {
type Error = num::TryFromIntError;

#[inline]
fn try_from(align: usize) -> Result<ValidAlign, Self::Error> {
if align.is_power_of_two() {
// SAFETY: Just checked for power-of-two
unsafe { Ok(ValidAlign::new_unchecked(align)) }
} else {
Err(num::TryFromIntError(()))
}
}
}

impl cmp::Eq for ValidAlign {}

impl cmp::PartialEq for ValidAlign {
#[inline]
fn eq(&self, other: &Self) -> bool {
self.as_nonzero() == other.as_nonzero()
}
}

impl cmp::Ord for ValidAlign {
#[inline]
fn cmp(&self, other: &Self) -> cmp::Ordering {
self.as_nonzero().cmp(&other.as_nonzero())
}
}

impl cmp::PartialOrd for ValidAlign {
#[inline]
fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
Some(self.cmp(other))
}
}

#[cfg(target_pointer_width = "16")]
type ValidAlignEnum = ValidAlignEnum16;
#[cfg(target_pointer_width = "32")]
type ValidAlignEnum = ValidAlignEnum32;
#[cfg(target_pointer_width = "64")]
type ValidAlignEnum = ValidAlignEnum64;

#[derive(Copy, Clone)]
#[repr(u16)]
enum ValidAlignEnum16 {
_Align1Shl0 = 1 << 0,
_Align1Shl1 = 1 << 1,
_Align1Shl2 = 1 << 2,
_Align1Shl3 = 1 << 3,
_Align1Shl4 = 1 << 4,
_Align1Shl5 = 1 << 5,
_Align1Shl6 = 1 << 6,
_Align1Shl7 = 1 << 7,
_Align1Shl8 = 1 << 8,
_Align1Shl9 = 1 << 9,
_Align1Shl10 = 1 << 10,
_Align1Shl11 = 1 << 11,
_Align1Shl12 = 1 << 12,
_Align1Shl13 = 1 << 13,
_Align1Shl14 = 1 << 14,
_Align1Shl15 = 1 << 15,
}

#[derive(Copy, Clone)]
#[repr(u32)]
enum ValidAlignEnum32 {
_Align1Shl0 = 1 << 0,
_Align1Shl1 = 1 << 1,
_Align1Shl2 = 1 << 2,
_Align1Shl3 = 1 << 3,
_Align1Shl4 = 1 << 4,
_Align1Shl5 = 1 << 5,
_Align1Shl6 = 1 << 6,
_Align1Shl7 = 1 << 7,
_Align1Shl8 = 1 << 8,
_Align1Shl9 = 1 << 9,
_Align1Shl10 = 1 << 10,
_Align1Shl11 = 1 << 11,
_Align1Shl12 = 1 << 12,
_Align1Shl13 = 1 << 13,
_Align1Shl14 = 1 << 14,
_Align1Shl15 = 1 << 15,
_Align1Shl16 = 1 << 16,
_Align1Shl17 = 1 << 17,
_Align1Shl18 = 1 << 18,
_Align1Shl19 = 1 << 19,
_Align1Shl20 = 1 << 20,
_Align1Shl21 = 1 << 21,
_Align1Shl22 = 1 << 22,
_Align1Shl23 = 1 << 23,
_Align1Shl24 = 1 << 24,
_Align1Shl25 = 1 << 25,
_Align1Shl26 = 1 << 26,
_Align1Shl27 = 1 << 27,
_Align1Shl28 = 1 << 28,
_Align1Shl29 = 1 << 29,
_Align1Shl30 = 1 << 30,
_Align1Shl31 = 1 << 31,
}

#[derive(Copy, Clone)]
#[repr(u64)]
enum ValidAlignEnum64 {
_Align1Shl0 = 1 << 0,
_Align1Shl1 = 1 << 1,
_Align1Shl2 = 1 << 2,
_Align1Shl3 = 1 << 3,
_Align1Shl4 = 1 << 4,
_Align1Shl5 = 1 << 5,
_Align1Shl6 = 1 << 6,
_Align1Shl7 = 1 << 7,
_Align1Shl8 = 1 << 8,
_Align1Shl9 = 1 << 9,
_Align1Shl10 = 1 << 10,
_Align1Shl11 = 1 << 11,
_Align1Shl12 = 1 << 12,
_Align1Shl13 = 1 << 13,
_Align1Shl14 = 1 << 14,
_Align1Shl15 = 1 << 15,
_Align1Shl16 = 1 << 16,
_Align1Shl17 = 1 << 17,
_Align1Shl18 = 1 << 18,
_Align1Shl19 = 1 << 19,
_Align1Shl20 = 1 << 20,
_Align1Shl21 = 1 << 21,
_Align1Shl22 = 1 << 22,
_Align1Shl23 = 1 << 23,
_Align1Shl24 = 1 << 24,
_Align1Shl25 = 1 << 25,
_Align1Shl26 = 1 << 26,
_Align1Shl27 = 1 << 27,
_Align1Shl28 = 1 << 28,
_Align1Shl29 = 1 << 29,
_Align1Shl30 = 1 << 30,
_Align1Shl31 = 1 << 31,
_Align1Shl32 = 1 << 32,
_Align1Shl33 = 1 << 33,
_Align1Shl34 = 1 << 34,
_Align1Shl35 = 1 << 35,
_Align1Shl36 = 1 << 36,
_Align1Shl37 = 1 << 37,
_Align1Shl38 = 1 << 38,
_Align1Shl39 = 1 << 39,
_Align1Shl40 = 1 << 40,
_Align1Shl41 = 1 << 41,
_Align1Shl42 = 1 << 42,
_Align1Shl43 = 1 << 43,
_Align1Shl44 = 1 << 44,
_Align1Shl45 = 1 << 45,
_Align1Shl46 = 1 << 46,
_Align1Shl47 = 1 << 47,
_Align1Shl48 = 1 << 48,
_Align1Shl49 = 1 << 49,
_Align1Shl50 = 1 << 50,
_Align1Shl51 = 1 << 51,
_Align1Shl52 = 1 << 52,
_Align1Shl53 = 1 << 53,
_Align1Shl54 = 1 << 54,
_Align1Shl55 = 1 << 55,
_Align1Shl56 = 1 << 56,
_Align1Shl57 = 1 << 57,
_Align1Shl58 = 1 << 58,
_Align1Shl59 = 1 << 59,
_Align1Shl60 = 1 << 60,
_Align1Shl61 = 1 << 61,
_Align1Shl62 = 1 << 62,
_Align1Shl63 = 1 << 63,
}
18 changes: 18 additions & 0 deletions library/core/tests/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,21 @@ fn const_unchecked_layout() {
assert_eq!(LAYOUT.align(), ALIGN);
assert_eq!(Some(DANGLING), NonNull::new(ALIGN as *mut u8));
}

#[test]
fn layout_debug_shows_log2_of_alignment() {
// `Debug` is not stable, but here's what it does right now
let layout = Layout::from_size_align(24576, 8192).unwrap();
let s = format!("{:?}", layout);
assert_eq!(s, "Layout { size_: 24576, align_: 8192 (1 << 13) }");
}

// Running this normally doesn't do much, but it's also run in Miri, which
// will double-check that these are allowed by the validity invariants.
#[test]
fn layout_accepts_all_valid_alignments() {
for align in 0..usize::BITS {
let layout = Layout::from_size_align(0, 1_usize << align).unwrap();
assert_eq!(layout.align(), 1_usize << align);
}
}
19 changes: 15 additions & 4 deletions src/test/ui/consts/std/alloc.32bit.stderr
Original file line number Diff line number Diff line change
@@ -1,14 +1,25 @@
error[E0080]: it is undefined behavior to use this value
--> $DIR/alloc.rs:8:1
--> $DIR/alloc.rs:9:1
|
LL | const LAYOUT_INVALID: Layout = unsafe { Layout::from_size_align_unchecked(0x1000, 0x00) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed at .align_: encountered 0, but expected something greater or equal to 1
LL | const LAYOUT_INVALID_ZERO: Layout = unsafe { Layout::from_size_align_unchecked(0x1000, 0x00) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed at .align_.0.<enum-tag>: encountered 0x00000000, but expected a valid enum tag
Copy link
Member

@bjorn3 bjorn3 Mar 27, 2022

Choose a reason for hiding this comment

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

This error message is not exactly clear about what property the value needs to have.

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 think that's ok. It's a best-effort UB check -- the place to look is at the # Safety comment, not the error message.

One gets essentially the same error from things like this too:

pub const BAD_ORDERING: std::cmp::Ordering = unsafe { std::mem::transmute(3_u8) };

So it'll get better here if the error in general gets better for enums, which seems fine for this PR.

|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
= note: the raw bytes of the constant (size: 8, align: 4) {
00 10 00 00 00 00 00 00 │ ........
}

error: aborting due to previous error
error[E0080]: it is undefined behavior to use this value
--> $DIR/alloc.rs:13:1
|
LL | const LAYOUT_INVALID_THREE: Layout = unsafe { Layout::from_size_align_unchecked(9, 3) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed at .align_.0.<enum-tag>: encountered 0x00000003, but expected a valid enum tag
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
= note: the raw bytes of the constant (size: 8, align: 4) {
09 00 00 00 03 00 00 00 │ ........
}

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
19 changes: 15 additions & 4 deletions src/test/ui/consts/std/alloc.64bit.stderr
Original file line number Diff line number Diff line change
@@ -1,14 +1,25 @@
error[E0080]: it is undefined behavior to use this value
--> $DIR/alloc.rs:8:1
--> $DIR/alloc.rs:9:1
|
LL | const LAYOUT_INVALID: Layout = unsafe { Layout::from_size_align_unchecked(0x1000, 0x00) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed at .align_: encountered 0, but expected something greater or equal to 1
LL | const LAYOUT_INVALID_ZERO: Layout = unsafe { Layout::from_size_align_unchecked(0x1000, 0x00) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed at .align_.0.<enum-tag>: encountered 0x0000000000000000, but expected a valid enum tag
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
= note: the raw bytes of the constant (size: 16, align: 8) {
00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 │ ................
}

error: aborting due to previous error
error[E0080]: it is undefined behavior to use this value
--> $DIR/alloc.rs:13:1
|
LL | const LAYOUT_INVALID_THREE: Layout = unsafe { Layout::from_size_align_unchecked(9, 3) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed at .align_.0.<enum-tag>: encountered 0x0000000000000003, but expected a valid enum tag
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
= note: the raw bytes of the constant (size: 16, align: 8) {
09 00 00 00 00 00 00 00 03 00 00 00 00 00 00 00 │ ................
}

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
7 changes: 6 additions & 1 deletion src/test/ui/consts/std/alloc.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
// stderr-per-bitwidth
// ignore-debug (the debug assertions change the error)
use std::alloc::Layout;

// ok
const LAYOUT_VALID: Layout = unsafe { Layout::from_size_align_unchecked(0x1000, 0x08) };

// not ok, since alignment needs to be non-zero.
const LAYOUT_INVALID: Layout = unsafe { Layout::from_size_align_unchecked(0x1000, 0x00) };
const LAYOUT_INVALID_ZERO: Layout = unsafe { Layout::from_size_align_unchecked(0x1000, 0x00) };
//~^ ERROR it is undefined behavior to use this value

// not ok, since alignment needs to be a power of two.
const LAYOUT_INVALID_THREE: Layout = unsafe { Layout::from_size_align_unchecked(9, 3) };
//~^ ERROR it is undefined behavior to use this value

fn main() {}