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

A debug allocator which removes overalignment from align < 8 allocations #99074

Closed
wants to merge 1 commit into from
Closed
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
122 changes: 122 additions & 0 deletions library/std/src/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

I do want to say that I think this is the right place to insert such checks, since it neatly sidesteps questions about what programs which override the global allocator get to rely on. Thanks!

#[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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should/can we check to ensure the input pointer has the lowest bits set according to the align, and abort if not (because that means they gave us the wrong alignment, which happens).

Copy link
Member Author

@saethlin saethlin Jul 10, 2022

Choose a reason for hiding this comment

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

Are you referring to detecting an attempt to dealloc a different pointer value than one returned by alloc? I think that's a useful property for allocators to have, but it seems like glibc's allocator already does the full implementation of that (as opposed to just checking a few bits), and perhaps as a result people don't make this mistake. I haven't found any code in the wild that would be caught by such a check.

In this PR I'm more interested to know if t-libs is willing to take such an allocator, and I'm starting with a behavior that I know exposes a lot of latent bugs in the wild. We can always add more things to this later if it is accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I mean if you alloc with align 2, then try and dealloc with align 1, the offset calculated will be wrong, wouldn't it? Because the offset is calculated based on the alignment.

Recovering the alignment from the given to dealloc address (if it ends in 0b111, assume alignment 1, if it ends in 0b110, assume 2, 0b100 is 4) and then aborting with an error if it's wrong, because we would have tried to free with the wrong address seems sensible.

As in, the address we were given was the same as the address we passed back from alloc, but the alignment is different.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhhhh I see. Yes the current implementation is definitely not good here, because it turns code which is UB but works into an error that only makes any sense if you know about this implementation.

I think eventually we should detect and report that error, but for this PR I am wary of scope creep and I'm not even sure how to print in an allocator.

Copy link
Member

Choose a reason for hiding this comment

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

I think you could allocate a bit more space to store some information in the bytes directly above the pointer you hand back to to the user (either the real Layout, the originally allocated pointer, etc. There are multiple approaches that could be taken). Then you can extract it with read_unaligned. This would also make it easier to add future checks that use this information to verify other things, such as the layout being correct.

I think it might be slightly clearer than the bitwise trickery in this PR, and it's pretty typical of an allocator to have information in some sort of "header" anyway.

// 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
Copy link
Member

Choose a reason for hiding this comment

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

IMO this really needs to report the issue/abort/something. What it does now seems to just be returning a misaligned pointer silently, and hoping the caller notices. This seems likely to end up not noticed at all, or noticed far away from the allocation site.

I don't have strong thoughts on how to handle it, but I think printing output to stderr and aborting might be a start (or perhaps seeing what handle_alloc_error does? Does it have some solution for the "report-and-abort without unwinding" problem?).

Given that this is mostly the scaffolding for other checks... I think it's worth figuring this out sooner rather than later.

Copy link
Contributor

Choose a reason for hiding this comment

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

An issue is that stderr might not exist, this is in alloc. We always can panic with a message, though, but we'd need a panic_nounwind!() usable in alloc/core, which would be generally useful even outside of this PR, since we have debug assertions that panic when code might not be unwind safe, and we have debug assertions that abort, but then they can't print any info.

handle_alloc_error seems to be for what callers do when their allocation fails. Looks like it can unwind, if someone sets a hook that panics. I don't see anything that specifically prevents that. Haven't tested that though.

Copy link
Member

@thomcc thomcc Jul 11, 2022

Choose a reason for hiding this comment

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

An issue is that stderr might not exist, this is in alloc

The default allocator is in std. If it were in alloc, it couldn't call malloc/HeapAlloc/etc. This does not need to be used for cases where the user does not have the system allocator, or cases where they have overridden it with their own allocator (and I would be fairly strongly opposed to doing anything like this when the allocator has been overridden).

I didn't mean actually use handle_alloc_error for this, I meant see what we do by default in that function, which presumably both prints some message and aborts.

Copy link
Member

Choose a reason for hiding this comment

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

Also to be clear, even in std, you are correct that there may be no standard error stream (such as on wasm*-unknown-unknown). I think it's okay for that to be best effort, so long as the effort is actually pretty good and usually does help the user, (although the aborting should be unconditional, IMO).

And it's a debugging feature, one that is only available via unstable flags. It not being possible on some targets doesn't mean it can't be available on any. We can always just not enable this on targets where we'd have no method of doing anything about detected misuse. That said, I think just about everything with std should be able to do something here (perhaps wasm has no stderr, though).

Copy link
Member Author

Choose a reason for hiding this comment

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

What it does now seems to just be returning a misaligned pointer silently, and hoping the caller notices.

I'm pretty sure this doesn't return a misaligned pointer silently. The effect of this strategy is to populate the lowest 3 bits to the greatest extent possible on the application side, and on the actual system allocator side keep them always clear. So this just indiscriminately clears the lowest 3 bits when handing a pointer back to the system allocator, which I'm pretty sure makes deallocation with the wrong layout totally fine if the underlying system allocator also doesn't care about layout.

// 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<NonNull<[u8]>, AllocError> {
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/common/alloc.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::alloc::{GlobalAlloc, Layout, System};
use crate::alloc::{GlobalAlloc, Layout};
use crate::cmp;
use crate::ptr;

Expand Down Expand Up @@ -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,
Expand Down
6 changes: 4 additions & 2 deletions library/std/src/sys/hermit/alloc.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions library/std/src/sys/sgx/alloc.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions library/std/src/sys/solid/alloc.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions library/std/src/sys/unix/alloc.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions library/std/src/sys/unsupported/alloc.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions library/std/src/sys/wasm/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 4 additions & 2 deletions library/std/src/sys/windows/alloc.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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 {
Expand Down