-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
#95295 causes unsoundness in multiple existing crates #101899
Comments
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 #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. |
I looked through the issue; the change seems to be mainly to make the job of |
Adding the regression label to make sure I have eyes on it during the release process. |
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 Backing out for this release is prudent. For later releases, it might be a reasonable state to treat a size > cc @scottmcm |
[stable] Prepare 1.64.0 release This PR prepares the 1.64.0 stable release builds. In addition to bumping the channel and including the latest release notes changes, this PR also backports the following PRs: * rust-lang#100852 * rust-lang#101366 * rust-lang#101468 * rust-lang#101922 This PR also reverts the following PRs, as decided in rust-lang#101899 (comment): * rust-lang#95295 * rust-lang#99136 (followup to the previous PR) r? `@ghost` cc `@rust-lang/release`
removing prioritization label since it looks it's waiting on T-libs (Zulip discussion). @estebank do you think we can remove also @rustbot label -I-prioritize |
Are you talking about #95334? That seems more like a Miri oddity than an issue in practice. It successfully allocates more than 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 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
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 So, while there's a long tail of platforms that fulfill allocation requests larger than |
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 |
The OP does not link to the source locations instantiating those |
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 🙃
This is the case in the current nightly code, correct? This is just a statement to not also do #99134? |
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:
Of the crates that were sound conditional on
|
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. |
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). |
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 Protecting against this requires that the allocation size is checked before the |
...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:
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:
However, I'm not sold on the idea that C compatible allocation must fail for 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. |
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: 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 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 |
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 As such, I still think guaranteeing the
|
As an addendum, while I was looking through the published crates that call Unconditionally unsound crates(These crates may be unsound for reasons beyond those listed; I just picked whatever issue was easiest to illustrate.)
I found 4 crates that were previously sound conditional on Finally, I found 27 crates that call |
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
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 |
An alternative is to add an |
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 #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. |
Remove the old `ValidAlign` name Since it looks like there won't be any reverts needed in `Layout` for rust-lang#101899 (comment), finish off this change that I'd left out of rust-lang#102072. r? `@thomcc` cc tracking issue rust-lang#102070
Remove the old `ValidAlign` name Since it looks like there won't be any reverts needed in `Layout` for rust-lang#101899 (comment), finish off this change that I'd left out of rust-lang#102072. r? ``@thomcc`` cc tracking issue rust-lang#102070
Remove the old `ValidAlign` name Since it looks like there won't be any reverts needed in `Layout` for rust-lang/rust#101899 (comment), finish off this change that I'd left out of #102072. r? ``@thomcc`` cc tracking issue #102070
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 toisize::MAX
bytes, this restriction was also added toLayout::from_size_align_unchecked()
, which is a public and widely used API. Some crates were sound under the previous overflow property, usually panicking or returning an error after checking theLayout
againstisize::MAX
. However, these have become unsound under the new overflow property, since just constructing the overlargeLayout
is now UB. Also, some crates created overlarge layouts for the sole purpose of feeding them intohandle_alloc_error()
. To list the instances I've found:GlobalAlloc::realloc()
impl incore
:semver
v1.0.14:hashbrown
v0.12.3:rusqlite
v0.28.0 (admittedly contrived):allocator_api
v0.6.0:pyembed
v0.22.0:cap
v0.1.1:scoped-arena
v0.4.1:Also, many more crates were sound under the condition that
alloc::alloc()
always fails on allocations larger thanisize::MAX
bytes, but likely unsound if it were to successfully return an allocated pointer. Before #95295, they would either panic, return an error, or callhandle_alloc_error()
fromalloc()
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
bumpalo
v3.11.0:async-task
v4.3.0:zerocopy
v0.6.1:memsec
v0.6.2:bevy_ecs
v0.8.1:lasso
v0.6.0:thin-dst
v1.1.0:lightproc
v0.3.5:thin-vec
v0.2.8:bsn1
v0.4.0:seckey
v0.11.2:slice-dst
v1.5.1:stable-vec
v0.4.0:The text was updated successfully, but these errors were encountered: