Skip to content

Commit

Permalink
Fix issues from review and unsoundness of RawVec::into_box
Browse files Browse the repository at this point in the history
  • Loading branch information
TimDiekmann committed Mar 26, 2020
1 parent 56cbf2f commit 2526acc
Show file tree
Hide file tree
Showing 17 changed files with 430 additions and 468 deletions.
96 changes: 45 additions & 51 deletions src/liballoc/alloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use core::intrinsics::{self, min_align_of_val, size_of_val};
use core::ptr::{NonNull, Unique};
use core::usize;
use core::{mem, usize};

#[stable(feature = "alloc_module", since = "1.28.0")]
#[doc(inline)]
Expand Down Expand Up @@ -165,102 +165,96 @@ pub unsafe fn alloc_zeroed(layout: Layout) -> *mut u8 {
#[unstable(feature = "allocator_api", issue = "32838")]
unsafe impl AllocRef for Global {
#[inline]
fn alloc(&mut self, layout: Layout, init: AllocInit) -> Result<(NonNull<u8>, usize), AllocErr> {
let new_size = layout.size();
if new_size == 0 {
Ok((layout.dangling(), 0))
} else {
unsafe {
fn alloc(&mut self, layout: Layout, init: AllocInit) -> Result<MemoryBlock, AllocErr> {
unsafe {
if layout.size() == 0 {
Ok(MemoryBlock::new(layout.dangling(), layout))
} else {
let raw_ptr = match init {
AllocInit::Uninitialized => alloc(layout),
AllocInit::Zeroed => alloc_zeroed(layout),
};
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Ok((ptr, new_size))
Ok(MemoryBlock::new(ptr, layout))
}
}
}

#[inline]
unsafe fn dealloc(&mut self, ptr: NonNull<u8>, layout: Layout) {
if layout.size() != 0 {
dealloc(ptr.as_ptr(), layout)
unsafe fn dealloc(&mut self, memory: MemoryBlock) {
if memory.size() != 0 {
dealloc(memory.ptr().as_ptr(), memory.layout())
}
}

#[inline]
unsafe fn grow(
&mut self,
ptr: NonNull<u8>,
layout: Layout,
memory: &mut MemoryBlock,
new_size: usize,
placement: ReallocPlacement,
init: AllocInit,
) -> Result<(NonNull<u8>, usize), AllocErr> {
let old_size = layout.size();
) -> Result<(), AllocErr> {
let old_size = memory.size();
debug_assert!(
new_size >= old_size,
"`new_size` must be greater than or equal to `layout.size()`"
"`new_size` must be greater than or equal to `memory.size()`"
);

if old_size == new_size {
return Ok((ptr, new_size));
return Ok(());
}

let new_layout = Layout::from_size_align_unchecked(new_size, memory.align());
match placement {
ReallocPlacement::InPlace => return Err(AllocErr),
ReallocPlacement::MayMove if memory.size() == 0 => {
*memory = self.alloc(new_layout, init)?
}
ReallocPlacement::MayMove => {
if old_size == 0 {
self.alloc(Layout::from_size_align_unchecked(new_size, layout.align()), init)
} else {
// `realloc` probably checks for `new_size > old_size` or something similar.
// `new_size` must be greater than or equal to `old_size` due to the safety constraint,
// and `new_size` == `old_size` was caught before
intrinsics::assume(new_size > old_size);
let ptr =
NonNull::new(realloc(ptr.as_ptr(), layout, new_size)).ok_or(AllocErr)?;
let new_layout = Layout::from_size_align_unchecked(new_size, layout.align());
init.initialize_offset(ptr, new_layout, old_size);
Ok((ptr, new_size))
}
// `realloc` probably checks for `new_size > old_size` or something similar.
intrinsics::assume(new_size > old_size);
let ptr = realloc(memory.ptr().as_ptr(), memory.layout(), new_size);
*memory = MemoryBlock::new(NonNull::new(ptr).ok_or(AllocErr)?, new_layout);
memory.init_offset(init, old_size);
}
ReallocPlacement::InPlace => Err(AllocErr),
}
Ok(())
}

#[inline]
unsafe fn shrink(
&mut self,
ptr: NonNull<u8>,
layout: Layout,
memory: &mut MemoryBlock,
new_size: usize,
placement: ReallocPlacement,
) -> Result<(NonNull<u8>, usize), AllocErr> {
let old_size = layout.size();
) -> Result<(), AllocErr> {
let old_size = memory.size();
debug_assert!(
new_size <= old_size,
"`new_size` must be smaller than or equal to `layout.size()`"
"`new_size` must be smaller than or equal to `memory.size()`"
);

if old_size == new_size {
return Ok((ptr, new_size));
return Ok(());
}

let new_layout = Layout::from_size_align_unchecked(new_size, memory.align());
match placement {
ReallocPlacement::InPlace => return Err(AllocErr),
ReallocPlacement::MayMove if new_size == 0 => {
let new_memory = MemoryBlock::new(new_layout.dangling(), new_layout);
let old_memory = mem::replace(memory, new_memory);
self.dealloc(old_memory)
}
ReallocPlacement::MayMove => {
let ptr = if new_size == 0 {
self.dealloc(ptr, layout);
layout.dangling()
} else {
// `realloc` probably checks for `new_size > old_size` or something similar.
// `new_size` must be smaller than or equal to `old_size` due to the safety constraint,
// and `new_size` == `old_size` was caught before
intrinsics::assume(new_size < old_size);
NonNull::new(realloc(ptr.as_ptr(), layout, new_size)).ok_or(AllocErr)?
};
Ok((ptr, new_size))
// `realloc` probably checks for `new_size < old_size` or something similar.
intrinsics::assume(new_size < old_size);
let ptr = realloc(memory.ptr().as_ptr(), memory.layout(), new_size);
*memory = MemoryBlock::new(NonNull::new(ptr).ok_or(AllocErr)?, new_layout);
}
ReallocPlacement::InPlace => Err(AllocErr),
}
Ok(())
}
}

Expand All @@ -272,7 +266,7 @@ unsafe impl AllocRef for Global {
unsafe fn exchange_malloc(size: usize, align: usize) -> *mut u8 {
let layout = Layout::from_size_align_unchecked(size, align);
match Global.alloc(layout, AllocInit::Uninitialized) {
Ok((ptr, _)) => ptr.as_ptr(),
Ok(memory) => memory.ptr().as_ptr(),
Err(_) => handle_alloc_error(layout),
}
}
Expand All @@ -288,7 +282,7 @@ pub(crate) unsafe fn box_free<T: ?Sized>(ptr: Unique<T>) {
let size = size_of_val(ptr.as_ref());
let align = min_align_of_val(ptr.as_ref());
let layout = Layout::from_size_align_unchecked(size, align);
Global.dealloc(ptr.cast().into(), layout)
Global.dealloc(MemoryBlock::new(ptr.cast().into(), layout))
}

/// Abort on memory allocation error or failure.
Expand Down
6 changes: 3 additions & 3 deletions src/liballoc/alloc/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@ use test::Bencher;
fn allocate_zeroed() {
unsafe {
let layout = Layout::from_size_align(1024, 1).unwrap();
let (ptr, _) = Global
let memory = Global
.alloc(layout.clone(), AllocInit::Zeroed)
.unwrap_or_else(|_| handle_alloc_error(layout));

let mut i = ptr.cast::<u8>().as_ptr();
let mut i = memory.ptr().cast::<u8>().as_ptr();
let end = i.add(layout.size());
while i < end {
assert_eq!(*i, 0);
i = i.offset(1);
}
Global.dealloc(ptr, layout);
Global.dealloc(memory);
}
}

Expand Down
15 changes: 4 additions & 11 deletions src/liballoc/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ use core::ops::{
};
use core::pin::Pin;
use core::ptr::{self, NonNull, Unique};
use core::slice;
use core::task::{Context, Poll};

use crate::alloc::{self, AllocInit, AllocRef, Global};
Expand Down Expand Up @@ -199,7 +198,7 @@ impl<T> Box<T> {
let ptr = Global
.alloc(layout, AllocInit::Uninitialized)
.unwrap_or_else(|_| alloc::handle_alloc_error(layout))
.0
.ptr()
.cast();
unsafe { Box::from_raw(ptr.as_ptr()) }
}
Expand Down Expand Up @@ -228,7 +227,7 @@ impl<T> Box<T> {
let ptr = Global
.alloc(layout, AllocInit::Zeroed)
.unwrap_or_else(|_| alloc::handle_alloc_error(layout))
.0
.ptr()
.cast();
unsafe { Box::from_raw(ptr.as_ptr()) }
}
Expand Down Expand Up @@ -265,13 +264,7 @@ impl<T> Box<[T]> {
/// ```
#[unstable(feature = "new_uninit", issue = "63291")]
pub fn new_uninit_slice(len: usize) -> Box<[mem::MaybeUninit<T>]> {
let layout = alloc::Layout::array::<mem::MaybeUninit<T>>(len).unwrap();
let ptr = Global
.alloc(layout, AllocInit::Uninitialized)
.unwrap_or_else(|_| alloc::handle_alloc_error(layout))
.0
.cast();
unsafe { Box::from_raw(slice::from_raw_parts_mut(ptr.as_ptr(), len)) }
unsafe { RawVec::with_capacity(len).into_box(len) }
}
}

Expand Down Expand Up @@ -776,7 +769,7 @@ impl<T: Copy> From<&[T]> for Box<[T]> {
let buf = RawVec::with_capacity(len);
unsafe {
ptr::copy_nonoverlapping(slice.as_ptr(), buf.ptr(), len);
buf.into_box().assume_init()
buf.into_box(slice.len()).assume_init()
}
}
}
Expand Down
19 changes: 12 additions & 7 deletions src/liballoc/collections/btree/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
// - A node of length `n` has `n` keys, `n` values, and (in an internal node) `n + 1` edges.
// This implies that even an empty internal node has at least one edge.

use core::alloc::MemoryBlock;
use core::cmp::Ordering;
use core::marker::PhantomData;
use core::mem::{self, MaybeUninit};
Expand Down Expand Up @@ -227,7 +228,10 @@ impl<K, V> Root<K, V> {
}

unsafe {
Global.dealloc(NonNull::from(top).cast(), Layout::new::<InternalNode<K, V>>());
Global.dealloc(MemoryBlock::new(
NonNull::from(top).cast(),
Layout::new::<InternalNode<K, V>>(),
));
}
}
}
Expand Down Expand Up @@ -392,14 +396,14 @@ impl<K, V> NodeRef<marker::Owned, K, V, marker::LeafOrInternal> {
let height = self.height;
let node = self.node;
let ret = self.ascend().ok();
Global.dealloc(
Global.dealloc(MemoryBlock::new(
node.cast(),
if height > 0 {
Layout::new::<InternalNode<K, V>>()
} else {
Layout::new::<LeafNode<K, V>>()
},
);
));
ret
}
}
Expand Down Expand Up @@ -1142,7 +1146,7 @@ impl<'a, K, V> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Internal>, marker::

(*left_node.as_leaf_mut()).len += right_len as u16 + 1;

if self.node.height > 1 {
let layout = if self.node.height > 1 {
ptr::copy_nonoverlapping(
right_node.cast_unchecked().as_internal().edges.as_ptr(),
left_node
Expand All @@ -1159,10 +1163,11 @@ impl<'a, K, V> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Internal>, marker::
.correct_parent_link();
}

Global.dealloc(right_node.node.cast(), Layout::new::<InternalNode<K, V>>());
Layout::new::<InternalNode<K, V>>()
} else {
Global.dealloc(right_node.node.cast(), Layout::new::<LeafNode<K, V>>());
}
Layout::new::<LeafNode<K, V>>()
};
Global.dealloc(MemoryBlock::new(right_node.node.cast(), layout));

Handle::new_edge(self.node, self.idx)
}
Expand Down
1 change: 1 addition & 0 deletions src/liballoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
#![feature(lang_items)]
#![feature(libc)]
#![cfg_attr(not(bootstrap), feature(negative_impls))]
#![feature(new_uninit)]
#![feature(nll)]
#![feature(optin_builtin_traits)]
#![feature(pattern)]
Expand Down
Loading

0 comments on commit 2526acc

Please sign in to comment.