-
Notifications
You must be signed in to change notification settings - Fork 25
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
Fix provenance UB and alignment UB #27
Conversation
A &Header cannot be used to get a useful pointer to data beyond it, because the pointer from the as-cast of the &Header only has provenance over the Header. After a set_len call that decreases the length, it is invalid to create a slice then try to get_unchecked into the region between the old and new length, because the reference in the slice that the ThinVec now Derefs to does not have provenance over that region. Alternatively, this is UB because the docs stipulate that you're not allowed to use `get_unchecked` to index out of bounds. Misaligned data pointers were produced when the gecko-ffi feature was enabled and T has an alignment greater than 4. I think the use of align_offset in tests is subtly wrong, align_offset seems to be for optimizations only. The docs say that a valid implementation may always return usize::MAX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks great, thanks! (I'm well acquainted with stacked borrows, but this code was written quite a while ago when everything was much more up in the air).
I've been trying to find where an unaligned access can happen, and how any of these changes would possibly change that. Could your provide some more details?
@@ -470,7 +453,18 @@ impl<T> ThinVec<T> { | |||
unsafe { self.ptr.as_ref() } | |||
} | |||
fn data_raw(&self) -> *mut T { | |||
self.header().data() |
There was a problem hiding this comment.
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.
@@ -565,7 +559,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)); |
There was a problem hiding this comment.
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
assert_eq!( | ||
head_ptr.align_offset(std::mem::align_of::<$typename>()), |
There was a problem hiding this comment.
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
.data::<T>() | ||
.as_ptr() | ||
.add(1) | ||
.cast::<T>() |
There was a problem hiding this comment.
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
Yeah totally. You can probably find the extant alignment issue on the latest release or commit to your branch by running Miri with Stacked Borrows off and
You can definitely detect it with
The symbolic alignment check is known to have false positives, but the normal alignment checker checks the actual value of a pointer, so it's possible to have a false negative because things just happen to be correctly aligned. So this is a Heisenbug. In a lot of cases if I try to stick in a Extra debug assertions to the rescue! If I apply only this diff to fix the indexing out of bounds problem but retain the alignment issue: diff --git a/src/lib.rs b/src/lib.rs
index 19e38b8..a5e3928 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -565,7 +565,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));
}
}
} Then running under gdb, we get a SIGILL from the assertion failure in
And looking at what we're building a slice out of...
I think the bug actually might originate in I think this patch sidesteps the issue by not considering |
Ah, I bet I know the issue. It doesn't affect real geck-ffi code but it would affect miri/tests which are in a weird zombie configuration. But very importantly: I think this would affect the actual production non-gecko-ffi when First let's look at gecko-ffi: When really running under gecko-ffi, we're dynamically linking the "empty singleton" to the following symbol defined in Firefox's C++: alignas(8) const nsTArrayHeader sEmptyTArrayHeader = {0, 0, 0}; But we include no such #[repr(C)]
struct Header {
_len: SizeType,
_cap: SizeType,
} The reason this blows up in impl<T> Drop for ThinVec<T> {
fn drop(&mut self) {
unsafe {
ptr::drop_in_place(&mut self[..]);
self.deallocate();
}
}
} Which eventually shells out to The problem is that When we allocate we use the With all of that said, you can probably see why non-gecko-ffi would also have this issue when You "fixed" this when you removed the The important safety guard is: calling This assertion helps keep us confident that you aren't accidentally creating/handling ThinVecs that aren't compatible with nsTArray's design, even if FFI hides the ctor from us. Conveniently it's using entirely static values so this guard has no runtime effects when you don't use invalid types. I would also like to restore some version of that guard because it also serves an important purpose: it removes the runtime branch on Here's my proposed tweaks (with extra docs so we all remember The Reasoning): // 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)))]
#[repr(C)]
struct Header {
_len: SizeType,
_cap: SizeType,
} (Really we should wrap the empty singleton static in a struct that is aligned to 8 but I don't want to write all that out just for tests..) fn data_raw(&self) -> *mut T {
// `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* an acceptable out-of-thin-air data pointer and
// we can remove the `dangling` special case.
align_of::<Header>() >= 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
}
}
} It would be good to also include a test for the |
And also explain the nuanced explanation in detail, and try to back up the implementation with tests that ensure that the data pointer for a zero-length ThinVec is correct, in and out of gecko-ffi mode. Co-authored-by: Aria Beingessner <[email protected]>
When I turn on |
Ha, funny. My bad. The SIGILL is a double-panic from the over-align assertion. Basically with the way I recommended, when you try to access an empty invalid ThinVec, it panics, which tries to run its destructor and get the data pointer, and then panics again. I think the simplest fix is to add
To the start of |
// 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)))] |
There was a problem hiding this comment.
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(...))]
.
There was a problem hiding this comment.
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.
Only asserting alignment in data_raw is a bit too late, because the check will occur for an always-empty ThinVec in the destructor, which will cause a double-panic and a SIGILL. Duplicating it in the constructors prevents creating an invalid ThinVec in the first place. The check in data_raw is retained as a prevention against using an invalid ThinVec through FFI.
Oops, looks like you left out an unsafe block! |
Thanks so much for finding this stuff and helping fix it! We can do the Right Thing with the wrapper that nika describes in a follow-up without bothering you with this anymore. |
It's not bothersome, I'm out here because I'm trying to fix things like this 😄 |
Oh also in case you didn't notice I updated your commit message with some extra notes. |
I've been running
MIRIFLAGS="-Zmiri-tag-raw-pointers" cargo miri test
on a number of crates, and I recently came across this one. Here's what I found:&Header
cannot be used to get a useful pointer to data beyond it, because the pointer from the as-cast of the&Header
only has provenance over theHeader
.set_len
call that decreases the length, it is invalid to create a slice then try toget_unchecked
into the region between the old and new length, because the reference in the slice that theThinVec
nowDeref
s to does not have provenance over that region. Alternatively, this is UB because the docs stipulate that you're not allowed to useget_unchecked
to index out of bounds.gecko-ffi
feature was enabled andT
has an alignment greater than 4.align_offset
in tests is subtly wrong,align_offset
seems to be for optimizations only. The docs say that a valid implementation may always returnusize::MAX
.FYI, the provenance issue here can be tricky to understand from Miri's output but I have a Miri PR up that improves the diagnostics dramatically for cases like this. Also, I'm told this code might be shipped as part of a web browser? If an interested party wants to see if these issues affect a codebase that doesn't work under Miri I have a rustc PR up which detects the alignment and
get_unchecked
issue when code is built with-Zbuild-std
.