From d641a1d3d5116cf6b41887b8dc876ce2a0b7cf3e Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sun, 8 May 2022 01:41:31 -0400 Subject: [PATCH] Remove overalignment from align < 8 allocations This reorganizes the implementation of the System allocator to permit adding various debuggig features. Currently, all that this implements is a scheme that allocates a little extra space for low-alignment allocations then returns a pointer into the actual allocation which is offset so that it is not over-aligned. This is a huge aid in discovering accidental reliance on over-alignment. Allocators designed for C can be relied upon to produce over-aligned pointers, so alignment-related bugs can be latent for a long time. This implementation is also factored so accommodate other patches to the default allocator which can help in detecting other sources of UB. --- library/std/src/alloc.rs | 122 +++++++++++++++++++++++ library/std/src/sys/common/alloc.rs | 4 +- library/std/src/sys/hermit/alloc.rs | 6 +- library/std/src/sys/sgx/alloc.rs | 6 +- library/std/src/sys/solid/alloc.rs | 6 +- library/std/src/sys/unix/alloc.rs | 6 +- library/std/src/sys/unsupported/alloc.rs | 6 +- library/std/src/sys/wasm/alloc.rs | 6 +- library/std/src/sys/windows/alloc.rs | 6 +- 9 files changed, 152 insertions(+), 16 deletions(-) diff --git a/library/std/src/alloc.rs b/library/std/src/alloc.rs index a05e0db3af716..281fce91b92ba 100644 --- a/library/std/src/alloc.rs +++ b/library/std/src/alloc.rs @@ -127,6 +127,128 @@ pub use alloc_crate::alloc::*; #[derive(Debug, Default, Copy, Clone)] pub struct System; +use crate::sys::alloc::System as Imp; + +// When debug assertions are not enabled, `System` just forwards down to the particular platform +// implementation. +#[cfg(not(debug_assertions))] +#[stable(feature = "alloc_system_type", since = "1.28.0")] +unsafe impl GlobalAlloc for System { + #[inline] + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + unsafe { Imp.alloc(layout) } + } + + #[inline] + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + unsafe { Imp.alloc_zeroed(layout) } + } + + #[inline] + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + unsafe { Imp.dealloc(ptr, layout) } + } + + #[inline] + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + unsafe { Imp.realloc(ptr, layout, new_size) } + } +} + +// Some system allocators (most notably any provided by calling malloc) will always return pointers +// with an alignment of 8. So for any allocation with an alignment less than 8, we increase the +// alignment to 8 and return a pointer which is offset into the allocation such that it is not +// over-aligned. +// We always bump up the size of an allocation by 8 when the alignment is less than 8. +#[cfg(debug_assertions)] +trait LayoutExt { + fn with_alignment_padding(self) -> Self; + unsafe fn add_alignment_padding(self, ptr: *mut u8) -> *mut u8; + unsafe fn remove_alignment_padding(self, ptr: *mut u8) -> *mut u8; +} +#[cfg(debug_assertions)] +impl LayoutExt for Layout { + fn with_alignment_padding(self) -> Self { + if self.align() < 8 { + Layout::from_size_align(self.size() + (8 - self.align()), 8).unwrap() + } else { + self + } + } + + unsafe fn add_alignment_padding(self, ptr: *mut u8) -> *mut u8 { + if !ptr.is_null() && self.align() < 8 { + // SAFETY: This must be called on a pointer previously returned by a padded Layout, + // which will always have space to do this offset + unsafe { ptr.add(8 - self.align()) } + } else { + ptr + } + } + + unsafe fn remove_alignment_padding(self, ptr: *mut u8) -> *mut u8 { + // We cannot just do the inverse of add_alignment_padding, because if a user deallocates + // with the wrong Layout, we would use that wrong Layout here to deduce the wrong offset to + // remove from the pointer. That would turn code that works fine because the underlying + // allocator ignores the Layout (but is technically UB) into code which does worse UB or + // halts the program with an unhelpful diagnostic from the underlying allocator. + // So we have two reasonable options. We could detect and clearly report the error + // ourselves, or since we know that all our alignment adjustments involve the low 3 bits, + // we could clear those and make this allocator transparent. + // At the moment we do the latter because it is unclear how to emit an error message from + // inside an allocator. + const ALIGNMENT_MASK: usize = usize::MAX << 3; + ptr.map_addr(|addr| addr & ALIGNMENT_MASK) + } +} + +// When debug assertions are enabled, we wrap the platform allocator with extra logic to help +// expose bugs. +#[cfg(debug_assertions)] +#[stable(feature = "alloc_system_type", since = "1.28.0")] +unsafe impl GlobalAlloc for System { + #[inline] + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + if layout.size() > isize::MAX as usize - 8 { + return ptr::null_mut(); + } + unsafe { + let ptr = Imp.alloc(layout.with_alignment_padding()); + layout.add_alignment_padding(ptr) + } + } + + #[inline] + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + unsafe { + let ptr = Imp.alloc_zeroed(layout.with_alignment_padding()); + layout.add_alignment_padding(ptr) + } + } + + #[inline] + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + unsafe { + let ptr = layout.remove_alignment_padding(ptr); + Imp.dealloc(ptr, layout.with_alignment_padding()) + } + } + + #[inline] + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + if new_size > isize::MAX as usize - 8 { + return ptr::null_mut(); + } + unsafe { + let ptr = layout.remove_alignment_padding(ptr); + let new_layout = + Layout::from_size_align(new_size, layout.align()).unwrap().with_alignment_padding(); + let ptr = Imp.realloc(ptr, layout.with_alignment_padding(), new_layout.size()); + layout.add_alignment_padding(ptr) + } + } +} + impl System { #[inline] fn alloc_impl(&self, layout: Layout, zeroed: bool) -> Result, AllocError> { diff --git a/library/std/src/sys/common/alloc.rs b/library/std/src/sys/common/alloc.rs index e8e7c51cb9ba6..5adcf46f5a4b0 100644 --- a/library/std/src/sys/common/alloc.rs +++ b/library/std/src/sys/common/alloc.rs @@ -1,4 +1,4 @@ -use crate::alloc::{GlobalAlloc, Layout, System}; +use crate::alloc::{GlobalAlloc, Layout}; use crate::cmp; use crate::ptr; @@ -36,7 +36,7 @@ pub const MIN_ALIGN: usize = 16; pub const MIN_ALIGN: usize = 4; pub unsafe fn realloc_fallback( - alloc: &System, + alloc: &impl GlobalAlloc, ptr: *mut u8, old_layout: Layout, new_size: usize, diff --git a/library/std/src/sys/hermit/alloc.rs b/library/std/src/sys/hermit/alloc.rs index d153914e77e10..c09179263c0ee 100644 --- a/library/std/src/sys/hermit/alloc.rs +++ b/library/std/src/sys/hermit/alloc.rs @@ -1,8 +1,10 @@ -use crate::alloc::{GlobalAlloc, Layout, System}; +use crate::alloc::{GlobalAlloc, Layout}; use crate::ptr; use crate::sys::hermit::abi; -#[stable(feature = "alloc_system_type", since = "1.28.0")] +#[derive(Debug, Default, Copy, Clone)] +pub(crate) struct System; + unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 { diff --git a/library/std/src/sys/sgx/alloc.rs b/library/std/src/sys/sgx/alloc.rs index 4aea28cb83e23..e8885ce9e0d24 100644 --- a/library/std/src/sys/sgx/alloc.rs +++ b/library/std/src/sys/sgx/alloc.rs @@ -1,4 +1,4 @@ -use crate::alloc::{GlobalAlloc, Layout, System}; +use crate::alloc::{GlobalAlloc, Layout}; use crate::ptr; use crate::sys::sgx::abi::mem as sgx_mem; use core::sync::atomic::{AtomicBool, Ordering}; @@ -56,7 +56,9 @@ unsafe impl dlmalloc::Allocator for Sgx { } } -#[stable(feature = "alloc_system_type", since = "1.28.0")] +#[derive(Debug, Default, Copy, Clone)] +pub(crate) struct System; + unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 { diff --git a/library/std/src/sys/solid/alloc.rs b/library/std/src/sys/solid/alloc.rs index d013bd8761003..285d3e661bff0 100644 --- a/library/std/src/sys/solid/alloc.rs +++ b/library/std/src/sys/solid/alloc.rs @@ -1,9 +1,11 @@ use crate::{ - alloc::{GlobalAlloc, Layout, System}, + alloc::{GlobalAlloc, Layout}, sys::common::alloc::{realloc_fallback, MIN_ALIGN}, }; -#[stable(feature = "alloc_system_type", since = "1.28.0")] +#[derive(Debug, Default, Copy, Clone)] +pub(crate) struct System; + unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 { diff --git a/library/std/src/sys/unix/alloc.rs b/library/std/src/sys/unix/alloc.rs index 9d6567c9fb471..b0224ebffcced 100644 --- a/library/std/src/sys/unix/alloc.rs +++ b/library/std/src/sys/unix/alloc.rs @@ -1,8 +1,10 @@ -use crate::alloc::{GlobalAlloc, Layout, System}; +use crate::alloc::{GlobalAlloc, Layout}; use crate::ptr; use crate::sys::common::alloc::{realloc_fallback, MIN_ALIGN}; -#[stable(feature = "alloc_system_type", since = "1.28.0")] +#[derive(Debug, Default, Copy, Clone)] +pub(crate) struct System; + unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 { diff --git a/library/std/src/sys/unsupported/alloc.rs b/library/std/src/sys/unsupported/alloc.rs index d715ae45401e6..d50039229f84d 100644 --- a/library/std/src/sys/unsupported/alloc.rs +++ b/library/std/src/sys/unsupported/alloc.rs @@ -1,7 +1,9 @@ -use crate::alloc::{GlobalAlloc, Layout, System}; +use crate::alloc::{GlobalAlloc, Layout}; use crate::ptr::null_mut; -#[stable(feature = "alloc_system_type", since = "1.28.0")] +#[derive(Debug, Default, Copy, Clone)] +pub(crate) struct System; + unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, _layout: Layout) -> *mut u8 { diff --git a/library/std/src/sys/wasm/alloc.rs b/library/std/src/sys/wasm/alloc.rs index 6dceb1689a8b7..4c94a0576b191 100644 --- a/library/std/src/sys/wasm/alloc.rs +++ b/library/std/src/sys/wasm/alloc.rs @@ -16,11 +16,13 @@ //! The crate itself provides a global allocator which on wasm has no //! synchronization as there are no threads! -use crate::alloc::{GlobalAlloc, Layout, System}; +use crate::alloc::{GlobalAlloc, Layout}; static mut DLMALLOC: dlmalloc::Dlmalloc = dlmalloc::Dlmalloc::new(); -#[stable(feature = "alloc_system_type", since = "1.28.0")] +#[derive(Debug, Default, Copy, Clone)] +pub(crate) struct System; + unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 { diff --git a/library/std/src/sys/windows/alloc.rs b/library/std/src/sys/windows/alloc.rs index fdc81cdea7dec..bb8d7c1031758 100644 --- a/library/std/src/sys/windows/alloc.rs +++ b/library/std/src/sys/windows/alloc.rs @@ -1,6 +1,6 @@ #![deny(unsafe_op_in_unsafe_fn)] -use crate::alloc::{GlobalAlloc, Layout, System}; +use crate::alloc::{GlobalAlloc, Layout}; use crate::ffi::c_void; use crate::ptr; use crate::sync::atomic::{AtomicPtr, Ordering}; @@ -187,7 +187,9 @@ unsafe fn allocate(layout: Layout, zeroed: bool) -> *mut u8 { // the pointer will be aligned to the specified alignment and not point to the start of the allocated block. // Instead there will be a header readable directly before the returned pointer, containing the actual // location of the start of the block. -#[stable(feature = "alloc_system_type", since = "1.28.0")] +#[derive(Debug, Default, Copy, Clone)] +pub(crate) struct System; + unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 {