search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.7k stars 12.49k forks source link

ABI code generates oversize loads when returning with a cast #45543

Open arielb1 opened 6 years ago

arielb1 commented 6 years ago

Meta

$ rustc --version
rustc 1.22.0-nightly (185cc5f26 2017-10-02)

STR

#![crate_type="rlib"]

#[no_mangle]
pub extern "C" fn foo(x: &(i8, i8, i8)) -> (i8, i8, i8) {
    *x
}

target is x86_64-unknown-linux-gnu

Expected Result

Code generated should not have UB

Actual Result

The generated no-opt LLVM IR is:

; ModuleID = 'cast.cgu-0.rs'
source_filename = "cast.cgu-0.rs"
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: nounwind uwtable
define i32 @foo({ i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }* noalias readonly dereferenceable(3)) unnamed_addr #0 {
start:
  %_3 = alloca { i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }
  ; COMMENT MINE: %_0 has size 3
  %_0 = alloca { i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }
  %1 = bitcast { i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }* %0 to i8*
  %2 = bitcast { i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }* %_3 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %2, i8* %1, i64 3, i32 1, i1 false)
  %3 = bitcast { i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }* %_3 to i8*
  %4 = bitcast { i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }* %_0 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %4, i8* %3, i64 3, i32 1, i1 false)
  ; COMMENT MINE it is cast to an i32 here
  %5 = bitcast { i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }* %_0 to i32*
  ; COMMENT MINE and then loaded as through it was size 4
  %6 = load i32, i32* %5, align 1
  ret i32 %6
}

; Function Attrs: argmemonly nounwind
declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1) #1

attributes #0 = { nounwind uwtable "probe-stack"="__rust_probestack" }
attributes #1 = { argmemonly nounwind }

This loads 4 bytes of an alloca of size 3. I couldn't trivially turn this to an end-to-end mis-code-generation (this can't be used to load from anything but the return pointer, so you can't do the standard "load at end of page" trick), but I can't see how can oversize loads can be anything but UB.

cc @eddyb - I think one solution would be to pad the return pointer alloca to always have enough space for the cast, aka:

  %_0_store = alloca {{ i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }, [1 x i8]}
  %_0 = getelementptr inbounds {{ i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }, [1 x i8]}, {{ i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }, [1 x i8]}* %_0_store, i32 0, i32 1

I think the problem load is the one in https://github.com/rust-lang/rust/blob/e847f30f571e617990f7193665ab10127100e9df/src/librustc_trans/mir/block.rs#L247, but I can't be sure.

arielb1 commented 6 years ago

This also happens when casting arguments - currently arguments can be "arbitrary" temporaries, so the return value fix won't work, but this still can't directly be used for the "end of page" segfaults:

#![crate_type="rlib"]

#[repr(C)]
#[derive(Copy, Clone)]
pub struct U24(u8, u8, u8);

extern "C" {
    fn bar(x: U24) -> U24;
}

pub extern "C" fn foo(x: U24) -> U24 {
    unsafe { bar(x) }
}

generates:

; ModuleID = 'cast2.cgu-0.rs'
source_filename = "cast2.cgu-0.rs"
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%U24 = type { i8, [0 x i8], i8, [0 x i8], i8, [0 x i8] }

; cast2::foo
; Function Attrs: nounwind uwtable
define i32 @_ZN5cast23foo17h1f3d5ac3e0c9cc02E(i32) unnamed_addr #0 {
start:
  %abi_cast1 = alloca i32
  %_3 = alloca %U24
  %x = alloca %U24
  %_0 = alloca %U24
  %abi_cast = alloca i32
  %arg0 = alloca %U24
  store i32 %0, i32* %abi_cast
  %1 = bitcast %U24* %arg0 to i8*
  %2 = bitcast i32* %abi_cast to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %1, i8* %2, i64 3, i32 1, i1 false)
  %3 = bitcast %U24* %arg0 to i8*
  %4 = bitcast %U24* %x to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %4, i8* %3, i64 3, i32 1, i1 false)
  %5 = bitcast %U24* %x to i8*
  %6 = bitcast %U24* %_3 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %6, i8* %5, i64 3, i32 1, i1 false)
  %7 = bitcast %U24* %_3 to i32*
  %8 = load i32, i32* %7, align 1 ; bad load #1
  %9 = call i32 @bar(i32 %8)
  store i32 %9, i32* %abi_cast1
  %10 = bitcast %U24* %_0 to i8*
  %11 = bitcast i32* %abi_cast1 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %10, i8* %11, i64 3, i32 1, i1 false)
  br label %bb1

bb1:                                              ; preds = %start
  %12 = bitcast %U24* %_0 to i32*
  %13 = load i32, i32* %12, align 1 ; bad load #2
  ret i32 %13
}

; Function Attrs: argmemonly nounwind
declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1) #1

; Function Attrs: nounwind
declare i32 @bar(i32) unnamed_addr #2

attributes #0 = { nounwind uwtable "probe-stack"="__rust_probestack" }
attributes #1 = { argmemonly nounwind }
attributes #2 = { nounwind "probe-stack"="__rust_probestack" }
arielb1 commented 6 years ago

The example above makes doing the alloca oversize less of a good idea - we eventually want to allow passing references to struct fields directly as function parameters, where we do not want to add a "guard byte". I think the "i24" case (which I think also includes 5/6/7-byte fields) should just be handled by doing an ugly memcpy.

eddyb commented 6 years ago

I remember fixing this, I don't understand how we're still having issues like these.

arielb1 commented 6 years ago

@eddyb

We fixed this for stores, where the problems were obvious (stack corruption), to actually get a problem with a load is harder - you have to engage an LLVM optimization.

nikomatsakis commented 6 years ago

triage: P-medium

dotdash commented 6 years ago

According to https://github.com/rust-lang/rust/issues/29988#issuecomment-299713235 this was fixed once.

nikomatsakis commented 6 years ago

Question: Do we feel this needs to stay P-high? This hasn't yet cropped up in the wild as far as I know, but it'd be good to get it done so that it never does.

@dotdash, think you'll have time to investigate?

dotdash commented 6 years ago

I have a fix for x86_64 where the return type is just wrong (should be i24) but there's a slightly more involved issue for (at least) sparc64, where a 24 bit struct is passed as i32 instead of i24, so the current bitcast for the load of the return vakue just won't do.

I can probably finish that on the weekend. Or I can just submit the fix for x86_64 first and add the rest later.

Am 18.01.2018 16:16 schrieb "Niko Matsakis" notifications@github.com:

Question: Do we feel this needs to stay P-high? This hasn't yet cropped up in the wild as far as I know, but it'd be good to get it done so that it never does.

@dotdash https://github.com/dotdash, think you'll have time to investigate?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/45543#issuecomment-358677475, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOGMpAiKZqZjvT1M_OA5yvU8Fdi3WxHks5tL2BTgaJpZM4QG9B8 .

nikomatsakis commented 6 years ago

triage: P-medium

We do not know of an instance of this occurring in the wild. It is fixed on x86_64 (right @dotdash?). Seems like it's not P-high. But @dotdash if you are able to close completely, that would be awesome!

steveklabnik commented 3 years ago

Triage: don't have a good way to test this out, since I don't really know anything about sparc64.