ill-typed unused FFI declarations can cause UB

[arielb1]

Current status: Clicky clicky

This compiles and prints "p is not null and 0x0":

pub mod bad {
    #[allow(improper_ctypes)]
    extern {
        pub fn malloc(x: usize) -> &'static mut ();
    }
    
    #[no_mangle]
    pub fn bar() {
        let _m = malloc as unsafe extern "C" fn(usize) -> &'static mut ();
    }
}

pub mod good {
    extern {
        fn malloc(x: usize) -> *const u8;
    }
    
    pub fn foo() {
        unsafe {
            let p = malloc(0x13371337deadbeef); // your computer doesn't have enough memory
            if p.is_null() {
                panic!("p is null");
            } else {
                panic!("p is not null and {:?}", p);
            }
        }
    }
}

fn main() {
    bad::bar();
    good::foo();
}

The problem is that we have two declarations of the "malloc" symbol, but LLVM uses a global namespace for these. So during codegen, the 2nd declaration we generate overwrites the first. In this case, the "ill-typed" malloc declaration (bad::malloc) comes last, up putting a nonnull attribute on malloc, which causes mod good to be miscompiled.

Here's another example that does not involve malloc. It does not get miscompiled currently, but it demonstrates the issue.

[arielb1]

I haven't actually seen this in practice, but in rust-windowing/gl-rs#438 I think someone has (erroneously) declared GetProcAddress as returning a (non-null) function pointer, which might cause other random code in the same executable to break because the erroneous nonnull reaches them.

[nikomatsakis]

Some questions:

• Is it possible for us to have two distinct definitions in LLVM for malloc, such that the call which is not dead does not inherit the non-null attribute?
• Alternatively, or perhaps just additionally, we could detect and lint against the mismatch.

[nikomatsakis]

triage: P-medium

We should fix this, but not a burning issue.

[gnzlbg]

Note that this is not restricted to extern "C", but it also impacts Rust function declarations: https://llvm.godbolt.org/z/03s-aL

I wondered whether, in some cases, LLVM could merge the attributes of the declarations with the definition, introducing UB in safe Rust code like this:

fn foo(x: *mut u8) { ... }
extern "Rust" {
    fn foos_mangled_name(x: &mut u8);
}

The above link shows that, currently, when merging noalias, the noalias of the declaration can end up being removed when a definition without noalias is present in the same module.

It is unclear to me whether making the definition noalias would be a sound optimization of LLVM-IR. noalias is modeled after C's restrict, and in C, the behavior of programs with a declaration like the above, which doesn't match the "restrict"/"noalias" qualifier of the definition, is undefined. So LLVM can assume that it never happens, and if it does, merging noalias in any way would be "ok" for C (not for Rust, and unclear whether it's ok for LLVM-IR, hopefully not).

[gnzlbg]

Alternatively, or perhaps just additionally, we could detect and lint against the mismatch.

@nikomatsakis if the incorrect declaration is in a different crate, could it become visible here due to LTO ? If so, then warning against this would need some kind of whole-program analysis.