Fix (A)Rc<[T]> slices larger than isize::MAX bytes when collecting from a TrustedLen iter

[the8472]

Due to ptr::add restrictions allocations must be <= isize::MAX bytes (see UCG). Since (A)RcBox is 2 words larger than the slice even when Vec ensures that the allocation is small enough the internal layout calculation of (A)Rc can still exceed isize::MAX, so we have to check it directly.

Affected methods:

• Arc::new_uninit_slice
• Arc::from_iter
• Rc::new_uninit_slice
• Rc::from_iter

Zulip discussion
Fixes #95334

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[eddyb]

I haven't kept up with any other discussions (so this might be redundant), but I just wanted to say that because 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.
cc @RalfJung @rust-lang/wg-allocators

[Gankra]

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.

[CAD97]

I agree that this limitation is very difficult to hold manually. My only concern to adding the check into Layout is that checking it generically may be more expensive than expected. (Well, that, and that from_size_align_unchecked would no longer be fully unchecked, but eh.)

Specifically, Layout itself doesn't require that size is a multiple of align, just that size rounded up to a multiple of align is less than usizeisize::MAX. This would make the check for too-large size more complicated; rather than size <= isize::MAX, it'd be (perhaps? my modular math brain is not fully awake) something like ((size - 1) & !(align - 1)) + align <= isize::MAX. (And running out of bits also seems like a problematic edge case...)

(EDIT: After checking: the current check is if size > usize::MAX - (align - 1) { return Err }; changing that to isize::MAX is a trivial change.)

_{And I made pad_to_align infallible.}

So:

• Yes these specific cases should be patched
• Yes we should make the invariant harder to break
• But we should be careful that this doesn't cause unnecessary checking with normal use if possible.
  • This probably means that Layout users using the building API need to use .and_then and .unwrap once at the end, rather than unwrapping along the way.
  • Alternatively, introduce a separate LayoutBuilder type which works infallibily (except for debug checked integer overflow) and only checks upon conversion to Layout proper.

Here with Rc the error would come from Layout::extend exceeding the allowed size.

[the8472]

Closing since #95295 is getting FCPed