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

unsized_fn_params allows some unsized locals #111175

Open
RalfJung opened this issue May 4, 2023 · 5 comments
Open

unsized_fn_params allows some unsized locals #111175

RalfJung opened this issue May 4, 2023 · 5 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html F-unsized_fn_params `#![feature(unsized_fn_params)]` requires-nightly This issue requires a nightly compiler in some way.

Comments

@RalfJung
Copy link
Member

RalfJung commented May 4, 2023

Found in rust-lang/miri#2872: this code

#![feature(core_intrinsics, unsized_fn_params)]
use std::intrinsics;

pub fn forget_unsized(t: [i32]) {
    intrinsics::forget(t)
}

with -Zmir-opt-level=0 generates this LLVM IR

define void @_ZN4test14forget_unsized17h6fbb3409c8556ed4E(ptr %0, i64 %1) unnamed_addr #0 {
start:
  %_2 = alloca { ptr, i64 }, align 8
  %t = alloca { ptr, i64 }, align 8
  %2 = getelementptr inbounds { ptr, i64 }, ptr %t, i32 0, i32 0
  store ptr %0, ptr %2, align 8
  %3 = getelementptr inbounds { ptr, i64 }, ptr %t, i32 0, i32 1
  store i64 %1, ptr %3, align 8
  %4 = getelementptr inbounds { ptr, i64 }, ptr %t, i32 0, i32 0
  %5 = load ptr, ptr %4, align 8, !noundef !2
  %6 = getelementptr inbounds { ptr, i64 }, ptr %t, i32 0, i32 1
  %7 = load i64, ptr %6, align 8, !noundef !2
  %8 = mul nsw i64 %7, 4
  %9 = alloca i8, i64 %8, align 16
  call void @llvm.memcpy.p0.p0.i64(ptr align 16 %9, ptr align 1 %5, i64 %8, i1 false)
  %10 = getelementptr inbounds { ptr, i64 }, ptr %_2, i32 0, i32 0
  store ptr %9, ptr %10, align 8
  %11 = getelementptr inbounds { ptr, i64 }, ptr %_2, i32 0, i32 1
  store i64 %7, ptr %11, align 8
  ret void
}

Notice the alloca. This is bad! Our alloca code path is unsound (it does not properly account for alignment); the point of the unsized_fn_params feature gate (separate from unsized_locals) was to avoid hitting that code path.

I repeat my stance that we should remove the unsound alloca code path (effectively de-imlementing unsized locals) to ensure this can never happen.

We don't have a MIR building ping group AFAIK, so Cc @rust-lang/wg-mir-opt I guess.

Here's a Miri testcase:

#![feature(forget_unsized, unsized_fn_params)]

fn main() {
    let b: Box<[u32]> = Box::new([1, 2, 3]);
    std::mem::forget_unsized(*b);
}
@RalfJung
Copy link
Member Author

RalfJung commented May 4, 2023

I forgot how exactly #78152 works, but for some reason it lets this pass. Either the analysis should reject this, or MIR building needs to not create an unsized local here.

@jyn514 jyn514 added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. F-unsized_fn_params `#![feature(unsized_fn_params)]` labels May 4, 2023
@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 4, 2023

The bad MIR builing is here:

if let ExprKind::Deref { arg } = expr.kind {

Any unsized function call operand other than a Box deref expression not wrapped in a block, is compiled by as_operand which introduces a temporary.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 23, 2023

@rustbot label -I-unsound

(#71416 has been closed)

@rustbot
Copy link
Collaborator

rustbot commented May 23, 2023

Error: Parsing relabel command in comment failed: ...'-I-unsound' | error: a label delta at >| ' (#71416 h'...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@rustbot rustbot removed the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label May 23, 2023
@RalfJung RalfJung changed the title unsized_fn_params allows some unsized locals, hitting unsound alloca codegen path unsized_fn_params allows some unsized locals Aug 6, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2023

This is still an issue with current rustc (fca59ab) -- a similar issue got fixed (#111355) but this one is still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html F-unsized_fn_params `#![feature(unsized_fn_params)]` requires-nightly This issue requires a nightly compiler in some way.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants