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

rustc has wrong signature for C function with 16-byte aligned stack argument in x86_64 Linux #80127

Closed
bobbobbio opened this issue Dec 17, 2020 · 18 comments · Fixed by #112157
Closed
Assignees
Labels
A-FFI Area: Foreign function interface (FFI) 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-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bobbobbio
Copy link

bobbobbio commented Dec 17, 2020

We were writing some code which interacts with a C shared library, but when we were calling a function in the library from Rust, we found our program was crashing. Upon inspection in the debugger, it seems that some of the arguments were getting corrupted. The Rust function signature looked correct so it was surprising. I have managed to reduce the issue down to a minimal repro.

#[repr(C)]
#[repr(align(16))]
#[derive(Debug, Copy, Clone)]
pub struct bar {
    pub a: ::std::os::raw::c_ulong,
    pub b: ::std::os::raw::c_ulong,
}

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct baz {
    pub a: bool,
    pub b: ::std::os::raw::c_uint,
}

extern "C" {
    pub fn foo_func(
        a: *mut ::std::os::raw::c_void,
        b: *mut ::std::os::raw::c_void,
        c: *const ::std::os::raw::c_char,
        d: ::std::os::raw::c_ulong,
        e: bool,
        f: baz,
        g: *mut ::std::os::raw::c_void,
        h: bar,
        i: *mut ::std::os::raw::c_void,
        j: *mut ::std::os::raw::c_void,
        k: *mut ::std::os::raw::c_void,
        l: *mut ::std::os::raw::c_void,
        m: *const ::std::os::raw::c_char,
    ) -> ::std::os::raw::c_int;
}

fn main() {
    let f = baz { a: true, b: 67 };
    let h = bar { a: 0, b: 99 };
    let m = std::ffi::CString::new("Hello, world").unwrap();
    unsafe {
        foo_func(
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            12,
            true,
            f,
            std::ptr::null_mut(),
            h,
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            m.as_ptr(),
        )
    };
}

foo.h

#include <stdbool.h>

struct bar {
    unsigned long a;
    unsigned long b;
}  __attribute__((aligned(16)));

struct baz {
    bool a;
    unsigned b;
};

int
foo_func(
    void *a,
    void *b,
    const char *c,
    unsigned long d,
    bool e,
    struct baz f,
    void *g,
    struct bar h,
    void *i,
    void *j,
    void *k,
    void *l,
    const char *m);

foo.c

#include <foo.h>
#include <stdio.h>

int
foo_func(
    void *a,
    void *b,
    const char *c,
    unsigned long d,
    bool e,
    struct baz f,
    void *g,
    struct bar h,
    void *i,
    void *j,
    void *k,
    void *l,
    const char *m)
{
    printf("m = %s", m);
    return 0;
}

I compiled the C code into a shared library using clang
clang -shared foo.c -o libfoo.so -I. -g

here is the version of clang

clang version 11.0.0
Target: x86_64-unknown-linux-gnu
Thread model: posix

I compiled the Rust code using cargo and rustc 1.46.0 (04488afe3 2020-08-24), but told it to link against libfoo.so
cargo:rustc-link-lib=dylib=foo

I set an rpath on the binary so it can find the library and when I run it, it crashes with a stack overflow

thread 'main' has overflowed its stack
fatal runtime error: stack overflow
fish: “target/debug/bug_repro” terminated by signal SIGABRT (Abort)

Opening it up in the debugger we can see the argument corruption

In this case the stack overflow happens in printf

#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65
#1  0x00007ffff7bf0d8e in __vfprintf_internal (s=0x7ffff7d5d760 <_IO_2_1_stdout_>, format=0x7ffff7dca5f5 "m = %s", ap=ap@entry=0x7fffffffe590, mode_flags=mode_flags@entry=0) at vfprintf-internal.c:1645
#2  0x00007ffff7bda8d8 in __printf (format=<optimized out>) at printf.c:33
#3  0x00007ffff7dca5d0 in foo_func (a=0x0, b=0x0, c=0x0, d=12, e=true, f=..., g=0x0, h=..., i=0x0, j=0x0, k=0x0, l=0x555555815b40, m=0x7fffff7fe000 <error: Cannot access memory at address 0x7fffff7fe000>) at foo.c:20
#4  0x00005555555597b9 in bug_repro::main () at src/main.rs:39

Comparing the local variables and arguments of the two frames

(gdb) info locals
m = std::ffi::c_str::CString {inner: alloc::boxed::Box<[u8]> {data_ptr: 0x555555815b40 "Hello, world\000", length: 13}}
h = bug_repro::bar {a: 0, b: 99}
f = bug_repro::baz {a: true, b: 67}
(gdb) frame 3
#3  0x00007ffff7dca5d0 in foo_func (a=0x0, b=0x0, c=0x0, d=12, e=true, f=..., g=0x0, h=..., i=0x0, j=0x0, k=0x0, l=0x555555815b40, m=0x7fffff7fe000 <error: Cannot access memory at address 0x7fffff7fe000>) at foo.c:20
20          printf("m = %s", m);
(gdb) info args
a = 0x0
b = 0x0
c = 0x0
d = 12
e = true
f = {a = true, b = 67}
g = 0x0
h = {a = 99, b = 0}
i = 0x0
j = 0x0
k = 0x0
l = 0x555555815b40
m = 0x7fffff7fe000 <error: Cannot access memory at address 0x7fffff7fe000>
(gdb)

You can see things start going off the rails starting with argument h which doesn't have the right value, and every argument after that is wrong. Including m which has some garbage stack data, so its no surprise the stack overflowed when printf was reading at m

If we take a look at the llvm-ir we can see the problem

The Rust declaration of the function looks like this

; Function Attrs: nounwind nonlazybind uwtable
declare i32 @foo_func(i8*, i8*, i8*, i64, i1 zeroext, i64, i8*, %bar* noalias nocapture byval(%bar) dereferenceable(16), i8*, i8*, i8*, i8*, i8*) unnamed_addr #3

and the C definition of the function looks like this

; Function Attrs: noinline nounwind optnone uwtable
define dso_local i32 @foo_func(i8* %0, i8* %1, i8* %2, i64 %3, i1 zeroext %4, i64 %5, i8* %6, %struct.bar* byval(%struct.bar) align 16 %7, i8* %8, i8* %9, i8* %10, i8* %11, i8* %12) #0 {

In my limited understanding of this, the only difference is the align 16 for the byval argument which Rust is missing. I can find this mentioned in the LLVM reference

The byval attribute also supports specifying an alignment with the align attribute. It indicates the alignment of the stack slot to form and the known alignment of the pointer specified to the call site. If the alignment is not specified, then the code generator makes a target-specific assumption.

So, since the argument is being passed via the stack, it seems that the alignment of the stack storage can be specified. I wasn't sure at first if this mattered but going back and looking at the assembly produced I was able to determine that it does.

On the C side of the function call we can see that h has this address

(gdb) print &h
$2 = (struct bar *) 0x7fffffffe720

if we subtract 8 from this address, we can see we then get the right value

(gdb) print *(struct bar *)(((void *)&h) - 8)
$5 = {a = 0, b = 99}

The function we are calling is expecting the argument to be at $rbp + 0x20, but the Rust call-site is putting it at $rbp + 0x18 (which is not a 16-byte aligned address) So I assume this missing alignment is causing issues because the caller and callee don't agree where to look on the stack for the argument

This issue has been assigned to @pcwalton via this comment.

@bobbobbio bobbobbio added the C-bug Category: This is a bug. label Dec 17, 2020
@jonas-schievink jonas-schievink added A-FFI Area: Foreign function interface (FFI) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 17, 2020
@tmiasko
Copy link
Contributor

tmiasko commented Dec 18, 2020

That would be:

// FIXME(eddyb) We should be doing this, but at least on
// i686-pc-windows-msvc, it results in wrong stack offsets.
// attrs.pointee_align = Some(self.layout.align.abi);

@Aaron1011 Aaron1011 added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Dec 24, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 24, 2020
@apiraino
Copy link
Contributor

Assigning P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@apiraino apiraino added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 30, 2020
Aaron1011 added a commit to Aaron1011/rust that referenced this issue Jan 8, 2021
Fixes rust-lang#80127

This will require additional testing to see if the old FIXME still
applies
@eddyb
Copy link
Member

eddyb commented Apr 30, 2021

That would be:

// FIXME(eddyb) We should be doing this, but at least on
// i686-pc-windows-msvc, it results in wrong stack offsets.
// attrs.pointee_align = Some(self.layout.align.abi);

@tmiasko Oops, I saw this sooner, but that's very much not it. That's only meant for optimizations, and LLVM makes the mistake of having multiple meanings on one attribute (align causes it to aligns byval stack slots whereas normally it just informs LLVM of alignment guarantees).

I wrote more here, but it looks like we're missing an entire dimension of ABI we weren't aware of (i.e. correct alignment of byval stack slots): #80822 (comment)

@BGR360
Copy link
Contributor

BGR360 commented Jun 16, 2022

Can anybody give a description of what would have to be changed in order to fix this? This just bit us in the butt again in our production code base and we'd like to put some engineering time towards fixing it if it sounds doable.

@bjorn3
Copy link
Member

bjorn3 commented Jun 16, 2022

This I think:

#80822 (comment)

[...]
So what probably needs to happen is what I saying above about making on_stack hold an alignment. And then someone would have to go through every use of byval (make_indirect_byval or otherwise) and see what Clang does in that case for alignment.

@BGR360
Copy link
Contributor

BGR360 commented Jun 16, 2022

What does it mean "see what Clang does in that case for alignment"?

@bjorn3
Copy link
Member

bjorn3 commented Jun 17, 2022

Looking at the abi handling code in clang for the respective calling conventions and copying the logic to set the alignment for arguments at LLVM ir level verbatim.

@BGR360
Copy link
Contributor

BGR360 commented Jun 17, 2022

#80822 (comment)

we should probably also make sure to never emit the align attribute from pointee_align when we emit byval because of on_stack - if there's an align value for byval it should be as part of on_stack and not separate.

@eddyb I don't understand why you think this. Could you elaborate on your reasoning here?

The LLVM language reference states that align has additional (not different) meaning when applied to byval parameters:

byval(<ty>)
...
The byval attribute also supports specifying an alignment with the align attribute. It indicates the alignment of the stack slot to form and the known alignment of the pointer specified to the call site. If the alignment is not specified, then the code generator makes a target-specific assumption.

align <n> or align(<n>)
...
Note that this attribute has additional semantics when combined with the byval or preallocated attribute, which are documented there.

If that's the case, it doesn't seem all that weird to keep letting pointee_align have the effect it does. But I'm very new to this area of rustc so I assume I'm missing something.

@bjorn3
Copy link
Member

bjorn3 commented Jun 17, 2022

I believe @eddyb's suggestion is to at

Indirect { attrs: ArgAttributes, extra_attrs: Option<ArgAttributes>, on_stack: bool },
replace the on_stack field with something like Option<Align> (or maybe better a custom enum) and if it is Some use this alignment value instead of the value in pointee_align. pointee_align is only meant for optimizations I believe (allowing None unconditionally), is not meant to affect the ABI.

@BGR360
Copy link
Contributor

BGR360 commented Jun 17, 2022

on_stack: Option<Align> seems a little wrong to me. It sort of says "please pass this arg on the stack, but don't necessarily require that it's aligned properly unless I say so." Don't we always have to ensure that args are properly aligned when they're passed on the stack?

What if we just changed the implementation of apply_attrs_llfn to always put an align with arg.layout.align().abi?

PassMode::Indirect { ref attrs, extra_attrs: None, on_stack: true } => {
    let i = apply(attrs);
    let byval = llvm::CreateByValAttr(cx.llcx, arg.layout.llvm_type(cx));
-   attributes::apply_to_llfn(llfn, llvm::AttributePlace::Argument(i), &[byval]);
+   let align = llvm::CreateAlignmentAttr(cx.llcx, arg.layout.align().abi);
+   attributes::apply_to_llfn(llfn, llvm::AttributePlace::Argument(i), &[byval, align]);
}

We'd still need to ignore pointee_align when on_stack: true so we don't get two align attributes.

@bjorn3
Copy link
Member

bjorn3 commented Jun 17, 2022

That is incorrect. The call conv alignment doesn't need to match the memory alignment. arg.layout.align() gives the memory alignment.

on_stack: Option seems a little wrong to me. It sort of says "please pass this arg on the stack, but don't necessarily require that it's aligned properly unless I say so." Don't we always have to ensure that args are properly aligned when they're passed on the stack?

The None value is for when the value is not passed at a fixed offset on the stack, but as a pointer to an already correctly aligned memory location. (could be somewhere on the stack and could be on the heap)

@BGR360
Copy link
Contributor

BGR360 commented Jun 17, 2022

I think some wires got crossed in my brain as I was writing that up. I think what I really meant to say is that we don't strictly need to specify an Align when on_stack: true, as we can get it from arg.layout.align().abi if we see that on_stack: true. Is that correct?

Also, general conceptual question: in theory, shouldn't we be able to emit the same LLVM regardless of target architecture / calling convention? Is it just that LLVM IR doesn't truly live up to its "promise" of being target agnostic? Or that we don't trust the LLVM backend to spit out the exact code we want? Or is it nothing to do with LLVM at all and more to do with the fact that there are other backends that we'd like to support?

@bjorn3
Copy link
Member

bjorn3 commented Jun 17, 2022

I think what I really meant to say is that we don't strictly need to specify an Align when on_stack: true, as we can get it from arg.layout.align().abi if we see that on_stack: true. Is that correct?

I'm not sure if that always gives the correct alignment.

Also, general conceptual question: in theory, shouldn't we be able to emit the same LLVM regardless of target architecture / calling convention?

Nope, the frontend is required to handle half of the calling convention. Basically the part to lower from high level types to primitives. LLVM only handles the register assignment and stack layout. If you don't do the half you need to do as frontend, you will get LLVM to split up your structs into one argument for each field recursively, which is almost never the correct calling convention. Rustc made this mistake once for wasm32-unknown-unknown and can't change it because of wasm-bindgen depending on the current abi, so we are stuck with it not being abi compatible with C code compiled by clang. The wasm32-wasi and wasm32-unknown-emscripten targets do use the correct C abi fortunately.

@BGR360
Copy link
Contributor

BGR360 commented Jun 17, 2022

I'm not sure if that always gives the correct alignment.

If not, then what would we use to set on_stack: Some(align) in the first place?

If you don't do the half you need to do as frontend, you will get LLVM to split up your structs into one argument for each field recursively, which is almost never the correct calling convention.

Huh, interesting.

@bjorn3
Copy link
Member

bjorn3 commented Jun 17, 2022

If not, then what would we use to set on_stack: Some(align) in the first place?

That did probably be platform dependent. I unfortunatly can't find where clang does the abi computation.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 2, 2022

Visiting for P-high review

it looks like PR #103830 is on track to fix this.

jyn514 added a commit to jyn514/rust that referenced this issue Dec 31, 2022
rustc_target: Add alignment to indirectly-passed by-value types, correcting the  alignment of `byval` on x86 in the process.

Commit 88e4d2c from five years ago removed
support for alignment on indirectly-passed arguments because of problems with
the `i686-pc-windows-msvc` target. Unfortunately, the `memcpy` optimizations I
recently added to LLVM 16 depend on this to forward `memcpy`s. This commit
attempts to fix the problems with `byval` parameters on that target and now
correctly adds the `align` attribute.

The problem is summarized in [this comment] by `@eddyb.` Briefly, 32-bit x86 has
special alignment rules for `byval` parameters: for the most part, their
alignment is forced to 4. This is not well-documented anywhere but in the Clang
source. I looked at the logic in Clang `TargetInfo.cpp` and tried to replicate
it here. The relevant methods in that file are
`X86_32ABIInfo::getIndirectResult()` and
`X86_32ABIInfo::getTypeStackAlignInBytes()`. The `align` parameter attribute
for `byval` parameters in LLVM must match the platform ABI, or miscompilations
will occur. Note that this doesn't use the approach suggested by eddyb, because
I felt it was overkill to store the alignment in `on_stack` when special
handling is really only needed for 32-bit x86.

As a side effect, this should fix rust-lang#80127, because it will make the `align`
parameter attribute for `byval` parameters match the platform ABI on LLVM
x86-64.

[this comment]: rust-lang#80822 (comment)
@pnkfelix
Copy link
Member

pnkfelix commented Mar 3, 2023

also, assigning this to @pcwalton on assumption that PR #103830 is on track.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 3, 2023

@rustbot assign @pcwalton

@rustbot rustbot self-assigned this Mar 3, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 15, 2023
Resurrect: rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of byval on x86 in the process.

Same as rust-lang#111551, which I [accidentally closed](rust-lang#111551 (comment)) :/

---

This resurrects PR rust-lang#103830, which has sat idle for a while.

Beyond rust-lang#103830, this also:
- fixes byval alignment for types containing vectors on Darwin (see `tests/codegen/align-byval-vector.rs`)
- fixes byval alignment for overaligned types on x86 Windows (see `tests/codegen/align-byval.rs`)
- fixes ABI for types with 128bit requested alignment on ARM64 Linux (see `tests/codegen/aarch64-struct-align-128.rs`)

r? `@nikic`

---

`@pcwalton's` original PR description is reproduced below:

Commit 88e4d2c from five years ago removed
support for alignment on indirectly-passed arguments because of problems with
the `i686-pc-windows-msvc` target. Unfortunately, the `memcpy` optimizations I
recently added to LLVM 16 depend on this to forward `memcpy`s. This commit
attempts to fix the problems with `byval` parameters on that target and now
correctly adds the `align` attribute.

The problem is summarized in [this comment] by `@eddyb.` Briefly, 32-bit x86 has
special alignment rules for `byval` parameters: for the most part, their
alignment is forced to 4. This is not well-documented anywhere but in the Clang
source. I looked at the logic in Clang `TargetInfo.cpp` and tried to replicate
it here. The relevant methods in that file are
`X86_32ABIInfo::getIndirectResult()` and
`X86_32ABIInfo::getTypeStackAlignInBytes()`. The `align` parameter attribute
for `byval` parameters in LLVM must match the platform ABI, or miscompilations
will occur. Note that this doesn't use the approach suggested by eddyb, because
I felt it was overkill to store the alignment in `on_stack` when special
handling is really only needed for 32-bit x86.

As a side effect, this should fix rust-lang#80127, because it will make the `align`
parameter attribute for `byval` parameters match the platform ABI on LLVM
x86-64.

[this comment]: rust-lang#80822 (comment)
@bors bors closed this as completed in 0becc89 Jul 15, 2023
bjorn3 pushed a commit to bjorn3/rust that referenced this issue Jul 22, 2023
Resurrect: rustc_target: Add alignment to indirectly-passed by-value types, correcting the alignment of byval on x86 in the process.

Same as rust-lang#111551, which I [accidentally closed](rust-lang#111551 (comment)) :/

---

This resurrects PR rust-lang#103830, which has sat idle for a while.

Beyond rust-lang#103830, this also:
- fixes byval alignment for types containing vectors on Darwin (see `tests/codegen/align-byval-vector.rs`)
- fixes byval alignment for overaligned types on x86 Windows (see `tests/codegen/align-byval.rs`)
- fixes ABI for types with 128bit requested alignment on ARM64 Linux (see `tests/codegen/aarch64-struct-align-128.rs`)

r? `@nikic`

---

`@pcwalton's` original PR description is reproduced below:

Commit 88e4d2c from five years ago removed
support for alignment on indirectly-passed arguments because of problems with
the `i686-pc-windows-msvc` target. Unfortunately, the `memcpy` optimizations I
recently added to LLVM 16 depend on this to forward `memcpy`s. This commit
attempts to fix the problems with `byval` parameters on that target and now
correctly adds the `align` attribute.

The problem is summarized in [this comment] by `@eddyb.` Briefly, 32-bit x86 has
special alignment rules for `byval` parameters: for the most part, their
alignment is forced to 4. This is not well-documented anywhere but in the Clang
source. I looked at the logic in Clang `TargetInfo.cpp` and tried to replicate
it here. The relevant methods in that file are
`X86_32ABIInfo::getIndirectResult()` and
`X86_32ABIInfo::getTypeStackAlignInBytes()`. The `align` parameter attribute
for `byval` parameters in LLVM must match the platform ABI, or miscompilations
will occur. Note that this doesn't use the approach suggested by eddyb, because
I felt it was overkill to store the alignment in `on_stack` when special
handling is really only needed for 32-bit x86.

As a side effect, this should fix rust-lang#80127, because it will make the `align`
parameter attribute for `byval` parameters match the platform ABI on LLVM
x86-64.

[this comment]: rust-lang#80822 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment