Enforce that layout size fits in isize in Layout by CAD97 · Pull Request #95295 · rust-lang/rust (original) (raw)

As it turns out, enforcing this in APIs that already enforce usize overflow is fairly trivial. Layout::from_size_align_unchecked continues to "allow" sizes which (when rounded up) would overflow isize, but these are now declared as library UB for Layout, meaning that consumers of Layout no longer have to check this before making an allocation.

(Note that this is "immediate library UB;" IOW it is valid for a future release to make this immediate "language UB," and there is an extant patch to do so, to allow Miri to catch this misuse.)

See also #95252, Zulip discussion.
Fixes #95334

Some relevant quotes:

@eddyb, #95252 (comment)

[B]ecause of the non-trivial presence of both of these among code published on e.g. crates.io:

1. Layout "producers" / GlobalAlloc "users": smart pointers (including alloc::rc copies with small tweaks), collections, etc.
2. Layout "consumers" / GlobalAlloc "providers": perhaps fewer of these, but anything built on top of OS APIs like mmap will expose > isize::MAX allocations (on 32-bit hosts) if they lack extra checks

IMO the only responsible option is to enforce the isize::MAX limit in Layout, which:

• makes Layout sound in terms of only ever allowing allocations where (alloc_base_ptr: *mut u8).offset(size) is never UB
• frees both "producers" and "consumers" of Layout from manually reimplementing the checks
  • manual checks can be risky, e.g. if the final size passed to the allocator isn't the one being checked
  • this applies retroactively, fixing the overall soundness of existing code with zero transition period or any changes required from users (as long as going through Layout is mandatory, making a "choke point")

Feel free to quote this comment onto any relevant issue, I might not be able to keep track of developments.

@Gankra, #95252 (comment)

As someone who spent way too much time optimizing libcollections checks for this stuff and tried to splatter docs about it everywhere on the belief that it was a reasonable thing for people to manually take care of: I concede the point, it is not reasonable. I am wholy spiritually defeated by the fact that liballoc of all places is getting this stuff wrong. This isn't throwing shade at the folks who implemented these Rc features, but rather a statement of how impractical it is to expect anyone out in the wider ecosystem to enforce them if some of the most audited rust code in the library that defines the very notion of allocating memory can't even reliably do it.

We need the nuclear option of Layout enforcing this rule. Code that breaks this rule is deeply broken and any "regressions" from changing Layout's contract is a correctness fix. Anyone who disagrees and is sufficiently motivated can go around our backs but the standard library should 100% refuse to enable them.

cc also @RalfJung @rust-lang/wg-allocators. Even though this technically supersedes #95252, those potential failure points should almost certainly still get nicer panics than just "unwrap failed" (which they would get by this PR).

It might additionally be worth recommending to users of the Layout API that they should ideally use .and_then/? to complete the entire layout calculation, and then panic! from a single location at the end of Layout manipulation, to reduce the overhead of the checks and optimizations preserving the exact location of each panic which are conceptually just one failure: allocation too big.

Probably deserves a T-lang and/or T-libs-api FCP (this technically solidifies the objects must be no larger than isize::MAX rule further, and the UCG document says this hasn't been RFCd) and a crater run. Ideally, no code exists that will start failing with this addition; if it does, it was likely (but not certainly) causing UB.

Changes the raw_vec allocation path, thus deserves a perf run as well.

I suggest hiding whitespace-only changes in the diff view.