Skip to content
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

{Rc,Arc}::{new_uninit_slice,from_iter} create too large references #95334

Closed
Tracked by #10
RalfJung opened this issue Mar 26, 2022 · 4 comments · Fixed by #95295
Closed
Tracked by #10

{Rc,Arc}::{new_uninit_slice,from_iter} create too large references #95334

RalfJung opened this issue Mar 26, 2022 · 4 comments · Fixed by #95295
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Mar 26, 2022

See #95252, #95295 and the discussion on Zulip for details. I just figured we should also have an issue to track this.

The following code:

#![feature(new_uninit)]
use std::rc::Rc;

fn main() {
    let p = Rc::<[u8]>::new_uninit_slice(isize::MAX as usize + 1);
    p.last();
}

fails in Miri (on a 32bit target) with

error: Undefined Behavior: invalid metadata in wide pointer: slice is bigger than largest supported object
    --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/rc.rs:1326:24
     |
1326 |             ptr::write(&mut (*inner).strong, Cell::new(1));
     |                        ^^^^^^^^^^^^^^^^^^^^ invalid metadata in wide pointer: slice is bigger than largest supported object
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
             
     = note: inside `std::rc::Rc::<[std::mem::MaybeUninit<u8>]>::try_allocate_for_layout::<[closure@std::rc::Rc<[std::mem::MaybeUninit<u8>]>::allocate_for_slice::{closure#0}], [closure@std::rc::Rc<[std::mem::MaybeUninit<u8>]>::allocate_for_slice::{closure#1}]>` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/rc.rs:1326:24
     = note: inside `std::rc::Rc::<[std::mem::MaybeUninit<u8>]>::allocate_for_layout::<[closure@std::rc::Rc<[std::mem::MaybeUninit<u8>]>::allocate_for_slice::{closure#0}], [closure@std::rc::Rc<[std::mem::MaybeUninit<u8>]>::allocate_for_slice::{closure#1}]>` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/rc.rs:1295:13
     = note: inside `std::rc::Rc::<[std::mem::MaybeUninit<u8>]>::allocate_for_slice` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/rc.rs:1375:13
     = note: inside `std::rc::Rc::<[u8]>::new_uninit_slice` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/rc.rs:691:31
note: inside `main` at rc.rs:5:13
    --> rc.rs:5:13
     |
5    |     let p = Rc::<[u8]>::new_uninit_slice(isize::MAX as usize + 1);
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

In fact it fails even without the +1 since the RcBox is still too big.

On a 64bit target it would fail the same way if creating that huge allocation would not already lead Miri to abort earlier. ;)

@RalfJung RalfJung added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 26, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 26, 2022
@RalfJung
Copy link
Member Author

Cc @Gankra @CAD97 @the8472 @rust-lang/wg-allocators

bors added a commit to rust-lang/miri that referenced this issue Mar 26, 2022
another test for too big type

The existing test covers "slice is bigger than largest supported object" but we had no test covering "total size is bigger than largest supported object", which happens when the unsized tail itself is okay in terms of size, but together with the sized prefix it becomes too big.

Cc rust-lang/rust#95334
bors added a commit to rust-lang/miri that referenced this issue Mar 26, 2022
another test for too big type

The existing test covers "slice is bigger than largest supported object" but we had no test covering "total size is bigger than largest supported object", which happens when the unsized tail itself is okay in terms of size, but together with the sized prefix it becomes too big.

Cc rust-lang/rust#95334
@nagisa
Copy link
Member

nagisa commented Mar 26, 2022

I wonder if fixing this is in any way tied to the discussion about the stability of size_of output. In particular I imagine something like Rc::<[u8]>::new_uninit_slice(isize::MAX as usize - 2); works today (since the size_of::<RcBox> = 2), and would work after some fix for this issue, but does that also imply that we cannot ever increase the size of RcBox either, even though it is a very private implementation detail?

@Gankra
Copy link
Contributor

Gankra commented Mar 26, 2022

I think the extreme vagueness of repr(rust) already implies we get to change size_of whenever and break you if you're trying to super precisely rely on them having a stable value.

@Lokathor
Copy link
Contributor

Yes, very few standard library types make any promises about their size (though a few do, eg Vec, NonZeroFoo, etc).

@m-ou-se m-ou-se added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 30, 2022
@bors bors closed this as completed in 4ec97d9 Jul 10, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
Enforce that layout size fits in isize in Layout

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](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Layout.20Isn't.20Enforcing.20The.20isize.3A.3AMAX.20Rule).
Fixes rust-lang/rust#95334

Some relevant quotes:

`@eddyb,` rust-lang/rust#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,` rust-lang/rust#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`](https://rust-lang.github.io/unsafe-code-guidelines/layout/scalars.html#isize-and-usize) 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
6 participants