rust
rust copied to clipboard
#95295 causes unsoundness in multiple existing crates
I was recently looking through the draft release notes when I noticed #95295. While it makes sense for Layout::from_size_align() to restrict allocations to isize::MAX bytes, this restriction was also added to Layout::from_size_align_unchecked(), which is a public and widely used API. Several crates were sound under the previous overflow property, usually panicking or returning an error after checking the Layout against isize::MAX. However, these have become unsound under the new overflow property, since just constructing the overlarge Layout is now UB. Also, some crates created overlarge layouts for the sole purpose of feeding them into handle_alloc_error(). To list the instances I've found:
- The provided
GlobalAlloc::realloc()impl incore:use std::alloc::{GlobalAlloc, Layout, System}; struct Alloc; // SAFETY: Wraps `System`'s methods. unsafe impl GlobalAlloc for Alloc { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { System.alloc(layout) } unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { System.dealloc(ptr, layout) } } let alloc = Alloc; let ptr = unsafe { alloc.alloc(Layout::new::<u8>()) }; assert!(!ptr.is_null()); // SAFETY: // - `ptr` is currently allocated from `alloc`. // - The layout is the same layout used to allocate `ptr`. // - The new size is greater than zero. // - The new size, rounded up to the alignment, is less than `usize::MAX`. unsafe { alloc.realloc(ptr, Layout::new::<u8>(), isize::MAX as usize + 1) }; // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at <Alloc as core::alloc::GlobalAlloc>::realloc() hashbrownv0.12.3:// features = ["raw"] use hashbrown::raw::RawTable; assert!(cfg!(target_feature = "sse2")); RawTable::<u8>::with_capacity(usize::MAX / 64 * 7 + 8); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 17, 16) // at hashbrown::raw::TableLayout::calculate_layout_for()bumpalov3.11.0:// debug-assertions = false use bumpalo::Bump; Bump::try_with_capacity(isize::MAX as usize + 1).unwrap_err(); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at bumpalo::layout_from_size_align() // at bumpalo::Bump::try_with_capacity()rusqlitev0.28.0 (admittedly contrived):// --target i686-unknown-linux-gnu // features = ["bundled", "vtab"] use rusqlite::{ ffi, vtab::{ self, sqlite3_vtab, sqlite3_vtab_cursor, Context, IndexInfo, VTab, VTabConnection, VTabCursor, Values, }, Connection, }; use std::os::raw::c_int; #[repr(C)] struct DummyTab { base: sqlite3_vtab } // SAFETY: `DummyTab` is `repr(C)` and starts with a `sqlite3_vtab`. unsafe impl<'vtab> VTab<'vtab> for DummyTab { type Aux = (); type Cursor = DummyCursor; fn connect( _: &mut VTabConnection, _: Option<&Self::Aux>, _: &[&[u8]], ) -> rusqlite::Result<(String, Self)> { let s = String::from_utf8(vec![b'\x01'; isize::MAX as usize]).unwrap(); Err(rusqlite::Error::SqliteFailure(ffi::Error::new(0), Some(s))) } fn best_index(&self, _: &mut IndexInfo) -> rusqlite::Result<()> { unimplemented!() } fn open(&'vtab mut self) -> rusqlite::Result<Self::Cursor> { unimplemented!() } } #[repr(C)] struct DummyCursor { base: sqlite3_vtab_cursor } // SAFETY: `DummyCursor` is `repr(C)` and starts with a `sqlite3_vtab_cursor`. unsafe impl VTabCursor for DummyCursor { fn filter(&mut self, _: c_int, _: Option<&str>, _: &Values<'_>) -> rusqlite::Result<()> { unimplemented!() } fn next(&mut self) -> rusqlite::Result<()> { unimplemented!() } fn eof(&self) -> bool { unimplemented!() } fn column(&self, _: &mut Context, _: c_int) -> rusqlite::Result<()> { unimplemented!() } fn rowid(&self) -> rusqlite::Result<i64> { unimplemented!() } } let conn = Connection::open_in_memory().unwrap(); let module = vtab::eponymous_only_module::<DummyTab>(); conn.create_module("dummy", module, None).unwrap(); conn.execute("SELECT * FROM dummy", ()).unwrap(); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at rusqlite::util::sqlite_string::SqliteMallocString::from_str()bevy_ecsv0.8.1:// --target i686-unknown-linux-gnu use bevy_ecs::component::{Component, Components}; #[derive(Component)] #[component(storage = "SparseSet")] struct Data([u8; usize::MAX / 128 + 1]); Components::default().init_component::<Data>(&mut Default::default()); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at bevy_ecs::storage::blob_vec::repeat_layout() // at bevy_ecs::storage::blob_vec::array_layout() // at bevy_ecs::storage::blob_vec::BlobVec::grow_exact()allocator_apiv0.6.0:use allocator_api::RawVec; let mut raw_vec: RawVec<u8> = RawVec::new(); raw_vec.reserve(0, isize::MAX as usize + 1); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at <core::alloc::Layout as allocator_api::libcore::alloc::LayoutExt>::repeat() // at <core::alloc::Layout as allocator_api::libcore::alloc::LayoutExt>::array::<u8>() // at allocator_api::liballoc::raw_vec::RawVec::<u8, allocator_api::global::Global>::reserve_internal()capv0.1.1:use cap::Cap; use std::alloc::{GlobalAlloc, Layout, System}; let alloc = Cap::new(System, usize::MAX); // SAFETY: The layout has non-zero size. let ptr = unsafe { alloc.alloc(Layout::new::<u8>()) }; assert!(!ptr.is_null()); // SAFETY: // - `ptr` is currently allocated from `alloc`. // - The layout is the same layout used to allocate `ptr`. // - The new size is greater than zero. // - The new size, rounded up to the alignment, is less than `usize::MAX`. unsafe { alloc.realloc(ptr, Layout::new::<u8>(), isize::MAX as usize + 1) }; // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at <cap::Cap<std::alloc::System> as core::alloc::GlobalAlloc>::realloc()scoped_arenav0.4.1:use scoped_arena::Scope; Scope::new().to_scope_many::<u8>(0, 0); // calls Layout::from_size_align_unchecked(usize::MAX - 1, 1) // at scoped_arena::Scope::<'_, scoped_arena::allocator_api::Global>::to_scope_many::<u8>()
Also, many more crates were sound under the condition that alloc::alloc() always fails on allocations larger than isize::MAX bytes, but likely unsound if it were to successfully return an allocated pointer. Before #95295, they would either panic, return an error, or call handle_alloc_error() from alloc() failing to satisfy the overlarge request. Many of these crates have now become unconditionally unsound after the change.
Now-unsound crates that depended on overlarge alloc() failing
semverv1.0.14:// --target i686-unknown-linux-gnu let mut s = String::from_utf8(vec![b'0'; isize::MAX as usize - 4]).unwrap(); s.replace_range(..1, "1"); s.parse::<Prerelease>().unwrap(); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at semver::identifier::Identifier::new_unchecked()async-taskv4.3.0:// --target i686-unknown-linux-gnu use std::{future, mem, task::Waker}; const SIZE: usize = isize::MAX as usize - mem::size_of::<Option<Waker>>() - 10; let _ = async_task::spawn(future::pending::<[u8; SIZE]>(), |_| {}); // calls Layout::from_size_align_unchecked(isize::MAX as usize - 2, 4) // at async_task::utils::Layout::into_std() // at async_task::raw::RawTask::<core::future::Pending<[u8; {_}]>, [u8; {_}], {closure}>::eval_task_layout()zerocopyv0.6.1:// features = ["alloc"] use zerocopy::FromBytes; u8::new_box_slice_zeroed(isize::MAX as usize + 1); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at <u8 as zerocopy::FromBytes>::new_box_slice_zeroed()memsecv0.6.2:// --target x86_64-unknown-linux-gnu use libc::_SC_PAGESIZE; // SAFETY: `_SC_PAGESIZE` is a valid `sysconf` argument. let page_size = unsafe { libc::sysconf(_SC_PAGESIZE) as usize }; assert!(page_size != usize::MAX); let size = isize::MAX as usize - page_size * 5 - 13; // SAFETY: No preconditions. unsafe { memsec::malloc_sized(size) }; // calls Layout::from_size_align_unchecked(isize::MAX as usize - page_size + 1, page_size) // at memsec::alloc::raw_alloc::alloc_aligned()lassov0.6.0:// debug-assertions = false use lasso::{Capacity, Rodeo}; let bytes = (isize::MAX as usize + 1).try_into().unwrap(); let _: Rodeo = Rodeo::with_capacity(Capacity::new(0, bytes)); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at lasso::arena::Bucket::with_capacity()thin-dstv1.1.0:use thin_dst::ThinBox; struct DummyIter; impl Iterator for DummyIter { type Item = u8; fn next(&mut self) -> Option<Self::Item> { unimplemented!() } } impl ExactSizeIterator for DummyIter { fn len(&self) -> usize { isize::MAX as usize + 1 } } ThinBox::new((), DummyIter); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at thin_dst::polyfill::alloc_layout_extra::repeat_layout() // at thin_dst::polyfill::alloc_layout_extra::layout_array::<u8>() // at thin_dst::ThinBox::<(), u8>::layout()lightprocv0.3.5:// --target i686-unknown-linux-gnu use lightproc::lightproc::LightProc; use std::{ future::Future, pin::Pin, task::{Context, Poll}, }; #[repr(align(4))] struct Dummy; impl Future for Dummy { type Output = [u8; isize::MAX as usize - 2]; fn poll(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<Self::Output> { unimplemented!() } } LightProc::build(Dummy, |_| {}, Default::default()); // calls Layout::from_size_align_unchecked(isize::MAX as usize - 2, 4) // at lightproc::raw_proc::RawProc::<Dummy, [u8; {_}], {closure}>::proc_layout()thin-vecv0.2.8:// --target x86_64-unknown-linux-gnu use thin_vec::ThinVec; ThinVec::<u8>::with_capacity(isize::MAX as usize - 21); // calls Layout::from_size_align_unchecked(isize::MAX as usize - 6, 8) // at thin_vec::layout::<u8>() // at thin_vec::header_with_capacity::<u8>()bsn1v0.4.0:// --target i686-unknown-linux-gnu use bsn1::{Der, IdRef}; struct Iter<'a>(Option<&'a [u8]>); impl Clone for Iter<'_> { fn clone(&self) -> Self { Self(Some(&[0; 7])) } } impl<'a> Iterator for Iter<'a> { type Item = &'a [u8]; fn next(&mut self) -> Option<Self::Item> { self.0.take() } } let vec = vec![0; isize::MAX as usize - 1]; Der::from_id_iterator(IdRef::eoc(), Iter(Some(&vec))); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at bsn1::buffer::Buffer::reserve()seckeyv0.11.2:// default-features = false // features = ["use_std"] use seckey::SecBytes; SecBytes::new(isize::MAX as usize + 1); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at seckey::bytes::alloc::malloc_sized()slice-dstv1.5.1:use slice_dst::SliceDst; <[u8]>::layout_for(isize::MAX as usize + 1); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, 1) // at slice_dst::layout_polyfill::repeat_layout() // at slice_dst::layout_polyfill::layout_array::<u8>() // at <[u8] as slice_dst::SliceDst>::layout_for()stable-vecv0.4.0:use stable_vec::ExternStableVec; ExternStableVec::<u16>::with_capacity(usize::MAX / 4 + 1); // calls Layout::from_size_align_unchecked(isize::MAX as usize, 2) // at <stable_vec::core::bitvec::BitVecCore<u16> as stable_vec::core::Core<u16>>::realloc()rayon-hashv0.5.0:use rayon_hash::HashMap; use std::mem; let size = (usize::MAX / mem::size_of::<usize>() / 2 * 5 + 25) / 11; HashMap::<(), ()>::with_capacity(size); // calls Layout::from_size_align_unchecked(isize::MAX as usize + 1, size_of::<usize>()) // at rayon_hash::alloc::Layout::from_size_align_unchecked() // at rayon_hash::alloc::Layout::repeat() // at rayon_hash::alloc::Layout::array::<rayon_hash::std_hash::table::HashUint>() // at rayon_hash::std_hash::table::calculate_layout::<(), ()>()
cc @rust-lang/libs-api
This will hit stable in ~5-6 days (next Thursday), so would be great to get some eyes on quickly. If possible, I think ideally we'd back it out of the upcoming release (cc @pietroalbini).
FWIW, even though one of my crates was impacted, I think this is plausibly fine? I at least think that it was known to be an issue in advance. (Certainly the discussion in https://github.com/rust-lang/rust/pull/95295 seems that way).
Concretely, the amount of unsoundness this produces is probably less than the amount of unsoundness that the change allows us to avoid.
Concretely, the amount of unsoundness this produces is probably less than the amount of unsoundness that the change allows us to avoid.
I looked through the issue; the change seems to be mainly to make the job of Layout consumers such as alloc() somewhat simpler. But are there any underlying alloc() implementations in practice that can successfully return more than isize::MAX bytes? Certainly, it would be possible to create one on top of, e.g., mmap() on Linux, but libcs generally refrain from actually allowing it, due to the footguns that result even in C. Alternatively, are there soundness concerns beyond (the lack of) handling of overlarge allocations?
Adding the regression label to make sure I have eyes on it during the release process.
As I mentioned on Zulip, I think we should revert #95295 for now so we have time to consider this issue before stably adding the isize::MAX promise.
Backed the PRs responsible for this (https://github.com/rust-lang/rust/pull/95295 and https://github.com/rust-lang/rust/pull/99136) off the 1.64.0 release, cc @CAD97. Those are still present in beta 1.65 and nightly 1.66 for now, unless someone reverts them from there (won't have time myself unfortunately).
But are there any underlying
alloc()implementations in practice that can successfully return more thanisize::MAXbytes?
On 64-bit, no, but yes on 32-bit. Any crate relying on such an allocation failing is unsound. The std bug which prompted making this change was with Rc allowing creating an allocation of over isize::MAX bytes succeeding and creating unsoundness due to that.
Backing out for this release is prudent. For later releases, it might be a reasonable state to treat a size > isize::MAX as library UB but not immediate language UB; IIUC this would result in any uses of _unchecked remaining exactly as (un)sound as previously.
cc @scottmcm
removing prioritization label since it looks it's waiting on T-libs (Zulip discussion).
@estebank do you think we can remove also T-compiler or better keep them in the loop?
@rustbot label -I-prioritize
On 64-bit, no, but yes on 32-bit. Any crate relying on such an allocation failing is unsound. The std bug which prompted making this change was with
Rcallowing creating an allocation of overisize::MAXbytes succeeding and creating unsoundness due to that.
Are you talking about #95334? That seems more like a Miri oddity than an issue in practice. It successfully allocates more than isize::MAX bytes when emulating 32-bit targets, even when the actual targets' libcs unconditionally reject it:
use std::alloc::{self, Layout};
let layout = Layout::from_size_align(isize::MAX as usize + 1, 1).unwrap();
// SAFETY: The layout has a non-zero size.
let ptr = unsafe { alloc::alloc(layout) };
println!("{ptr:p}");
if !ptr.is_null() {
// SAFETY: `ptr` points to allocated memory.
unsafe { alloc::dealloc(ptr, layout) };
}
// cargo +1.63.0 run --target i686-unknown-linux-gnu: 0x0
// cargo +1.63.0 run --target i686-unknown-linux-musl: 0x0
// cargo +nightly-2022-06-10 miri run --target i686-unknown-linux-gnu: 0x26260
// cargo +nightly-2022-06-10 miri run --target i686-unknown-linux-musl: 0x25de9
What I'm asking is, should allocators on 32-bit targets actually be capable of fulfilling a request for more than isize::MAX/PTRDIFF_MAX bytes? Because I'd argue that if a general-purpose allocator has a C/C++ interface (like most), and if it allows overlarge allocations, then the allocator is broken. Neither C/C++ compilers nor libraries are equipped to soundly handle objects that large; "A non-exhaustive list of ways C compilers break for objects larger than PTRDIFF_MAX bytes" was published back in 2016.
But I concede that some allocators might be silly enough to allow it anyway, perhaps as a holdover from the 16-bit era. Looking at our 32-bit targets with std support:
- *-*-windows-*: By default, 32-bit applications on Windows are restricted to the lower 2 GB of the address space. This can be extended to the full 4 GB on 64-bit Windows, using the linker flag
/LARGEADDRESSAWAREon MSVC or--large-address-awareon MinGW. However, the largest hole that can be safely filled withVirtualAllocempirically seems to be only0x7fe5e000bytes, at least with the UCRT loaded. - *-unknown-linux-gnu*: glibc has enforced a
0x7ffffffflimit since 2.30 (Aug. 2019). - *-linux-android*: Bionic supports two implementations. scudo has enforced a
0x7ffffffflimit since LLVM 4.0.0 (Mar. 2017), when it first added 32-bit support. jemalloc has enforced a0x70000000limit since 4.1.0 (Feb. 2016). - *-unknown-linux-musl*: musl has enforced a
0x7ffffffflimit since 0.7.0 (Mar. 2011). - *-unknown-freebsd: jemalloc has enforced a
0x70000000limit since 4.1.0 (Feb. 2016), included in FreeBSD 11.0 (Oct. 2016). - i686-unknown-haiku: libroot's hoard2 has enforced a
0x3f60e740limit since it was added to the current source control in 2002. - riscv32imc-esp-espidf: ESP-IDF enforces a limit of
SOC_MAX_CONTIGUOUS_RAM_SIZE, which can be up to0x02000000on the ESP32-S3. - *-unknown-emscripten: Emscripten has two implementations. Its dlmalloc fork only enforces a
0xffffffbflimit. emmalloc only enforces a0xffffffc7limit. - wasm32-unknown-unknown: dlmalloc-rs only enforces a
0xfffeffcclimit. - wasm32-wasi: wasi-libc's dlmalloc fork only enforces a
0xffffffbflimit. - *-unknown-linux-uclibc*: uClibc-ng has three implementations. "malloc" (the non-MMU default) only appears to enforce a
0xfffffff0limit. "malloc-simple" has enforced a0x7ffffffflimit since 1.0.37 (Dec. 2020). "malloc-standard" (the MMU default, based on dlmalloc) only enforces a0xffffffdflimit. - *-unknown-netbsd*: By default, NetBSD libc uses jemalloc, which has enforced a
0x70000000limit since 4.1.0 (Feb. 2016), included in NetBSD 9.0 (Feb. 2020). It can be configured to use a far older phk-malloc fork, which only enforces a limit of0xffffffff - getpagesize(). - i686-unknown-openbsd: OpenBSD libc only enforces a
0xffffeffelimit. - *-apple-ios, i686-apple-darwin: libmalloc only enforces an "absolute max size" of
0xffffffff - vm_page_size * 2. But the various "zones" may enforce stricter limits; I'm not familiar enough with Apple code to follow the dynamic dispatch. - *-apple-watchos: The watchOS runtime libraries do not appear to be publicly available.
- armv7a-kmc-solid_asp3-eabi*: The SOLID-OS runtime libraries do not appear to be publicly available.
Some of the offending targets may have dynamic linkers that preclude sufficiently large blocks, as with Windows; I've only tested the Windows, Linux, and WASM targets directly. The main problematic allocators seem to be dlmalloc forks, mostly used in WASM, and phk-malloc forks, used in the BSDs.
Overall, all of the largest desktop, server, and mobile platforms either have their own PTRDIFF_MAX enforcement, delegate to a version of jemalloc new enough to enforce it, or have a virtual-memory layout that precludes it entirely. (It appears that overlarge blocks cannot even be directly mapped on Android, as a security measure.) The main problems occur in the BSDs, the big WASM environments, uClibc platforms, and potentially Apple systems, although I doubt that Apple would leave a hole like this open.
So, while there's a long tail of platforms that fulfill allocation requests larger than PTRDIFF_MAX, there's a far longer tail of libraries entirely unequipped to handle objects that large. Even if we were to restrict all Layout producers to isize::MAX, there will still be a myriad "safe" C bindings to libraries that can internally generate overlarge requests and cause UB if they succeed. I simply don't see how this situation can improve without adding restrictions to Layout consumers.
-unknown-linux-gnu: glibc has enforced a 0x7fffffff limit since 2.30 (Aug. 2019).
Rust's minimum supported glibc version for i686-unknown-linux-gnu is 2.17.
Rust's minimum supported glibc version for i686-unknown-linux-gnu is 2.17.
Indeed. However, there's not much we can do about that, given the state of the C ecosystem. To fix this, either the immovable object must be moved (updating the libc or patching its allocator), or the unstoppable force must be stopped (fixing every C library that makes the PTRDIFF_MAX assumption, and preventing them from becoming broken again). Looking at glibc in particular, it looks like the big distros will be supporting pre-2.30 versions to some extent until 2030 or so.
The OP does not link to the source locations instantiating those Layouts. Some context why crates are doing that and whether we need to provide alternatives would help.
@estebank do you think we can remove also T-compiler or better keep them in the loop?
Better to leave it. The team can remove it ourselves on the next t-compiler meeting if we collectively decide not to track this as closely (and rely on t-libs notifying us if we need to revisit).
Thanks for the CC, @CAD97. I guess it's good that my other change about making this a niche never happened 🙃
For later releases, it might be a reasonable state to treat a size >
isize::MAXas library UB but not immediate language UB
This is the case in the current nightly code, correct? This is just a statement to not also do #99134?
The OP does not link to the source locations instantiating those
Layouts. Some context why crates are doing that and whether we need to provide alternatives would help.
Sorry, I added the paths to the functions at the bottom of each snippet, but I probably should have made it clearer.
Of the crates that were unconditionally unsound:
GlobalAlloc::realloc(): The overflow condition forGlobalAlloc::realloc()used to match the overflow condition forLayout::from_size_align_unchecked(), but the latter has been made stricter.semverv1.0.14:Identifier::new_unchecked()can request an allocation of up toisize::MAX as usize + 5bytes (with alignment 2), given a string ofisize::MAXbytes. However,Identifiernever uses pointer arithmetic to reach the end of the allocation, nor does it produce any slices larger than the original string. The allocation request is no longer allowed.hashbrownv0.12.3:TableLayout::calculate_layout_for()uses checked arithmetic to protect against overflow, which is no longer sufficient. It later checks againstisize::MAX.rusqlitev0.28.0:SqliteMallocString::from_str()uses checked arithmetic to protect against overflow. It passes the size throughc_int::try_from(), which prevents sizes larger thanisize::MAXfrom being allocated. However, if the checked arithmetic or allocation fails, it usesLayout::from_size_align_unchecked()with alignment 1 to create a layout forhandle_alloc_error(). Ifsis exactlyisize::MAXbytes long, this fails the newLayoutcondition.allocator_apiv0.6.0: ALayout::array()implementation was added when the method was still unstable. It uses the old internal definition ofLayout::repeat(), which is no longer sound. Otherwise, the crate performs the sameisize::MAXchecks thatcoreandallocperform.pyembedv0.22.0: Since Python 2.7, CPython has never allowed any requests larger thanisize::MAXto be issued to the underlying allocator. However,pyalloc::rust_malloc()is no longer sound, since the size can overflowisize::MAXwhen rounded up toMIN_ALIGN.capv0.1.1:<Cap<_> as GlobalAlloc>::realloc()is affected by the sameGlobalAlloc::alloc()issue.scoped-arenav0.4.1:Scope::to_scope_from_iter(),Scope::to_scope_many(),Scope::to_scope_many_with(), and the correspondingScopeProxymethods all construct overlargeLayouts to conditionally pass tohandle_alloc_error(). They later use the safeLayoutmethods for the actual allocation.
Of the crates that were sound conditional on alloc::alloc() enforcing an isize::MAX bound:
bumpalov3.11.0:Bump::try_with_capacity()callsLayout::from_size_align_unchecked()with alignment 1, which is no longer universally safe.async-taskv4.3.0: A fullLayoutimplementation was added to enable constLayoutmanipulation. At the end,Layout::into_std()usesStdLayout::from_size_align_unchecked()to obtain a regularStdLayout. Its implementation ofLayout::extend()is no longer sound.zerocopyv0.6.1: The providedFromBytes::new_box_slice_zeroed()impl uses checked arithmetic to protect against overflow, which is no longer sufficient.memsecv0.6.2:raw_alloc::alloc_aligned()is called only byalloc::_malloc(). The latter performs an initial overflow check againstusize::MAX, which is no longer sufficient.bevy_ecsv0.8.1: ALayout::array()implementation was added when the method was still unstable. It uses the old internal definition ofLayout::repeat(), which is no longer sound.lassov0.6.0:Bucket::with_capacity()callsLayout::from_size_alignment_unchecked()with alignment 1, which is no longer universally safe.thin-dstv1.1.0: ALayout::array()implementation was added when the method was still unstable. It uses the old internal definition ofLayout::repeat(), which is no longer sound.lightprocv0.3.5:RawProc::proc_layout()usesLayout::from_size_align_unchecked()to construct the layout of a union of two types. This is no longer sound, since the size of one type can overflowisize::MAXwhen rounded up to the other type's alignment.thin-vecv0.2.8:thin_vec::layout()relies on several manual layout calculations, since theLayoutmanipulation methods were still unstable when it was modified to usealloc::alloc(). The calculations use checked arithmetic to protect against overflow, which is no longer sufficient.bsn1v0.4.0:Buffer::reserve()callsLayout::from_size_align_unchecked()with alignment 1 to create a layout forhandle_alloc_error(). This is no longer universally safe.seckeyv0.11.2:alloc::malloc_sized()is only called bySecBytes::with(); it callsLayout::from_size_alignment_unchecked()with alignment 1, which is no longer universally safe.slice-dstv1.5.1: ALayout::array()implementation was added when the method was still unstable. It uses the old internal definition ofLayout::repeat(), which is no longer sound.stable-vecv0.4.0:<BitVecCore<_> as Core<_>>::realloc()uses checked arithmetic to protect against overflow, which is no longer sufficient.
thin-vec v0.2.8: thin_vec::layout() relies on several manual layout calculations, since the Layout manipulation methods were still unstable when it was modified to use alloc::alloc(). The calculations use checked arithmetic to protect against overflow, which is no longer sufficient.
Even though I was initially in favor of the isize::MAX change, this (and the rest of the list, which is very similar) seems very convincing to me. It should be fine to do this, and rely on those requirements not changing. The cases I had looked at previously were my own and scoped arena, which were less convincing to me.
Looking at glibc in particular, it looks like the big distros will be supporting pre-2.30 versions to some extent until 2030 or so.
This compact overview might be helpful. Slowest mover being - unsurprisingly - the RHEL 8 flavours which will be stuck at glibc 2.28 until standard support ends 2029-05-31 (though obviously paid extensions of that support exist).
What I'm asking is, should allocators on 32-bit targets actually be capable of fulfilling a request for more than
isize::MAX/PTRDIFF_MAXbytes? Because I'd argue that if a general-purpose allocator has a C/C++ interface (like most), and if it allows overlarge allocations, then the allocator is broken. Neither C/C++ compilers nor libraries are equipped to soundly handle objects that large; "A non-exhaustive list of ways C compilers break for objects larger than PTRDIFF_MAX bytes" was published back in 2016.
That blog post shows an example of the bug on a 64-bit system, which is possible because of malloc elision by LLVM: the larger than isize::MAX allocation never reaches the allocator and the allocation appears to succeed. I believe this same optimization could be exploited in Rust to cause unsoundness (though I'm not 100% sure).
Protecting against this requires that the allocation size is checked before the __rust_alloc call: either in Layout itself (as done in #95295) or in the global allocator glue code in alloc.
thin-dst, slice-dst
...oops, I didn't realize I made my own crates potentially unsound... which definitely speaks to how subtle this can be.
I'll repeat what I've said previously: there are like 3½ solutions to the underlying issue:
- Status quo; either that
- Calling
allocwithsize > isize::MAXis unsound, or - Creating a reference to an allocated object with
size > isize::MAXis unsound;
- Calling
- Mandate that
Layoutwithsize > isize::MAXis disallowed; or - Mandate that
allocimplementations must fail when givensize > isize::MAX.
The status quo has proven untenable with std forgetting checks. There's no perfect solution here, but I think we can successfully do a bit of harm reduction. Roughly, I propose that the best solution may be:
- The safe
Layoutmethods enforcesize <= isize::MAX; Layout::from_size_align_uncheckedwithsize > isize::MAXis guaranteed not to be UB on its own;- std's
GlobalAllocimplementations guarantee that allocation withsize > isize::MAXfail; - the unstable
Allocatortrait mandates that allocators handle and return an error layouts withsize > isize::MAX; - the
GlobalAlloctrait nominally requires the caller to guarantee thatsize <= isize::MAXbut strongly recommends that implementers allow and always fail to allocate for such layouts; and - the
#[global_allocator]shim introduces a precheck forsize > isize::MAXto ensure that the free functions handlesize > isize::MAXeven with arbitraryGlobalAlloc.
However, I'm not sold on the idea that C compatible allocation must fail for size > isize::MAX or be broken in C; rather, in C "solution" to pointer math between "really far pointers" is simply not to do that.
And even if we can reasonably guarantee that any hosted target either refuses to serve an overlarge allocation or requires an easily-documented-as-unsound-flag, there's still freestanding targets to consider.
alloc returning an allocation of
> isize::MAXbytes
Here's an example of overlarge allocation succeeding: (on the playground on current stable 1.64) this fails in debug with the allocation failure but passes in release:
https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=1074f009331582a852ff32ea49ea1fc6
let layout = Layout::from_size_align(isize::MAX as usize + 1, 1).unwrap();
unsafe {
let ptr = alloc(layout);
if ptr.is_null() {
handle_alloc_error(layout);
}
dealloc(ptr, layout);
}
println!(
"successfully made 2^{} byte allocation",
layout.size().trailing_zeros()
);
assert_eq!(layout.size().count_ones(), 1);
assert_eq!(layout.size().leading_zeros(), 0);
... and here's a proof of concept in safe code where an allocation of usize::MAX bytes is observed and used to do a safe indexing offset of more than isize::MAX bytes:
https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=dec83b12bac018e257536f19c6622f7a
fn main() {
let offset = test();
println!("0x{offset:X}");
}
fn test() -> usize {
let mut v = Vec::<u8>::with_capacity(usize::MAX);
let slice = v.spare_capacity_mut();
let front = &slice[0] as *const _ as usize;
let back = &slice[usize::MAX - 1] as *const _ as usize;
return back - front;
}
This only "works" and prints 0xFFFFFFFFFFFFFFFE on current stable (1.64) as current beta (1.65) and nightly both have the change to Layout such that this is treated as a capacity error before hitting the elidable allocation. Actually "using" an overlong vector like this is very difficult as simple things can quickly cause the vector to actually allocate. (For example, anything with a potentially panicking edge will cause the allocation to manifest.) Also, as always with UB, the results can vary with changes that shouldn't seem to impact things.
Author of thin-vec here: I always knew the checks on thin-vec were incorrect, hence my comment to this effect: https://github.com/Gankra/thin-vec/blob/05127f96717b1478b4ce2fa2fcae7ac86fb741e7/src/lib.rs#L339
This is 100% me being lazy and just saying "eugh, this UB isn't worth the effort" because I knew that the vast majority of allocators prevent this but by the letter of the API I was doing something unsound (also the primary deployment of this library turns on some flags that introduce more aggressive limits for backcompat with firefox anyway).
(edit: when nnethercote was first asking me about integrating thin-vec into rustc this line was like the first thing i pointed out to warn about it)
It's also worth noting one more thing: the simplicity of doing manual checked layout calculation becomes less important as we stabilize more of Layout. Yes, code from before the functionality was stable exists, but more code will be written in the future than exists currently.
As such, I still think guaranteeing the isize::MAX rule is respected by safe code using Layout is very desirable. If we go for clean semantics moving forward while not making existing worse, the counterproposal to my harm-reduction proposal would be
- The safe
Layoutmethods enforcesize <= isize::MAX; Layout::from_size_align_unchecked withsize > isize::MAXis carefully defined as breaking the safety but not validity invariant ofLayout`: we document the behavior such that e.g.Layoutmanipulation functions are sound but may return incorrect (e.g. moduloisize::MAX as usize + 1) results or an unsafeLayout(i.e.size > isize::MAX),- Allocation is unsound with
size > isize::MAX, and handle_layout_erroraccepts unsafeLayout;
- Allocator implementations are encouraged (but not required) to add a guarantee that
size > isize::MAXallocations are not UB and always fail- But we note that this is not sufficient for the replaceable
#[global_allocator]andGlobalnever exposes any unsafe requirement weakenings provided by the concrete#[global_allocator]; and
- But we note that this is not sufficient for the replaceable
- Optionally still include a precheck shim in the
allocfree fn(s) to directly callhandle_alloc_errorwhensize > isize::MAXas a risk mitigation.
As an addendum, while I was looking through the published crates that call Layout::from_size_align_unchecked(), I found 25 crates that are unsound before and after the change, regardless of whether alloc() enforces an isize::MAX bound:
Unconditionally unsound crates
(These crates may be unsound for reasons beyond those listed; I just picked whatever issue was easiest to illustrate.)
zstd-sysv2.0.1+zstd.1.5.2: Assumes thatalloc::dealloc()ignores the layout passed to it.// --target wasm32-unknown-unknown // default-features = false // SAFETY: No preconditions. let cctx = unsafe { zstd_sys::ZSTD_createCCtx() }; assert!(!ptr.is_null()); // SAFETY: `ptr` was allocated with `ZSTD_createCCtx()`. unsafe { zstd_sys::ZSTD_freeCCtx(cctx) }; // calls alloc::dealloc() with size 1 on an allocation of size 3672 // at zstd_sys::wasm_shim::rust_zstd_wasm_shim_free()matrixmultiplyv0.3.2: Uses wrapping arithmetic to compute the allocation size.// export MATMUL_SGEMM_NC=65528 MATMUL_SGEMM_KC=16384 MATMUL_SGEMM_MC=8 // --target i686-unknown-linux-gnu // features = ["constconf"] // overflow-checks = false assert!(is_x86_feature_detected!("fma")); let size = usize::MAX / 256 + 1; // SAFETY: // - `a` points to a matrix of size 8 x 16384 with strides (0, 0). // - `b` points to a matrix of size 16384 x 65528 with strides (0, 0). // - `c` points to a matrix of size 8 x 65528 with strides (65528, 1). unsafe { matrixmultiply::sgemm( 8, 16384, 65528, 0.0, &0.0, 0, 0, &0.0, 0, 0, 0.0, [0.0; 524224].as_mut_ptr(), 65528, 1, ); } // calls alloc::alloc() with size 0 // at matrixmultiply::aligned_alloc::Alloc::<f32>::new()rkyvv0.7.39: Fails to check forLayoutoverflow.use rkyv::AlignedVec; AlignedVec::with_capacity(usize::MAX - 14); // calls Layout::from_size_align_unchecked(usize::MAX - 14, 16) // at rkyv::util::aligned_vec::AlignedVec::with_capacity()sledv0.34.7: Fails to check for allocation failure.// debug-assertions = false use sled::Config; use std::{ alloc::{GlobalAlloc, Layout, System}, ptr, }; struct Alloc; #[global_allocator] static ALLOC: Alloc = Alloc; // SAFETY: Wraps `System`'s methods, possibly indicating failure. unsafe impl GlobalAlloc for Alloc { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { #[cfg(target_pointer_width = "32")] const SIZE: usize = 65536; #[cfg(target_pointer_width = "64")] const SIZE: usize = 4194304; if layout.size() == SIZE { ptr::null_mut() } else { System.alloc(layout) } } unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { System.dealloc(ptr, layout) } } Config::new().open().unwrap(); // dereferences a null pointer // at <sled::pagecache::pagetable::Node1 as crossbeam_epoch::atomic::Pointable>::deref() // at crossbeam_epoch::atomic::Shared::<sled::pagecache::pagetable::Node1>::deref() // at sled::pagecache::pagetable::PageTable::traverse()tinysetv0.4.14: Fails to check for allocation failure.use tinyset::SetU32; SetU32::with_capacity_and_bits(usize::MAX / 8 - 2, 0); // dereferences a null pointer // at tinyset::setu32::SetU32::with_capacity_and_bits()miniz_oxide_c_apiv0.3.0: Fails to check for allocation failure.// --target x86_64-unknown-linux-gnu use std::{ ffi::c_void, ptr, sync::atomic::{AtomicBool, Ordering}, }; static ALLOC: AtomicBool = AtomicBool::new(true); // SAFETY: Wraps `__libc_malloc()`, possibly indicating failure. #[no_mangle] extern "C" fn malloc(size: usize) -> *mut c_void { extern "C" { fn __libc_malloc(size: usize) -> *mut c_void; } if ALLOC.swap(true, Ordering::Relaxed) { unsafe { __libc_malloc(size) } } else { ptr::null_mut() } } ALLOC.store(false, Ordering::Relaxed); // SAFETY: Neither the input nor the output pointer is accessed. unsafe { miniz_oxide_c_api::tdefl_compress_mem_to_mem( &mut () as *mut _ as *mut c_void, 0, ptr::null(), 0, 0, ); } // calls ptr::write() with a null pointer // at miniz_oxide_c_api::tdef::tdefl_compress_mem_to_output()arrowv22.0.0: Uses wrapping arithmetic to compute the allocation size.// overflow-checks = false use arrow::alloc; alloc::allocate_aligned::<u16>(usize::MAX / 2 + 1); // calls alloc::alloc() with size 0 // at arrow::alloc::allocate_aligned::<u16>()mluav0.8.3: Fails to check forLayoutoverflow.// --target i686-unknown-linux-gnu // features = ["lua54", "vendored"] // overflow-checks = false use mlua::{Lua, Function}; let mut chunk = Vec::new(); chunk.extend_from_slice(b"\x1bLuaT\0\x19\x93\r\n\x1a\n\x04\x08\x08x"); chunk.extend_from_slice(b"\x56\0\0\0\0\0\0\0\0\0\0\0(w@\0"); chunk.extend_from_slice(b"\x80\x80\x80\0\0\0\x01\x7f\x7f\x7f\xff"); let lua = Lua::new(); let load: Function<'_> = lua.globals().get("load").unwrap(); load.call::<_, ()>(lua.create_string(&chunk).unwrap()).unwrap(); // calls Layout::from_size_align_unchecked(usize::MAX - 3, 8) // at mlua::lua::inner_new::allocator()v_framev0.2.5: Fails to check the requested size against 0.use v_frame::plane::PlaneData; PlaneData::<u8>::new(0); // calls alloc::alloc() with size 0 // at v_frame::plane::PlaneData::<u8>::new_uninitialized()tract-linalgv0.17.7: Fails to check for allocation failure.use std::{ alloc::{GlobalAlloc, Layout, System}, ptr, sync::atomic::{AtomicBool, Ordering}, }; use tract_linalg::{ frame::{ElementWise, ElementWiseImpl}, generic::SSigmoid4, }; struct Alloc(AtomicBool); #[global_allocator] static ALLOC: Alloc = Alloc(AtomicBool::new(true)); // SAFETY: Wraps `System`'s methods, possibly indicating failure. unsafe impl GlobalAlloc for Alloc { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { if self.0.load(Ordering::Relaxed) { System.alloc(layout) } else { ptr::null_mut() } } unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { System.dealloc(ptr, layout) } } ALLOC.0.store(false, Ordering::Relaxed); let ew = ElementWiseImpl::<SSigmoid4, _>::new(); ew.run(&mut [0.0]).unwrap(); // calls slice::from_raw_parts_mut() with a null pointer // at <tract_linalg::frame::element_wise::ElementWiseImpl< // tract_linalg::generic::sigmoid::SSigmoid4, // f32, // > as tract_linalg::frame::element_wise::ElementWise<f32>>::run()cryptovecv0.6.1: Uses wrapping arithmetic to compute the allocation size.// overflow-checks = false use cryptovec::CryptoVec; CryptoVec::with_capacity(usize::MAX / 2 + 2); // calls alloc::alloc_zeroed() with size 0 // at cryptovec::CryptoVec::with_capacity()scalyv0.0.37: Erroneously marks an unsafe function as safe in the public API.use scaly::String; use std::ptr; String::create(ptr::null_mut(), ptr::null(), 0); // dereferences a null pointer // at scaly::containers::string::String::create()rs-libcv0.2.3: Fails to check forLayoutoverflow.extern crate rs_libc; use std::{ alloc::{self, Layout}, mem, }; let size = usize::MAX - mem::size_of::<usize>() - 6; // SAFETY: The layout has non-zero size. unsafe { alloc::alloc(Layout::from_size_align(size, 8).unwrap()) }; // calls Layout::from_size_align_unchecked(usize::MAX - 6, 8) // at rs_libc::alloc::malloc()libdeflaterv0.10.0: Fails to check for allocation failure.// features = ["use_rust_alloc"] use libdeflater::Compressor; use std::{ alloc::{GlobalAlloc, Layout, System}, ptr, sync::atomic::{AtomicBool, Ordering}, }; struct Alloc(AtomicBool); #[global_allocator] static ALLOC: Alloc = Alloc(AtomicBool::new(true)); // SAFETY: Wraps `System`'s methods, possibly indicating failure. unsafe impl GlobalAlloc for Alloc { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { if self.0.load(Ordering::Relaxed) { System.alloc(layout) } else { ptr::null_mut() } } unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { System.dealloc(ptr, layout) } } ALLOC.0.store(false, Ordering::Relaxed); Compressor::new(Default::default()); // dereferences a null pointer // at libdeflater::malloc_wrapper::malloc()sendfdv0.4.3: Uses a truncating cast to compute the allocation size.// --target x86_64-unknown-linux-gnu use sendfd::SendWithFd; use std::os::unix::net::UnixDatagram; let socket = UnixDatagram::unbound().unwrap(); let fds = vec![0; u32::MAX as usize / 4 - 4]; socket.send_with_fd(&[], &fds).unwrap(); // calls alloc::alloc() with size 0 // at sendfd::construct_msghdr_for()refpoolv0.4.3: Fails to check for allocation failure.// debug-assertions = false use refpool::Pool; use std::{ alloc::{GlobalAlloc, Layout, System}, ptr, sync::atomic::{AtomicBool, Ordering}, }; struct Alloc(AtomicBool); #[global_allocator] static ALLOC: Alloc = Alloc(AtomicBool::new(true)); // SAFETY: Wraps `System`'s methods, possibly indicating failure. unsafe impl GlobalAlloc for Alloc { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { if self.0.load(Ordering::Relaxed) { System.alloc(layout) } else { ptr::null_mut() } } unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { System.dealloc(ptr, layout) } } let pool: Pool<()> = Pool::new(1); ALLOC.0.store(false, Ordering::Relaxed); pool.fill(); // calls NonNull::new_unchecked() with a null pointer // at <refpool::types::ElementPointer<()> as refpool::pointer::Pointer<refpool::refbox::RefBox<()>>>::wrap() // at refpool::pool::Pool::<()>::fill()pagecachev0.19.4: Fails to check for allocation failure.// serde = { version = "1.0.99", features = ["derive"] } use pagecache::{ConfigBuilder, Materializer, PageCache, PAGETABLE_NODE_SZ}; use serde::{Deserialize, Serialize}; use std::{ alloc::{GlobalAlloc, Layout, System}, ptr, }; struct Alloc; #[global_allocator] static ALLOC: Alloc = Alloc; // SAFETY: Wraps `System`'s methods, possibly indicating failure. unsafe impl GlobalAlloc for Alloc { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { if layout.size() == PAGETABLE_NODE_SZ { ptr::null_mut() } else { System.alloc(layout) } } unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { System.dealloc(ptr, layout) } } #[derive(Clone, Debug, Deserialize, Serialize)] struct Dummy; impl Materializer for Dummy { fn merge(&mut self, _: &Self) { unimplemented!() } } PageCache::<Dummy>::start(ConfigBuilder::new().build()).unwrap(); // calls Box::from_raw() with a null pointer // at pagecache::ds::pagetable::Node1::< // pagecache::ds::stack::Stack<( // core::option::Option<pagecache::pagecache::Update<Dummy>>, // pagecache::pagecache::CacheInfo, // )>, // >::new()slabmallocv0.11.0: Fails to check forLayoutoverflow.// debug-assertions = false use slabmalloc::{AllocablePage, Rawlink, SCAllocator}; use std::{alloc::Layout, sync::atomic::AtomicU64}; struct DummyPage; impl AllocablePage for DummyPage { const SIZE: usize = usize::MAX - 46; fn bitfield(&self) -> &[AtomicU64; 8] { unimplemented!() } fn bitfield_mut(&mut self) -> &mut [AtomicU64; 8] { unimplemented!() } fn prev(&mut self) -> &mut Rawlink<Self> { unimplemented!() } fn next(&mut self) -> &mut Rawlink<Self> { unimplemented!() } } let mut alloc = SCAllocator::<DummyPage>::new(usize::MAX - 126); let layout = Layout::from_size_align(0, 128).unwrap(); alloc.allocate(layout).unwrap(); // calls Layout::from_size_align_unchecked(usize::MAX - 126, 128) // at slabmalloc::sc::SCAllocator::<'_, DummyPage>::allocate()scryer-prologv0.8.127: Passes an incorrectlayouttoalloc::realloc().# --target x86_64-unknown-linux-gnu scryer-prolog < /dev/null # calls alloc::realloc() with prior size 18432 on an allocation of size 36864 # at scryer_prolog::machine::raw_block::RawBlock::<scryer_prolog::machine::heap::StandardHeapTraits>::grow()hecsv0.9.0: Uses wrapping arithmetic to compute the allocation size.// overflow-checks = false use hecs::World; World::new().reserve::<([u8; usize::MAX / 262144 + 1],)>(262144); // calls alloc::alloc() with size 0 // at hecs::archetype::Archetype::grow_exact()scudov0.1.2: Fails to check forLayoutoverflow.// overflow-checks = false use scudo::GlobalScudoAllocator; use std::{ alloc::{GlobalAlloc, Layout}, mem, }; let size = usize::MAX - mem::size_of::<usize>() * 2 + 2; let layout = Layout::array::<u8>(size).unwrap(); // SAFETY: The layout has non-zero size. unsafe { GlobalScudoAllocator.alloc(layout) }; // calls Layout::from_size_align_unchecked(usize::MAX - size_of::<usize>() * 2 + 2, size_of::<usize>() * 2) // at <scudo::GlobalScudoAllocator as core::alloc::global::GlobalAlloc>::alloc()ring_bufferv2.0.2: Uses wrapping arithmetic to compute the allocation size.// overflow-checks = false use ring_buffer::RingBuffer; RingBuffer::<u16>::with_capacity(usize::MAX / 4 + 2); // calls alloc::alloc() with size 0 // at ring_buffer::RingBuffer::<u16>::with_capacity()assemblerv0.10.1: Uses wrapping arithmetic to compute the allocation size.// overflow-checks = false use assembler::{ExecutableAnonymousMemoryMap, InstructionStreamHints}; use std::mem; let mut map = ExecutableAnonymousMemoryMap::new(0, false, false).unwrap(); map.instruction_stream(&InstructionStreamHints { number_of_labels: usize::MAX / mem::size_of::<usize>() + 1, ..Default::default() }); // calls alloc::alloc() with size 0 // at assembler::LabelledLocations::new()rquickjs-corev0.1.7: Uses wrapping arithmetic to compute the allocation size.// features = ["allocator"] // overflow-checks = false use rquickjs_core::{Allocator, RustAllocator}; use std::mem; RustAllocator.alloc(usize::MAX - mem::size_of::<usize>() * 2 + 2); // calls alloc::alloc() with size 0 // at <rquickjs_core::allocator::rust::RustAllocator as rquickjs_core::allocator::Allocator>::alloc()t-rust-less-libv0.2.15: Fails to check for allocation failure.use std::{ alloc::{GlobalAlloc, Layout, System}, ptr, sync::atomic::{AtomicBool, Ordering}, }; use t_rust_less_lib::memguard::SecretBytes; struct Alloc(AtomicBool); #[global_allocator] static ALLOC: Alloc = Alloc(AtomicBool::new(true)); // SAFETY: Wraps `System`'s methods, possibly indicating failure. unsafe impl GlobalAlloc for Alloc { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { if self.0.load(Ordering::Relaxed) { System.alloc(layout) } else { ptr::null_mut() } } unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { System.dealloc(ptr, layout) } } ALLOC.0.store(false, Ordering::Relaxed); SecretBytes::with_capacity(0); // calls NonNull::new_unchecked() with a null pointer // at t_rust_less_lib::memguard::alloc::alloc_aligned()
I found 4 crates that were previously sound conditional on alloc() enforcing an isize::MAX bound, and that become unconditionally sound if the safe Layout methods enforce an isize::MAX bound: planus v0.3.1; legion v0.4.0; aligned-utils v1.0.2; kg-symbol v0.2.0.
Finally, I found 27 crates that call Layout::from_size_align_unchecked(), but were unconditionally sound and remain so: flate2 v1.0.24; wasm-bindgen v0.2.83; wasmtime-runtime v0.40.1; stacker v0.1.15; ptr_meta v0.2.0; lodepng v3.7.0; unsafe-libyaml v0.2.2; io-uring v0.5.6; rav1e v0.5.1; shuffling-allocator v1.1.2; arcstr v1.1.4; sailfish v0.4.0; simd-abstraction v0.7.0; pic32-hal v0.7.0; dep-obj v0.38.4; enarx-shim-kvm v0.6.3; stakker v0.2.5; glommio v0.7.0; win-crypto-ng v0.4.0; tree-buf v0.10.0; sentinel v0.2.3; libscmp v0.2.0; composable-allocators v0.1.7; compu v0.5.3; exocore-apps-sdk v0.1.23; atopology v0.0.32; secmem-proc v0.1.1.
Layoutmanipulation functions are sound but may return incorrect (e.g. moduloisize::MAX as usize + 1) results or an unsafeLayout(i.e.size > isize::MAX),
This concerns me. I think if we're going to make this allowed then we should say that the manipulation functions are either fully correct or they panic/abort -- I wouldn't want to add new unsafety to things because all of a sudden their layout calculations start giving them garbage. We do still have the "rounding up needs to fit in usize" rule, which is the most important part (IMHO) for making those functions work efficiently. (It does lose us some potential optimizations that would have been nice, taking advantage of the isize::MAX limit, but we've been living without those forever, so that's fine.)
Optionally still include a precheck shim in the
allocfree fn(s) to directly callhandle_alloc_errorwhensize > isize::MAXas a risk mitigation.
Maybe have one in the default global allocator, just in case, and double-check that the popular better allocator crates already have these limit (which I'm 97% sure they already do).
Maybe have one in the default global allocator, just in case, and double-check that the popular better allocator crates already have these limit (which I'm 97% sure they already do).
As I mentioned in my earlier comment, none of our 3 WASM environments (Emscripten, wasi-libc, wasm32-unknown-unknown) enforce isize::MAX right now. Especially https://github.com/alexcrichton/dlmalloc-rs looks like it could be low-hanging fruit here.
An alternative is to add an assert!() in the existing from_size_align_unchecked function (and perhaps deprecate it), and add a new unchecked function with the additional requirement. That way, the original function doesn't suddenly get a new unsafe requirement. But it'd require coming up with a new name. (new_unchecked?)
We've discussed this in a few different libs-api meetings. After considering the alternatives, we felt that keeping #95295 is still the best path forward, even though it is technically a breaking change. Many of the potentially broken crates have already been updated. We've also marked https://github.com/rust-lang/rust/pull/95295 to be included in the release notes of 1.65 to call out extra attention to this change when that release goes out next week.