rust-lang / rust

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

byval argument of C ABI function passed to Rust ABI function has insufficient alignment #122211

Closed erikdesjardins closed 6 months ago

erikdesjardins commented 6 months ago

I tried this code:

#[repr(align(16))]
pub struct HighAlign {
    x: u128,
    y: u128,
}

extern { fn consume(x: &HighAlign); }

#[no_mangle]
pub unsafe extern "C" fn takes_aligned(x: HighAlign) {
    consume(&x);
}

I expected to see this happen: Alignment requirements are upheld.

Instead, this happened: https://godbolt.org/z/ej65d5hsv

define void @takes_aligned(ptr byval(%HighAlign) align 4 %x) unnamed_addr #0 {
  call void @consume(ptr align 16 %x)
  ret void
}

%x is clearly not align 16 as we indicate. This is unsound.

@rustbot label I-unsound A-ffi


This issue is somewhat similar to #80127. The fix for that issue, #112157, made this problem worse by causing Rust and byval alignments to differ in more cases. (That change is correct, because the Rust and byval alignment must differ to satisfy the ABI. It just uncovered this issue in more cases.)

This is also similar to #112480. However, unlike that one, this issue is practical (and easy) to fix. byval pointers are not exposed to Rust code, so we can just make a copy of the argument into a higher-aligned alloca, without breaking Rust semantics.

In fact, we already do this in the reverse situation, when the Rust alignment is lower than the byval alignment (https://godbolt.org/z/Ksxoo3aja):

#[repr(packed)]
pub struct LowAlign {
    x: u128,
    y: u128,
    z: u128,
}

extern "C" { fn consume(x: LowAlign); }

#[no_mangle]
pub unsafe fn call_low_align(x: LowAlign) {
    consume(x);
}
define void @call_low_align(ptr align 1 %x) unnamed_addr #0 {
  %0 = alloca %LowAlign, align 4
  call void @llvm.memcpy.p0.p0.i32(ptr align 4 %0, ptr align 1 %x, i32 48, i1 false)
  call void @consume(ptr byval(%LowAlign) align 4 %0)
  ret void
}

Meta

rustc --version --verbose:

rustc 1.76.0 (07dca489a 2024-02-04)
binary: rustc
commit-hash: 07dca489ac2d933c78d3c5158e3f43beefeb02ce
commit-date: 2024-02-04
host: x86_64-unknown-linux-gnu
release: 1.76.0
LLVM version: 17.0.6
Compiler returned: 0
erikdesjardins commented 6 months ago

I will open a fix for this shortly.

apiraino commented 6 months ago

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high