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.

Sign up for GitHub

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

Jump to bottom

Fix provenance UB and alignment UB #27

Merged
merged 4 commits into from
Feb 15, 2022
Merged

Fix provenance UB and alignment UB #27

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aba7a07
Fix provenance UB and alignment UB
saethlin Feb 12, 2022
2ab260b
Fix (for real this time) issues with alignment
saethlin Feb 14, 2022
a3df169
Duplicate the padding check in constructors
saethlin Feb 15, 2022
3b4eb98
Fix unsafe block
Gankra Feb 15, 2022
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
138 changes: 102 additions & 36 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,22 @@ mod impl_details {
}
}

/// The header of a ThinVec.
///
/// The _cap can be a bitfield, so use accessors to avoid trouble.
// The header of a ThinVec.
//
// The _cap can be a bitfield, so use accessors to avoid trouble.
//
// In "real" gecko-ffi mode, the empty singleton will be aligned
// to 8 by gecko. But in tests we have to provide the singleton
// ourselves, and Rust makes it hard to "just" align a static.
// To avoid messing around with a wrapper type around the
// singleton *just* for tests, we just force all headers to be
// aligned to 8 in this weird "zombie" gecko mode.
//
// This shouldn't affect runtime layout (padding), but it will
// result in us asking the allocator to needlessly overalign
// non-empty ThinVecs containing align < 8 types in
// zombie-mode, but not in "real" geck-ffi mode. Minor.
#[cfg_attr(all(feature = "gecko-ffi", any(test, miri)), repr(align(8)))]
Comment on lines +250 to +265
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative to this conditional attribute would be to make the static be of an over-aligned wrapper around the actual Header type, like:

#[repr(C, align(8))]
struct EmptyHeader(Header);

#[cfg(any(not(feature = "gecko-ffi"), test, miri))]
static EMPTY_HEADER: EmptyHeader = EmptyHeader(Header { _len: 0, _cap: 0 });

#[cfg(all(feature = "gecko-ffi", not(test), not(miri)))]
extern "C" {
    #[link_name = "sEmptyTArrayHeader"]
    static EMPTY_HEADER: EmptyHeader;
}

This would allow us to treat the empty header as 8-byte aligned with all configurations, despite the actual Header struct not being 8-byte aligned, which might be useful even for non-gecko-ffi code to avoid the check for empty ThinVec<&T>. We'd just need to change the check of for the empty header being aligned to:

// The empty header is over-aligned using the `EmptyHeader` wrapper, 
// meaning that if the header is at least as aligned as T, then the 
// one-past-the-end of the empty singleton is an acceptable out-of-thin-air 
// data pointer, and we can remove the dangling special case.
let empty_header_is_aligned = align_of::<EmptyHeader>() >= align_of::<T>();

It might also be worth adding a test to explicitly assert std::align_of::<EmptyHeader>() >= std::align_of::<Header>(), but IIRC rust doesn't silently under-align types with #[repr(C, align(...))].

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I completely forgot that SizeType without gecko-ffi on 64-bit systems is already defined using usize, not u32, so there's actually not much benefit to doing it this way other than making the empty_header_is_aligned check consistent across configurations.

#[repr(C)]
struct Header {
_len: SizeType,
Expand All @@ -264,23 +277,6 @@ impl Header {
fn set_len(&mut self, len: usize) {
self._len = assert_size(len);
}

fn data<T>(&self) -> *mut T {
let header_size = mem::size_of::<Header>();
let padding = padding::<T>();

let ptr = self as *const Header as *mut Header as *mut u8;

unsafe {
if padding > 0 && self.cap() == 0 {
// The empty header isn't well-aligned, just make an aligned one up
NonNull::dangling().as_ptr()
} else {
// This could technically result in overflow, but padding would have to be absurdly large for this to occur.
ptr.add(header_size + padding) as *mut T
}
}
}
}

#[cfg(feature = "gecko-ffi")]
Expand Down Expand Up @@ -321,10 +317,10 @@ impl Header {
/// optimize everything to not do that (basically, make ptr == len and branch
/// on size == 0 in every method), but it's a bunch of work for something that
/// doesn't matter much.
#[cfg(any(not(feature = "gecko-ffi"), test))]
#[cfg(any(not(feature = "gecko-ffi"), test, miri))]
static EMPTY_HEADER: Header = Header { _len: 0, _cap: 0 };

#[cfg(all(feature = "gecko-ffi", not(test)))]
#[cfg(all(feature = "gecko-ffi", not(test), not(miri)))]
extern "C" {
#[link_name = "sEmptyTArrayHeader"]
static EMPTY_HEADER: Header;
Expand Down Expand Up @@ -442,17 +438,24 @@ macro_rules! thin_vec {

impl<T> ThinVec<T> {
pub fn new() -> ThinVec<T> {
unsafe {
ThinVec {
ptr: NonNull::new_unchecked(&EMPTY_HEADER as *const Header as *mut Header),
boo: PhantomData,
}
}
ThinVec::with_capacity(0)
}

pub fn with_capacity(cap: usize) -> ThinVec<T> {
// `padding` contains ~static assertions against types that are
// incompatible with the current feature flags. We also call it to
// invoke these assertions when getting a pointer to the `ThinVec`
// contents, but since we also get a pointer to the contents in the
// `Drop` impl, trippng an assertion along that code path causes a
// double panic. We duplicate the assertion here so that it is
// testable,
let _ = padding::<T>();

if cap == 0 {
ThinVec::new()
ThinVec {
ptr: NonNull::new_unchecked(&EMPTY_HEADER as *const Header as *mut Header),
boo: PhantomData,
}
Gankra marked this conversation as resolved.
Show resolved Hide resolved
} else {
ThinVec {
ptr: header_with_capacity::<T>(cap),
Expand All @@ -470,7 +473,47 @@ impl<T> ThinVec<T> {
unsafe { self.ptr.as_ref() }
}
fn data_raw(&self) -> *mut T {
self.header().data()
Copy link
Owner

Choose a reason for hiding this comment

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

👍 this being wrong makes sense to me.

// `padding` contains ~static assertions against types that are
// incompatible with the current feature flags. Even if we don't
// care about its result, we should always call it before getting
// a data pointer to guard against invalid types!
let padding = padding::<T>();

// Although we ensure the data array is aligned when we allocate,
// we can't do that with the empty singleton. So when it might not
// be properly aligned, we substitute in the NonNull::dangling
// which *is* aligned.
//
// To minimize dynamic branches on `cap` for all accesses
// to the data, we include this guard which should only involve
// compile-time constants. Ideally this should result in the branch
// only be included for types with excessive alignment.
let empty_header_is_aligned = if cfg!(feature = "gecko-ffi") {
// in gecko-ffi mode `padding` will ensure this under
// the assumption that the header has size 8 and the
// static empty singleton is aligned to 8.
true
} else {
// In non-gecko-ffi mode, the empty singleton is just
// naturally aligned to the Header. If the Header is at
// least as aligned as T *and* the padding would have
// been 0, then one-past-the-end of the empty singleton
// *is* a valid data pointer and we can remove the
// `dangling` special case.
mem::align_of::<Header>() >= mem::align_of::<T>() && padding == 0
};

unsafe {
if !empty_header_is_aligned && self.header().cap() == 0 {
NonNull::dangling().as_ptr()
} else {
// This could technically result in overflow, but padding
// would have to be absurdly large for this to occur.
let header_size = mem::size_of::<Header>();
let ptr = self.ptr.as_ptr() as *mut u8;
ptr.add(header_size + padding) as *mut T
}
}
}

// This is unsafe when the header is EMPTY_HEADER.
Expand Down Expand Up @@ -565,7 +608,7 @@ impl<T> ThinVec<T> {
// doesn't re-drop the just-failed value.
let new_len = self.len() - 1;
self.set_len(new_len);
ptr::drop_in_place(self.get_unchecked_mut(new_len));
ptr::drop_in_place(self.data_raw().add(new_len));
Copy link
Owner

Choose a reason for hiding this comment

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

👍 this being wrong makes sense to me

}
}
}
Expand Down Expand Up @@ -915,7 +958,7 @@ impl<T> ThinVec<T> {
(*ptr).set_cap(new_cap);
self.ptr = NonNull::new_unchecked(ptr);
} else {
let mut new_header = header_with_capacity::<T>(new_cap);
let new_header = header_with_capacity::<T>(new_cap);

// If we get here and have a non-zero len, then we must be handling
// a gecko auto array, and we have items in a stack buffer. We shouldn't
Expand All @@ -931,8 +974,9 @@ impl<T> ThinVec<T> {
let len = self.len();
if cfg!(feature = "gecko-ffi") && len > 0 {
new_header
.as_mut()
.data::<T>()
.as_ptr()
.add(1)
.cast::<T>()
Copy link
Owner

Choose a reason for hiding this comment

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

👍 to this change for provenance

.copy_from_nonoverlapping(self.data_raw(), len);
self.set_len(0);
}
Expand Down Expand Up @@ -1373,6 +1417,28 @@ mod tests {
ThinVec::<u8>::new();
}

#[test]
fn test_data_ptr_alignment() {
let v = ThinVec::<u16>::new();
assert!(v.data_raw() as usize % 2 == 0);

let v = ThinVec::<u32>::new();
assert!(v.data_raw() as usize % 4 == 0);

let v = ThinVec::<u64>::new();
assert!(v.data_raw() as usize % 8 == 0);
}

#[test]
#[cfg_attr(feature = "gecko-ffi", should_panic)]
fn test_overaligned_type_is_rejected_for_gecko_ffi_mode() {
#[repr(align(16))]
struct Align16(u8);

let v = ThinVec::<Align16>::new();
assert!(v.data_raw() as usize % 16 == 0);
}

#[test]
fn test_partial_eq() {
assert_eq!(thin_vec![0], thin_vec![0]);
Expand Down Expand Up @@ -2654,9 +2720,9 @@ mod std_tests {
macro_rules! assert_aligned_head_ptr {
($typename:ty) => {{
let v: ThinVec<$typename> = ThinVec::with_capacity(1 /* ensure allocation */);
let head_ptr: *mut $typename = v.header().data::<$typename>();
let head_ptr: *mut $typename = v.data_raw();
assert_eq!(
head_ptr.align_offset(std::mem::align_of::<$typename>()),
Copy link
Owner

Choose a reason for hiding this comment

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

god i just looked this up and what a nightmare api

👍 to the change

head_ptr as usize % std::mem::align_of::<$typename>(),
0,
"expected Header::data<{}> to be aligned",
stringify!($typename)
Expand Down