Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

core::iter::repeat_n is unsound with Box<T> #130141

Closed
jwong101 opened this issue Sep 9, 2024 · 5 comments · Fixed by #130145
Closed

core::iter::repeat_n is unsound with Box<T> #130141

jwong101 opened this issue Sep 9, 2024 · 5 comments · Fixed by #130145
Assignees
Labels
A-box Area: Our favorite opsem complication C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@jwong101
Copy link
Contributor

jwong101 commented Sep 9, 2024

This is technically different from #130140 as there's no UAF. However, using core::iter::repeat_n with Box<T> has undefined behavior if you use the last/original element after moving the iterator, since the Box field has noalias properties.

fn main() {
    let mut y = std::iter::repeat_n(Box::new(0), 1);
    let x = y.next().unwrap();
    let _z = y;
    dbg!(*x);
}
Miri Backtrace

Compiling playground v0.0.1 (/playground)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.96s
     Running `/playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/playground`
error: Undefined Behavior: attempting a read access using <2719> at alloc1241[0x0], but that tag does not exist in the borrow stack for this location
 --> src/main.rs:5:5
  |
5 |     dbg!(*x);
  |     ^^^^^^^^
  |     |
  |     attempting a read access using <2719> at alloc1241[0x0], but that tag does not exist in the borrow stack for this location
  |     this error occurs as part of an access at alloc1241[0x0..0x4]
  |
  = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
  = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <2719> was created by a Unique retag at offsets [0x0..0x4]
 --> src/main.rs:3:13
  |
3 |     let x = y.next().unwrap();
  |             ^^^^^^^^^^^^^^^^^
help: <2719> was later invalidated at offsets [0x0..0x4] by a Unique retag (of a reference/box inside this compound value)
 --> src/main.rs:4:14
  |
4 |     let _z = y;
  |              ^
  = note: BACKTRACE (of the first span):
  = note: inside `main` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/macros.rs:364:13: 364:16
  = note: this error originates in the macro `dbg` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

Wrapping the element using MaybeUninit (or MaybeDangling once we have that) should make this sound.

@jwong101 jwong101 added the C-bug Category: This is a bug. label Sep 9, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 9, 2024
@theemathas
Copy link
Contributor

See also: rust-lang/unsafe-code-guidelines#245

@fee1-dead fee1-dead self-assigned this Sep 9, 2024
@saethlin saethlin added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Sep 9, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 9, 2024
@WaffleLapkin
Copy link
Member

The minimal example is actually this:

fn main() {
    let y = std::iter::repeat_n(Box::new(0), 0);
    let _z = y;
}

Because repeat_n eagerly calls drop for n = 0. This basically boils down to

let mut x = ManuallyDrop::new(Box::new(0));
ManuallyDrop::drop(&mut x);
let _y = x;

Which is indeed rust-lang/unsafe-code-guidelines#245. I quite strongly think that this is a bug in miri and/or ManuallyDrop (ManuallyDrop should use MaybeDangling inside of itself probably?). That being said this is not yet decided (again, see the UCG issue) :/

@apiraino
Copy link
Contributor

apiraino commented Sep 9, 2024

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 9, 2024
@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2024

I quite strongly think that this is a bug in miri and/or ManuallyDrop (ManuallyDrop should use MaybeDangling inside of itself probably?)

Yeah, it should -- someone just has to implement MaybeDangling first. ;)
See #118166.

@saethlin saethlin changed the title core::iter::repeat_n is unsound with Unique<T> core::iter::repeat_n is unsound with Box<T> Sep 9, 2024
@saethlin
Copy link
Member

saethlin commented Sep 9, 2024

I tweaked the title because while there is an experimental Miri flag to make Unique magical, codegen only has special treatment for Box.

@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 9, 2024
@bors bors closed this as completed in 2e367d9 Sep 17, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 26, 2024
…homcc,traviscross

Document subtleties of `ManuallyDrop`

After seeing rust-lang#130140 and rust-lang#130141, I figured that `ManuallyDrop` needs documentation explaining its subtleties, hence this PR.

See also rust-lang/unsafe-code-guidelines#245
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 27, 2024
Rollup merge of rust-lang#130279 - theemathas:manually-drop-docs, r=thomcc,traviscross

Document subtleties of `ManuallyDrop`

After seeing rust-lang#130140 and rust-lang#130141, I figured that `ManuallyDrop` needs documentation explaining its subtleties, hence this PR.

See also rust-lang/unsafe-code-guidelines#245
@workingjubilee workingjubilee added the A-box Area: Our favorite opsem complication label Oct 1, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-box Area: Our favorite opsem complication C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants